Patchwork [1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

login
register
mail settings
Submitter Mike Dunn
Date Dec. 20, 2011, 6:42 p.m.
Message ID <1324406536-18250-2-git-send-email-mikedunn@newsguy.com>
Download mbox | patch
Permalink /patch/132495/
State New
Headers show

Comments

Mike Dunn - Dec. 20, 2011, 6:42 p.m.
Ensure driver methods always go through the wrapper functions in the
partitioning code by creating a single "partition" on otherwise unpartitioned
devices.

Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
out-of-tree).

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/mtdcore.c |   17 ++++++++++++++---
 drivers/mtd/mtdpart.c |   10 +++++++---
 2 files changed, 21 insertions(+), 6 deletions(-)
Artem Bityutskiy - Dec. 22, 2011, 12:34 p.m.
On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
> Ensure driver methods always go through the wrapper functions in the
> partitioning code by creating a single "partition" on otherwise unpartitioned
> devices.
> 
> Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
> out-of-tree).
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

Looks good to me in general. Does it change anything from the user-space
POW (API/ABI)?

One thing to be aware of: some drivers do like this:

drivers/mtd/maps/plat-ram.c:

        /* check to see if there are any available partitions, or wether
         * to add this device whole */

        err = mtd_device_parse_register(info->mtd, pdata->probes, 0,
                        pdata->partitions, pdata->nr_partitions);
        if (!err)
                dev_info(&pdev->dev, "registered mtd device\n");

        if (pdata->nr_partitions) {
                /* add the whole device. */
                err = mtd_device_register(info->mtd, NULL, 0);
                if (err) {
                        dev_err(&pdev->dev,
                                "failed to register the entire device\n");
                }
        }

Could you please:
1. Try to find drivers like this and kill the redundant mtd_device_register()
   call.
2. There is no guarantee you will find all. So you could add a check in
   mtd_device_parse_register() which would print a warning if the device is
   added for the second time and return. So the code would work and people
   would be able to notice the warning and fix up the driver.

> -		add_mtd_device(&slave->mtd);
> +		ret = add_mtd_device(&slave->mtd);
> +		if (ret == 1)
> +			return -ENODEV;

Is this an unrelated change? Would you separate it out?
Artem Bityutskiy - Dec. 22, 2011, 1:05 p.m.
On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
> +		/*
> +		 * For unpartitioned devices, create a single "partition" that
> +		 * spans the entire device, so that driver methods go through
> +		 * partition wrappers in all cases.
> +		 */
> +		struct mtd_partition single_part = {
> +			.name = (char *)mtd->name,
> +			.offset = 0,
> +			.size = mtd->size,
> +			.mask_flags = 0,
> +			.ecclayout = mtd->ecclayout,
> +		};
> +		err = add_mtd_partitions(mtd, &single_part, 1);
>  	}

There is a problem with this approach :-(

Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition
MTD device from user-space. It is a bit strange, but we use block device
for re-partition in order to not invent new ioctls.

This function has this code:

                /* Only master mtd device must be used to add partitions */
                if (mtd_is_partition(mtd))
                        return -EINVAL;

and your patch  will brake re-partitioning.

So I guess we should go back to your first patch where you just change
the ->read() / ->read_oob() interface.

Or:

1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.

2. In the new functions you may do the stuff you do at patch 2.

For me the second approach is more plausible.
Artem Bityutskiy - Dec. 22, 2011, 6:03 p.m.
On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote:
> 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
> functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.

I will send a patch-set for this change tomorrow.

> 2. In the new functions you may do the stuff you do at patch 2.

Then you will be able to do this change easily.
Mike Dunn - Dec. 22, 2011, 11:28 p.m.
On 12/22/2011 04:34 AM, Artem Bityutskiy wrote:
> On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
>> Ensure driver methods always go through the wrapper functions in the
>> partitioning code by creating a single "partition" on otherwise unpartitioned
>> devices.
>>
>> Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
>> out-of-tree).
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> Looks good to me in general. Does it change anything from the user-space
> POW (API/ABI)?


