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

[Mike Dunn <mikedunn@newsguy.com>]

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