Patchwork [1/4] mtd: add new ioctl structures to be used for repartitioning

login
register
mail settings
Submitter Roman Tereshonkov
Date June 18, 2010, 10:08 a.m.
Message ID <1276855711-18570-2-git-send-email-roman.tereshonkov@nokia.com>
Download mbox | patch
Permalink /patch/56163/
State New
Headers show

Comments

Roman Tereshonkov - June 18, 2010, 10:08 a.m.
Signed-off-by: Roman Tereshonkov <roman.tereshonkov@nokia.com>
---
 include/mtd/mtd-abi.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
Artem Bityutskiy - July 18, 2010, 5 p.m.
CCing Arnd to review the ioctl interface.

One thing just struck me, see below.

On Fri, 2010-06-18 at 13:08 +0300, Roman Tereshonkov wrote:
> Signed-off-by: Roman Tereshonkov <roman.tereshonkov@nokia.com>
> ---
>  include/mtd/mtd-abi.h |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> index be51ae2..c2c6b41 100644
> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -88,6 +88,20 @@ struct otp_info {
>  	__u32 locked;
>  };
>  
> +#define MTD_MAX_PARTITION_NAME_LEN	64
> +struct mtd_partition_user {
> +	__u64 size;
> +	__u64 offset;
> +	__u32 mask_flags;
> +	char name[MTD_MAX_PARTITION_NAME_LEN];
> +	__u8 padding[128];	/* reserved for future, must be zero! */
> +};
> +
> +struct mtd_partitions {
> +	__u32 nparts;
> +	struct mtd_partition_user __user *parts;
> +};

Hmm, I think nowadays pointers should be passed as __u64 and
compat_ioctl() should be avoided.

Also please, document the ioctl a little - add some comments.

> +
>  #define MEMGETINFO		_IOR('M', 1, struct mtd_info_user)
>  #define MEMERASE		_IOW('M', 2, struct erase_info_user)
>  #define MEMWRITEOOB		_IOWR('M', 3, struct mtd_oob_buf)
> @@ -110,6 +124,7 @@ 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 MTDREPARTITION		_IOW('M', 23, struct mtd_partitions)
>  
>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace
Arnd Bergmann - July 18, 2010, 8:22 p.m.
On Sunday 18 July 2010, Artem Bityutskiy wrote:
> CCing Arnd to review the ioctl interface.
> 
> One thing just struck me, see below.
> 
> On Fri, 2010-06-18 at 13:08 +0300, Roman Tereshonkov wrote:
> > Signed-off-by: Roman Tereshonkov <roman.tereshonkov@nokia.com>
> > ---
> >  include/mtd/mtd-abi.h |   15 +++++++++++++++
> >  1 files changed, 15 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> > index be51ae2..c2c6b41 100644
> > --- a/include/mtd/mtd-abi.h
> > +++ b/include/mtd/mtd-abi.h
> > @@ -88,6 +88,20 @@ struct otp_info {
> >  	__u32 locked;
> >  };
> >  
> > +#define MTD_MAX_PARTITION_NAME_LEN	64
> > +struct mtd_partition_user {
> > +	__u64 size;
> > +	__u64 offset;
> > +	__u32 mask_flags;
> > +	char name[MTD_MAX_PARTITION_NAME_LEN];
> > +	__u8 padding[128];	/* reserved for future, must be zero! */
> > +};
> > +
> > +struct mtd_partitions {
> > +	__u32 nparts;
> > +	struct mtd_partition_user __user *parts;
> > +};
> 
> Hmm, I think nowadays pointers should be passed as __u64 and
> compat_ioctl() should be avoided.

Yes, that's generally true. It would be nice if the BLKPG ioctl
definition could be reused for this. It is more complicated
than it should be, but not more than this suggestion, and
it's an existing ioctl.

	Arnd
