From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgw-sa02.nokia.com ([147.243.1.48]) by bombadil.infradead.org with esmtps (Exim 4.72 #1 (Red Hat Linux)) id 1Or9m6-0003lk-UN for linux-mtd@lists.infradead.org; Thu, 02 Sep 2010 13:24:20 +0000 Date: Thu, 2 Sep 2010 16:24:16 +0300 From: Roman Tereshonkov To: Rohit Hassan Sathyanarayan Subject: Re: [PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support Message-ID: <20100902132416.GA9857@nokia.com> References: <000101cb2f11$86c3f5d0$944be170$%hs@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000101cb2f11$86c3f5d0$944be170$%hs@samsung.com> Cc: linux-mtd@lists.infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > --- > 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 > > +#ifdef CONFIG_MTD_PARTITIONS > +#include > +#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 > --- > >