Patchwork [1/1,MTD] Adding IOCTLs for run-time partitioning support

login
register
mail settings
Submitter Rohit Hassan Sathyanarayan
Date July 29, 2010, 11:30 a.m.
Message ID <000101cb2f11$86c3f5d0$944be170$%hs@samsung.com>
Download mbox | patch
Permalink /patch/60223/
State New
Headers show

Comments

Rohit Hassan Sathyanarayan - July 29, 2010, 11:30 a.m.
Hi All

Sending the patch on MTD for adding four IOCTLs.
MTDPARTITIONCREATE
MTDPARTITIONDELETE
MTDPARTITIONSETPERMISSION
MTDPARTITIONMERGE



Signed-off-by: Rohit HS <rohit.hs@samsung.com>
---
drivers/mtd/mtdchar.c |  126 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/mtd/mtd-abi.h |   17 ++++++
2 files changed, 142 insertions(+), 1 deletion(-)

---
Artem Bityutskiy - Aug. 23, 2010, 2:33 p.m.
On Thu, 2010-07-29 at 17:00 +0530, Rohit Hassan Sathyanarayan wrote:
> Hi All
> 
> Sending the patch on MTD for adding four IOCTLs.
> MTDPARTITIONCREATE
> MTDPARTITIONDELETE
> MTDPARTITIONSETPERMISSION
> MTDPARTITIONMERGE
> 
> 
> 
> Signed-off-by: Rohit HS <rohit.hs@samsung.com>

Could you pleas make scripts/checkpatch.pl happy?

Could you please take a look at the solution from Roman and compare it
to your solution: what is the difference, which one is better? He
submitted the patch earlier than you. And I may be mistaken, but your
solution looks broken, but I did not really look deep enough. See below.

Could you please also take a look at the discussion of Roman's patch and
the request for the interface - Arn asked to have it look as much as
possible as the corresponding ioctl for block devices.

I did not really review your patch, but few things are below.