Artem Bityutskiy - July 19, 2010, 4:06 a.m.
On Sun, 2010-07-18 at 20:22 +0000, Arnd Bergmann wrote:
> > > +#define MTD_MAX_PARTITION_NAME_LEN	64
> > > +struct mtd_partition_user {
> > > +	__u64 size;
> > > +	__u64 offset;
> > > +	__u32 mask_flags;
> > > +	char name[MTD_MAX_PARTITION_NAME_LEN];
> > > +	__u8 padding[128];	/* reserved for future, must be zero! */
> > > +};
> > > +
> > > +struct mtd_partitions {
> > > +	__u32 nparts;
> > > +	struct mtd_partition_user __user *parts;
> > > +};
> > 
> > Hmm, I think nowadays pointers should be passed as __u64 and
> > compat_ioctl() should be avoided.
> 
> Yes, that's generally true. It would be nice if the BLKPG ioctl
> definition could be reused for this. It is more complicated
> than it should be, but not more than this suggestion, and
> it's an existing ioctl.

Thanks for reply.

MTD devices do not support BLKPG, do you mean we should you the same
data-structures and names as block devices?
Arnd Bergmann - July 20, 2010, 9:19 a.m.
On Monday 19 July 2010, Artem Bityutskiy wrote:
> On Sun, 2010-07-18 at 20:22 +0000, Arnd Bergmann wrote:
> > Yes, that's generally true. It would be nice if the BLKPG ioctl
> > definition could be reused for this. It is more complicated
> > than it should be, but not more than this suggestion, and
> > it's an existing ioctl.
> 
> Thanks for reply.
> 
> MTD devices do not support BLKPG, do you mean we should you the same
> data-structures and names as block devices?

Yes. I'm not sure if it should also be possible to actually repartition
the flash using the mtdblock driver, but what I meant was to implement
the BLKPG API in the mtdchar driver, with slightly adapted semantics.

	Arnd
Artem Bityutskiy - July 20, 2010, 9:25 a.m.
On Tue, 2010-07-20 at 11:19 +0200, Arnd Bergmann wrote:
> On Monday 19 July 2010, Artem Bityutskiy wrote:
> > On Sun, 2010-07-18 at 20:22 +0000, Arnd Bergmann wrote:
> > > Yes, that's generally true. It would be nice if the BLKPG ioctl
> > > definition could be reused for this. It is more complicated
> > > than it should be, but not more than this suggestion, and
> > > it's an existing ioctl.
> > 
> > Thanks for reply.
> > 
> > MTD devices do not support BLKPG, do you mean we should you the same
> > data-structures and names as block devices?
> 
> Yes. I'm not sure if it should also be possible to actually repartition
> the flash using the mtdblock driver, but what I meant was to implement
> the BLKPG API in the mtdchar driver, with slightly adapted semantics.

No, using mtdblock is not feasible, because of several reasons like that
it is a debugging driver which does not handle bad blocks, and it works
on top of mtd, has no idea about mtd internals, and all the partitions
logic is inside mtd, and not available for mtdblock.

But yes, using the BLKPG API sounds like a reasonable idea, thanks.

Patch

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index be51ae2..c2c6b41 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -88,6 +88,20 @@  struct otp_info {
 	__u32 locked;
 };
 
+#define MTD_MAX_PARTITION_NAME_LEN	64
+struct mtd_partition_user {
+	__u64 size;
+	__u64 offset;
+	__u32 mask_flags;
+	char name[MTD_MAX_PARTITION_NAME_LEN];
+	__u8 padding[128];	/* reserved for future, must be zero! */
+};
+
+struct mtd_partitions {
+	__u32 nparts;
+	struct mtd_partition_user __user *parts;
+};
+
 #define MEMGETINFO		_IOR('M', 1, struct mtd_info_user)
 #define MEMERASE		_IOW('M', 2, struct erase_info_user)
 #define MEMWRITEOOB		_IOWR('M', 3, struct mtd_oob_buf)
@@ -110,6 +124,7 @@  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 MTDREPARTITION		_IOW('M', 23, struct mtd_partitions)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace