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

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

[Rohit Hassan Sathyanarayan]

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(-)

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
---

Re: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support

[Roman Tereshonkov]

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
> ---
> 
> 

RE: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support

[Rohit Hassan Sathyanarayan]

Hi Artem,

> -----Original Message-----
> From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
> Sent: Monday, August 23, 2010 8:04 PM
> To: Rohit Hassan Sathyanarayan
> Cc: linux-mtd@lists.infradead.org; 'David Woodhouse'; v.dalal@samsung.com
> Subject: Re: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support
> 
> 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?
> 
  I checked it with scripts/checkpatch.pl but didn't faced any problem.
  I will surely recheck it.
> 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.
> 
  Functionally both are same.
  But implementation differences are,
  Roman's patch passes the mtd to be partitioned by open mtd node itself. 
  Ex: If partitions are to be created on mtd4,then mtd4 itself is opened and
      Partitions are created and then mtd4 is deleted.

  In my patch, the mtd on which partitions are to be created is passed as 
  argument in MTDPARTITIONCREATE ioctl. 
  For changing from user to kernel space and access mtd, default read only
  mtd Partition is opened.
  Ex: If partitions are to be created on /dev/mtd4,then /dev/mtd0 is opened
      and mtd4 information is passed in MTDPARTITIONCREATE ioctl argument.

  Also permissions of the partitions can be changed with my patch.
  This patch also works independent of the device. I have tested this with
  Nand , OneNand and FlexOnenand.
  Implementation in kernel is minimal in my patch.  

  MTDPARTITIONMERGE is just another option, advantage of merge option
  over(delete+delete+create) is definitely number of system calls.     
  
> 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?
> 
  kfree() is at end of ioctl(MTDPARTITIONCREATE),by mistake it was not generated in patch. 

> --
> Best Regards,
> Artem Bityutskiy (Артём Битюцкий)

I have modified the patch as per your comments,

Regards,
Rohit.H.S




Signed-off-by: Rohit HS <rohit.hs@samsung.com>
---
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 91c8013..160538d 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -22,6 +22,12 @@
 
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_MTD_PARTITIONS
+#include <linux/mtd/partitions.h>
+#define MTD_PART_DEV_NUM	0x0000FFFF
+#define MTD_PART_FLAGS		0xFFFF0000
+#endif
+
 #define MTD_INODE_FS_MAGIC 0x11307854
 static struct vfsmount *mtd_inode_mnt __read_mostly;
 
@@ -471,7 +477,15 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 	int ret = 0;
 	u_long size;
 	struct mtd_info_user info;
-
+#ifdef CONFIG_MTD_PARTITIONS
+	struct mtd_partition	*pmtd;
+	struct mtd_partition	*pmtd1;
+	struct mtd_partition	free;
+	struct mtd_info *pinfo1;
+	struct mtd_info *pinfo2;
+	struct user_mtd	*ppart = NULL;
+	struct user_mtd_info	upart;
+#endif
 	DEBUG(MTD_DEBUG_LEVEL0, "MTD_ioctl\n");
 
 	size = (cmd & IOCSIZE_MASK) >> IOCSIZE_SHIFT;
@@ -825,6 +839,117 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 		}
 		file->f_pos = 0;
 		break;
+#ifdef CONFIG_MTD_PARTITIONS
+
+	case MTDPARTITIONCREATE:
+	{
+		u_int64_t	uoffset = 0;
+		u_int64_t	usize = 0;
+		u_int32_t	i;
+
+			if (copy_from_user(&upart, argp,
+				sizeof(struct user_mtd_info))) {
+				ret = -EFAULT;
+				break;
+			}
+			ppart = kzalloc(sizeof(struct user_mtd)*
+						upart.num_partitions
+						, GFP_KERNEL);
+			pmtd = kzalloc(sizeof(struct mtd_partition)*
+					upart.num_partitions,
+					GFP_KERNEL);
+			pmtd1 = pmtd;
+			if (ppart == NULL) {
+				ret = -EFAULT;
+				break;
+			}
+			if (copy_from_user(ppart,
+					upart.pst_user_partitions,
+					sizeof(struct user_mtd)*
+					upart.num_partitions)) {
+				ret = -EFAULT;
+				break;
+			}
+			for (i = 0; i < upart.num_partitions; i++) {
+				pmtd->name  = ppart->name;
+				pmtd->size  =
+					ppart->partition_size;
+				usize += pmtd->size;
+				pmtd->offset =
+					ppart->partition_offset;
+				uoffset += pmtd->offset;
+				pmtd->mask_flags =
+					ppart->partition_mask;
+
+				ppart++;
+				pmtd++;
+			}
+			add_mtd_partitions(mtd, pmtd1,
+						upart.num_partitions);
+			if ((mtd->size - usize) > 0) {
+				free.name	= "FREE";
+				free.size	= mtd->size - usize;
+				free.offset	= usize;
+				free.mask_flags = 0;
+				add_mtd_partitions(mtd, &free, 1);
+			}
+			kfree(pmtd1);
+		break;
+	}
+	case MTDPARTITIONDELETE:
+	{
+		u_int32_t mtd_num;
+
+			if (copy_from_user(&mtd_num, argp,
+					sizeof(mtd_num))) {
+				ret = -EFAULT;
+				break;
+			}
+			pinfo1 = get_mtd_device(NULL,
+					mtd_num);
+
+			if (pinfo1 != NULL) {
+				put_mtd_device(pinfo1);
+				del_mtd_device(pinfo1);
+			}
+		break;
+	}
+	case MTDPARTITIONSETPERMISSION:
+	{
+		u_int32_t flags;
+		int dev_num;
+
+			if (copy_from_user(&flags, argp,
+					sizeof(flags))) {
+				ret = -EFAULT;
+				break;
+			}
+			dev_num = (flags & MTD_PART_DEV_NUM);
+			flags = (flags & MTD_PART_FLAGS) >> 16;
+			pinfo1 = get_mtd_device(NULL, dev_num);
+			pinfo1->flags = flags;
+		break;
+	}
+	case MTDPARTITIONMERGE:
+	{
+		u_int8_t part_num[2];
+
+			if (copy_from_user(part_num, argp, 2)) {
+				ret = -EFAULT;
+				break;
+			}
+			pinfo1 = get_mtd_device(NULL, part_num[0]);
+			pinfo2 = get_mtd_device(NULL, part_num[1]);
+			if ((pinfo1 != NULL) &&
+				(pinfo2 != NULL)) {
+				pinfo1->size +=	pinfo2->size;
+				put_mtd_device(pinfo2);
+				del_mtd_device(pinfo2);
+			}
+		break;
+	}
+#endif
+
 	}
 
 	default:
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index be51ae2..cfebc75 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -30,6 +30,19 @@ 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 +123,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
---

RE: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support

[Artem Bityutskiy]

On Mon, 2010-09-06 at 08:04 +0530, Rohit Hassan Sathyanarayan wrote:
> +	case MTDPARTITIONCREATE:
> +	{
> +		u_int64_t	uoffset = 0;
> +		u_int64_t	usize = 0;
> +		u_int32_t	i;
> +
> +			if (copy_from_user(&upart, argp,
> +				sizeof(struct user_mtd_info))) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			ppart = kzalloc(sizeof(struct user_mtd)*
> +						upart.num_partitions
> +						, GFP_KERNEL);
> +			pmtd = kzalloc(sizeof(struct mtd_partition)*
> +					upart.num_partitions,
> +					GFP_KERNEL);
> +			pmtd1 = pmtd;
> +			if (ppart == NULL) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			if (copy_from_user(ppart,
> +					upart.pst_user_partitions,
> +					sizeof(struct user_mtd)*
> +					upart.num_partitions)) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +			for (i = 0; i < upart.num_partitions; i++) {
> +				pmtd->name  = ppart->name;
> +				pmtd->size  =
> +					ppart->partition_size;
> +				usize += pmtd->size;
> +				pmtd->offset =
> +					ppart->partition_offset;
> +				uoffset += pmtd->offset;
> +				pmtd->mask_flags =
> +					ppart->partition_mask;
> +
> +				ppart++;
> +				pmtd++;
> +			}
> +			add_mtd_partitions(mtd, pmtd1,
> +						upart.num_partitions);
> +			if ((mtd->size - usize) > 0) {
> +				free.name	= "FREE";
> +				free.size	= mtd->size - usize;
> +				free.offset	= usize;
> +				free.mask_flags = 0;
> +				add_mtd_partitions(mtd, &free, 1);
> +			}
> +			kfree(pmtd1);
> +		break;
> +	}

So what happens if you already have many partitions, and this ioctl is
called? Where these existing partitions and the existing master device
will go? Who will free the resources occupied by them and where?

> +struct user_mtd_info {
> +	struct user_mtd *pst_user_partitions;
> +	__u32 num_partitions;
> +};

I suggested you to look at the discussion of Roman's patches - we did
discuss there that pointers in ioctls are bad, because then you have to
have implement compat_ioctl. You need to use u64 to avoid problems in
64-bit kernel + 32-bit user-space environments.


-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)