> +#ifdef CONFIG_MTD_PARTITIONS
> +
> +		case MTDPARTITIONCREATE:
> +		{
> +		struct mtd_partition *pst_minfo = NULL;
> +		struct mtd_partition *pst_minfo_hold = NULL;
> +		struct mtd_partition st_free;
> +		uint64_t u64_userfreeoffset = 0;
> +		uint64_t u64_userfreesize = 0;

Do not prefix variable names with types - kernel people hate this, and
they will hate your patches if you do like this!

General suggestion: when you modify an existing file, stick to the style
used in this file, do not push for your own style. Or change the style
of the whole file.

> +		struct user_mtd *puser_partitions = NULL;
> +		struct user_mtd_info upartition;
> +		int i;

Variable names are too long and cryptic, try to think about s

> +			add_mtd_partitions(mtd, pst_minfo_hold,
> +						upartition.num_partitions);

So 'pst_minfo_hold' (bad name!) becomes the master partition. You
allocate it in this function. But who delets it? When? Could you please
point to the code which kfree()s this object?
Roman Tereshonkov - Sept. 2, 2010, 1:24 p.m.
On Thu, Jul 29, 2010 at 05:00:59PM +0530, Rohit Hassan Sathyanarayan wrote:
> Hi All
> 
> Sending the patch on MTD for adding four IOCTLs.
> MTDPARTITIONCREATE
> MTDPARTITIONDELETE
> MTDPARTITIONSETPERMISSION
> MTDPARTITIONMERGE
> 
> 
> 
> Signed-off-by: Rohit HS <rohit.hs at samsung.com>
> ---
> drivers/mtd/mtdchar.c |  126 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/mtd/mtd-abi.h |   17 ++++++
> 2 files changed, 142 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 8b223c0..704788c 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -22,6 +22,10 @@
> 
> #include <asm/uaccess.h>
> 
> +#ifdef CONFIG_MTD_PARTITIONS
> +#include <linux/mtd/partitions.h>
> +#endif
> +
> #define MTD_INODE_FS_MAGIC 0x11307854
> static struct vfsmount *mtd_inode_mnt __read_mostly;
> 
> @@ -826,6 +830,128 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
> }
> file->f_pos = 0;
> break;
> +#ifdef CONFIG_MTD_PARTITIONS
> +
> +		case MTDPARTITIONCREATE:
> +		{
> +		struct mtd_partition *pst_minfo = NULL;
> +		struct mtd_partition *pst_minfo_hold = NULL;
> +		struct mtd_partition st_free;
> +		uint64_t u64_userfreeoffset = 0;
> +		uint64_t u64_userfreesize = 0;
> +		struct user_mtd *puser_partitions = NULL;
> +		struct user_mtd_info upartition;
> +		int i;
> +
> +			if (copy_from_user(&upartition, argp,
> +				sizeof(struct user_mtd_info))) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			puser_partitions = kzalloc(sizeof(struct user_mtd)*
> +						upartition.num_partitions
> +						, GFP_KERNEL);
> +			pst_minfo = kzalloc(sizeof(struct mtd_partition)*
> +					upartition.num_partitions,
> +					GFP_KERNEL);
> +			pst_minfo_hold = pst_minfo;
> +			if (puser_partitions == NULL) {
> +				ret = -EFAULT;
> +				break;
> +			}

The case puser_partitions == NULL && pst_minfo != NULL leads to the memory leakage.

> +			if (copy_from_user(puser_partitions,
> +					upartition.pst_user_partitions,
> +					sizeof(struct user_mtd)*
> +					upartition.num_partitions)) {
> +				ret = -EFAULT;
> +				break;

What happens with previously allocated memory?

> +			}
> +			for (i = 0; i < upartition.num_partitions; i++) {
> +				pst_minfo->name  = puser_partitions->name;
> +				pst_minfo->size  =
> +					puser_partitions->partition_size;
> +				u64_userfreesize += pst_minfo->size;
> +				pst_minfo->offset =
> +					puser_partitions->partition_offset;
> +				u64_userfreeoffset += pst_minfo->offset;
> +				pst_minfo->mask_flags =
> +					puser_partitions->partition_mask;
> +
> +				puser_partitions++;
> +				pst_minfo++;
> +			}
> +			add_mtd_partitions(mtd, pst_minfo_hold,
> +						upartition.num_partitions);

The function add_mtd_partitions implies to be called only for all mtd partitions at once.
There are some dependences on the partition which goes the first in the passed list.
See add_one_partition function partno argument.

> +			if ((mtd->size - u64_userfreesize) > 0) {
> +				st_free.name	= "FREE";
> +				st_free.size	= mtd->size - u64_userfreesize;
> +				st_free.offset	= u64_userfreesize;
> +				st_free.mask_flags = 0;
> +				add_mtd_partitions(mtd, &st_free, 1);
> +			}
> +		break;
> +		}
> +		case MTDPARTITIONDELETE:
> +		{

How do you free memory allocated in MTDPARTITIONCREATE?

> +		struct mtd_info *pst_del_info = NULL;
> +		uint32_t mtd_partition_number;
> +
> +			if (copy_from_user(&mtd_partition_number, argp,
> +					sizeof(mtd_partition_number))) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			pst_del_info = get_mtd_device(NULL,
> +					mtd_partition_number);
> +
> +			if (pst_del_info != NULL) {
> +				put_mtd_device(pst_del_info);
> +				del_mtd_device(pst_del_info);
> +			}
> +			break;
> +		}

If the partition is used del_mtd_device returns EBUSY.
Should the user space be informed about this?

> +		case MTDPARTITIONSETPERMISSION:
> +		{
> +		uint32_t u32_mask_flags;
> +		struct mtd_info *pst_device = NULL;
> +		int i32_dev_num;
> +
> +			if (copy_from_user(&u32_mask_flags, argp,
> +					sizeof(u32_mask_flags))) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			i32_dev_num = (u32_mask_flags & 0x0000FFFF);
> +			u32_mask_flags = (u32_mask_flags & 0xFFFF0000) >> 16;
> +			pst_device = get_mtd_device(NULL, i32_dev_num);
> +			pst_device->flags = u32_mask_flags;
> +			break;
> +		}
> +		case MTDPARTITIONMERGE:

MERGE = DELETE + CREATE
Is this ioctl really needed?