No, should be completely transparent (at least that's my intention).  The only
new behavior is that the partition code puts a string in the log with the
partition boundaries and partition name, which in the single partition case is
mtd->name, which should avoid any confusion.


> One thing to be aware of: some drivers do like this:
>
> drivers/mtd/maps/plat-ram.c:
>
>         /* check to see if there are any available partitions, or wether
>          * to add this device whole */
>
>         err = mtd_device_parse_register(info->mtd, pdata->probes, 0,
>                         pdata->partitions, pdata->nr_partitions);
>         if (!err)
>                 dev_info(&pdev->dev, "registered mtd device\n");
>
>         if (pdata->nr_partitions) {
>                 /* add the whole device. */
>                 err = mtd_device_register(info->mtd, NULL, 0);
>                 if (err) {
>                         dev_err(&pdev->dev,
>                                 "failed to register the entire device\n");
>                 }
>         }
>
> Could you please:
> 1. Try to find drivers like this and kill the redundant mtd_device_register()
>    call.
> 2. There is no guarantee you will find all. So you could add a check in
>    mtd_device_parse_register() which would print a warning if the device is
>    added for the second time and return. So the code would work and people
>    would be able to notice the warning and fix up the driver.


OK.  Sorry, missed that.


>> -		add_mtd_device(&slave->mtd);
>> +		ret = add_mtd_device(&slave->mtd);
>> +		if (ret == 1)
>> +			return -ENODEV;
> Is this an unrelated change? Would you separate it out?


No, it's related.  Currently, mtd_device_parse_register() does the above if no
partitions are created, whereas here in add_mtd_partitions() the return code of
add_mtd_device() is not checked, so I added this to be consistent with the
previous behavior.  mtd_device_parse_register() propagates this return code back
to the caller, so the result is the same.

Thanks,
Mike
Mike Dunn - Dec. 22, 2011, 11:28 p.m.
On 12/22/2011 05:05 AM, Artem Bityutskiy wrote:
> On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
>> +		/*
>> +		 * For unpartitioned devices, create a single "partition" that
>> +		 * spans the entire device, so that driver methods go through
>> +		 * partition wrappers in all cases.
>> +		 */
>> +		struct mtd_partition single_part = {
>> +			.name = (char *)mtd->name,
>> +			.offset = 0,
>> +			.size = mtd->size,
>> +			.mask_flags = 0,
>> +			.ecclayout = mtd->ecclayout,
>> +		};
>> +		err = add_mtd_partitions(mtd, &single_part, 1);
>>  	}
> There is a problem with this approach :-(
>
> Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition
> MTD device from user-space. It is a bit strange, but we use block device
> for re-partition in order to not invent new ioctls.


Sorry, missed that too.  So many perils...

Thanks,
Mike
Mike Dunn - Dec. 22, 2011, 11:28 p.m.
On 12/22/2011 10:03 AM, Artem Bityutskiy wrote:
> On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote:
>> 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
>> functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.
> I will send a patch-set for this change tomorrow.
>
>> 2. In the new functions you may do the stuff you do at patch 2.
> Then you will be able to do this change easily.
>



OK, thank you Artem.  I'll watch for it and follow up with a reworked patch 2.

Mike
Artem Bityutskiy - Dec. 23, 2011, 2:47 p.m.
On Thu, 2011-12-22 at 15:28 -0800, Mike Dunn wrote:
> OK, thank you Artem.  I'll watch for it and follow up with a reworked patch 2.

I'm on it, but I think I've under-estimated the amount of work
and it won't be ready today.

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index b01993e..4ac1962 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -477,10 +477,21 @@  int mtd_device_parse_register(struct mtd_info *mtd, const char **types,
 	if (err > 0) {
 		err = add_mtd_partitions(mtd, real_parts, err);
 		kfree(real_parts);
+
 	} else if (err == 0) {
-		err = add_mtd_device(mtd);
-		if (err == 1)
-			err = -ENODEV;
+		/*
+		 * For unpartitioned devices, create a single "partition" that
+		 * spans the entire device, so that driver methods go through
+		 * partition wrappers in all cases.
+		 */
+		struct mtd_partition single_part = {
+			.name = (char *)mtd->name,
+			.offset = 0,
+			.size = mtd->size,
+			.mask_flags = 0,
+			.ecclayout = mtd->ecclayout,
+		};
+		err = add_mtd_partitions(mtd, &single_part, 1);
 	}
 
 	return err;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index a0bd2de..81975a5 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -665,9 +665,11 @@  int add_mtd_partitions(struct mtd_info *master,
 {
 	struct mtd_part *slave;
 	uint64_t cur_offset = 0;
-	int i;
+	int i, ret;
 
-	printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
+	if (nbparts > 1)
+		printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n",
+		       nbparts, master->name);
 
 	for (i = 0; i < nbparts; i++) {
 		slave = allocate_partition(master, parts + i, i, cur_offset);
@@ -678,7 +680,9 @@  int add_mtd_partitions(struct mtd_info *master,
 		list_add(&slave->list, &mtd_partitions);
 		mutex_unlock(&mtd_partitions_mutex);
 
-		add_mtd_device(&slave->mtd);
+		ret = add_mtd_device(&slave->mtd);
+		if (ret == 1)
+			return -ENODEV;
 
 		cur_offset = slave->offset + slave->mtd.size;
 	}