> +		{
> +		struct mtd_info *pst_merge_device1 = NULL;
> +		struct mtd_info *pst_merge_device2 = NULL;
> +		unsigned char partition_number[2];
> +
> +			if (copy_from_user(partition_number, argp, 2)) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			pst_merge_device1 = get_mtd_device(NULL,
> +							partition_number[0]);
> +			pst_merge_device2 = get_mtd_device(NULL,
> +							partition_number[1]);

The case when only one of pst_merge_device1 and pst_merge_device2 is non-NULL
leads to the unreleased mtd_device.

> +			if ((pst_merge_device1 != NULL) &&
> +				(pst_merge_device2 != NULL)) {
> +				pst_merge_device1->size +=
> +						pst_merge_device2->size;
> +				put_mtd_device(pst_merge_device2);
> +				del_mtd_device(pst_merge_device2);

pst_merge_device1 should be released probably.

> +			}
> +			break;
> +		}
> +#endif
> +
> }
> 
> default:
> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> index be51ae2..85168f2 100644
> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -30,6 +30,18 @@ struct mtd_oob_buf64 {
> __u64 usr_ptr;
> };
> 
> +struct user_mtd {
> +	char name[32];
> +	 __u64 partition_size;
> +	 __u64 partition_offset;
> +	 __u32 partition_mask;
> +};
> +
> +struct user_mtd_info {
> +	struct user_mtd *pst_user_partitions;
> +	__u32 num_partitions;
> +};
> +
> #define MTD_ABSENT		0
> #define MTD_RAM			1
> #define MTD_ROM			2
> @@ -110,7 +122,10 @@ struct otp_info {
> #define MEMERASE64		_IOW('M', 20, struct erase_info_user64)
> #define MEMWRITEOOB64		_IOWR('M', 21, struct mtd_oob_buf64)
> #define MEMREADOOB64		_IOWR('M', 22, struct mtd_oob_buf64)
> -
> +#define MTDPARTITIONCREATE      _IOW('M', 23, int)
> +#define MTDPARTITIONDELETE      _IOW('M', 24, int)
> +#define MTDPARTITIONSETPERMISSION _IOW('M', 26, int)
> +#define MTDPARTITIONMERGE       _IOW('M', 27, int)
> /*
> * Obsolete legacy interface. Keep it in order not to break userspace
> * interfaces
> ---
> 
>

Patch

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 8b223c0..704788c 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -22,6 +22,10 @@ 
 
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_MTD_PARTITIONS
+#include <linux/mtd/partitions.h>
+#endif
+
 #define MTD_INODE_FS_MAGIC 0x11307854
 static struct vfsmount *mtd_inode_mnt __read_mostly;
 
@@ -826,6 +830,128 @@  static int mtd_ioctl(struct inode *inode, struct file *file,
 		}
 		file->f_pos = 0;
 		break;
+#ifdef CONFIG_MTD_PARTITIONS
+
+		case MTDPARTITIONCREATE:
+		{
+		struct mtd_partition *pst_minfo = NULL;
+		struct mtd_partition *pst_minfo_hold = NULL;
+		struct mtd_partition st_free;
+		uint64_t u64_userfreeoffset = 0;
+		uint64_t u64_userfreesize = 0;
+		struct user_mtd *puser_partitions = NULL;
+		struct user_mtd_info upartition;
+		int i;
+
+			if (copy_from_user(&upartition, argp,
+				sizeof(struct user_mtd_info))) {
+				ret = -EFAULT;
+				break;
+			}
+			puser_partitions = kzalloc(sizeof(struct user_mtd)*
+						upartition.num_partitions
+						, GFP_KERNEL);
+			pst_minfo = kzalloc(sizeof(struct mtd_partition)*
+					upartition.num_partitions,
+					GFP_KERNEL);
+			pst_minfo_hold = pst_minfo;
+			if (puser_partitions == NULL) {
+				ret = -EFAULT;
+				break;
+			}
+			if (copy_from_user(puser_partitions,
+					upartition.pst_user_partitions,
+					sizeof(struct user_mtd)*
+					upartition.num_partitions)) {
+				ret = -EFAULT;
+				break;
+			}
+			for (i = 0; i < upartition.num_partitions; i++) {
+				pst_minfo->name  = puser_partitions->name;
+				pst_minfo->size  =
+					puser_partitions->partition_size;
+				u64_userfreesize += pst_minfo->size;
+				pst_minfo->offset =
+					puser_partitions->partition_offset;
+				u64_userfreeoffset += pst_minfo->offset;
+				pst_minfo->mask_flags =
+					puser_partitions->partition_mask;
+
+				puser_partitions++;
+				pst_minfo++;
+			}
+			add_mtd_partitions(mtd, pst_minfo_hold,
+						upartition.num_partitions);
+			if ((mtd->size - u64_userfreesize) > 0) {
+				st_free.name	= "FREE";
+				st_free.size	= mtd->size - u64_userfreesize;
+				st_free.offset	= u64_userfreesize;
+				st_free.mask_flags = 0;
+				add_mtd_partitions(mtd, &st_free, 1);
+			}
+		break;
+		}
+		case MTDPARTITIONDELETE:
+		{
+		struct mtd_info *pst_del_info = NULL;
+		uint32_t mtd_partition_number;
+
+			if (copy_from_user(&mtd_partition_number, argp,
+					sizeof(mtd_partition_number))) {
+				ret = -EFAULT;
+				break;
+			}
+			pst_del_info = get_mtd_device(NULL,
+					mtd_partition_number);
+
+			if (pst_del_info != NULL) {
+				put_mtd_device(pst_del_info);
+				del_mtd_device(pst_del_info);
+			}
+			break;
+		}
+		case MTDPARTITIONSETPERMISSION:
+		{
+		uint32_t u32_mask_flags;
+		struct mtd_info *pst_device = NULL;
+		int i32_dev_num;
+
+			if (copy_from_user(&u32_mask_flags, argp,
+					sizeof(u32_mask_flags))) {
+				ret = -EFAULT;
+				break;
+			}
+			i32_dev_num = (u32_mask_flags & 0x0000FFFF);
+			u32_mask_flags = (u32_mask_flags & 0xFFFF0000) >> 16;
+			pst_device = get_mtd_device(NULL, i32_dev_num);
+			pst_device->flags = u32_mask_flags;
+			break;
+		}
+		case MTDPARTITIONMERGE:
+		{
+		struct mtd_info *pst_merge_device1 = NULL;
+		struct mtd_info *pst_merge_device2 = NULL;
+		unsigned char partition_number[2];
+
+			if (copy_from_user(partition_number, argp, 2)) {
+				ret = -EFAULT;
+				break;
+			}
+			pst_merge_device1 = get_mtd_device(NULL,
+							partition_number[0]);
+			pst_merge_device2 = get_mtd_device(NULL,
+							partition_number[1]);
+			if ((pst_merge_device1 != NULL) &&
+				(pst_merge_device2 != NULL)) {
+				pst_merge_device1->size +=
+						pst_merge_device2->size;
+				put_mtd_device(pst_merge_device2);
+				del_mtd_device(pst_merge_device2);
+			}
+			break;
+		}
+#endif
+
 	}
 
 	default:
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index be51ae2..85168f2 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -30,6 +30,18 @@  struct mtd_oob_buf64 {
 	__u64 usr_ptr;
 };
 
+struct user_mtd {
+	char name[32];
+	 __u64 partition_size;
+	 __u64 partition_offset;
+	 __u32 partition_mask;
+};
+
+struct user_mtd_info {
+	struct user_mtd *pst_user_partitions;
+	__u32 num_partitions;
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -110,7 +122,10 @@  struct otp_info {
 #define MEMERASE64		_IOW('M', 20, struct erase_info_user64)
 #define MEMWRITEOOB64		_IOWR('M', 21, struct mtd_oob_buf64)
 #define MEMREADOOB64		_IOWR('M', 22, struct mtd_oob_buf64)
-
+#define MTDPARTITIONCREATE      _IOW('M', 23, int)
+#define MTDPARTITIONDELETE      _IOW('M', 24, int)
+#define MTDPARTITIONSETPERMISSION _IOW('M', 26, int)
+#define MTDPARTITIONMERGE       _IOW('M', 27, int)
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
  * interfaces