[PATCH RFC API ONLY] mtd: get parsers from lookup table

[PATCH RFC API ONLY] mtd: get parsers from lookup table

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Existing implementation of specifying flash device parsers became a bit
hacky and needs a cleanup. Currently it requires:
1) Passing parsers in a custom platform data by arch code
or
2) Hardcoding parsers table in a flash driver

The purpose of the new implementation is to:
1) Clean up flash drivers
2) Have a generic solution
3) Avoid code duplication

That new implementation assigns table of parsers to a MTD's parent
device. That way flash driver doesn't have to take care of passing
parsers. It's inspired by GPIO lookup table.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
It's obviously a RFC patch only. It doesn't really implement anything.
It DOESN'T COMPILE.

I'd like to know if you like this idea and if it's worth for me to
actually spend time implementing it.
---
 arch/arm/mach-pxa/corgi.c | 21 +++++++++++++--------
 drivers/mtd/mtdpart.c     |  4 ++++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
index 9a5a35e90769..0ff77e078d2b 100644
--- a/arch/arm/mach-pxa/corgi.c
+++ b/arch/arm/mach-pxa/corgi.c
@@ -615,16 +615,8 @@ static struct nand_bbt_descr sharpsl_bbt = {
 	.pattern = scan_ff_pattern
 };
 
-static const char * const probes[] = {
-	"cmdlinepart",
-	"ofpart",
-	"sharpslpart",
-	NULL,
-};
-
 static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
 	.badblock_pattern	= &sharpsl_bbt,
-	.part_parsers		= probes,
 };
 
 static struct resource sharpsl_nand_resources[] = {
@@ -688,6 +680,16 @@ static struct i2c_board_info __initdata corgi_i2c_devices[] = {
 	{ I2C_BOARD_INFO("wm8731", 0x1b) },
 };
 
+static struct mtd_parser_lookup_table corgi_mtd_parser_lookup_table = {
+	.dev_name = NULL, /* Set with registered name */
+	.table = {
+		{ .parser = "cmdlinepart" },
+		{ .parser = "ofpart" },
+		{ .parser = "sharpslpart" },
+		{ },
+	},
+};
+
 static void corgi_poweroff(void)
 {
 	if (!machine_is_corgi())
@@ -740,6 +742,9 @@ static void __init corgi_init(void)
 
 	platform_add_devices(devices, ARRAY_SIZE(devices));
 
+	corgi_mtd_parser_lookup_table.dev_name = dev_name(&sharpsl_nand_device.dev);
+	mtd_parsers_add_lookup_table(corgi_mtd_parser_lookup_table);
+
 	regulator_has_full_constraints();
 }
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 52e2cb35fc79..0d96857a57d0 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -936,6 +936,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	struct mtd_part_parser *parser;
 	int ret, err = 0;
 
+	if (!types) {
+		types = mtd_parsers_get(master->dev.parent);
+	}
+
 	if (!types)
 		types = mtd_is_partition(master) ? default_subpartition_types :
 			default_mtd_part_types;
-- 
2.13.7

Re: [PATCH RFC API ONLY] mtd: get parsers from lookup table

[Boris Brezillon]

Hi Rafal,

On Mon, 16 Jul 2018 13:17:12 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Existing implementation of specifying flash device parsers became a bit
> hacky and needs a cleanup. Currently it requires:
> 1) Passing parsers in a custom platform data by arch code
> or
> 2) Hardcoding parsers table in a flash driver
> 
> The purpose of the new implementation is to:
> 1) Clean up flash drivers
> 2) Have a generic solution
> 3) Avoid code duplication
> 
> That new implementation assigns table of parsers to a MTD's parent
> device. That way flash driver doesn't have to take care of passing
> parsers. It's inspired by GPIO lookup table.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> It's obviously a RFC patch only. It doesn't really implement anything.
> It DOESN'T COMPILE.
> 
> I'd like to know if you like this idea and if it's worth for me to
> actually spend time implementing it.

I love the idea. Actually, I'd like to extend that to fixed partition
definitions (those passed to mtd_device_register()) so that they can be
parsed by the fixed-partition parser (the one already taking care of
compatible = "fixed-partitions").

One comment below.

> ---
>  arch/arm/mach-pxa/corgi.c | 21 +++++++++++++--------
>  drivers/mtd/mtdpart.c     |  4 ++++
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
> index 9a5a35e90769..0ff77e078d2b 100644
> --- a/arch/arm/mach-pxa/corgi.c
> +++ b/arch/arm/mach-pxa/corgi.c
> @@ -615,16 +615,8 @@ static struct nand_bbt_descr sharpsl_bbt = {
>  	.pattern = scan_ff_pattern
>  };
>  
> -static const char * const probes[] = {
> -	"cmdlinepart",
> -	"ofpart",
> -	"sharpslpart",
> -	NULL,
> -};
> -
>  static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
>  	.badblock_pattern	= &sharpsl_bbt,
> -	.part_parsers		= probes,
>  };
>  
>  static struct resource sharpsl_nand_resources[] = {
> @@ -688,6 +680,16 @@ static struct i2c_board_info __initdata corgi_i2c_devices[] = {
>  	{ I2C_BOARD_INFO("wm8731", 0x1b) },
>  };
>  
> +static struct mtd_parser_lookup_table corgi_mtd_parser_lookup_table = {
> +	.dev_name = NULL, /* Set with registered name */

Can't you guess the device name? It's really weird that you have to
register the lookup table after the device has been registered. I
already had a similar discussion on GPIO lookup tables registration on
ams-delta here [1], and I think my comments apply to this case as well.

> +	.table = {
> +		{ .parser = "cmdlinepart" },
> +		{ .parser = "ofpart" },
> +		{ .parser = "sharpslpart" },
> +		{ },
> +	},
> +};
> +
>  static void corgi_poweroff(void)
>  {
>  	if (!machine_is_corgi())
> @@ -740,6 +742,9 @@ static void __init corgi_init(void)
>  
>  	platform_add_devices(devices, ARRAY_SIZE(devices));
>  
> +	corgi_mtd_parser_lookup_table.dev_name = dev_name(&sharpsl_nand_device.dev);
> +	mtd_parsers_add_lookup_table(corgi_mtd_parser_lookup_table);
> +
>  	regulator_has_full_constraints();
>  }
>  
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 52e2cb35fc79..0d96857a57d0 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -936,6 +936,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	struct mtd_part_parser *parser;
>  	int ret, err = 0;
>  
> +	if (!types) {
> +		types = mtd_parsers_get(master->dev.parent);
> +	}
> +
>  	if (!types)
>  		types = mtd_is_partition(master) ? default_subpartition_types :
>  			default_mtd_part_types;

Regards,

Boris

[1]https://patchwork.ozlabs.org/patch/920798/

Re: [PATCH RFC API ONLY] mtd: get parsers from lookup table

[Rafał Miłecki]

On 2018-07-16 18:23, Boris Brezillon wrote:
> Hi Rafal,
> 
> On Mon, 16 Jul 2018 13:17:12 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
> 
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Existing implementation of specifying flash device parsers became a 
>> bit
>> hacky and needs a cleanup. Currently it requires:
>> 1) Passing parsers in a custom platform data by arch code
>> or
>> 2) Hardcoding parsers table in a flash driver
>> 
>> The purpose of the new implementation is to:
>> 1) Clean up flash drivers
>> 2) Have a generic solution
>> 3) Avoid code duplication
>> 
>> That new implementation assigns table of parsers to a MTD's parent
>> device. That way flash driver doesn't have to take care of passing
>> parsers. It's inspired by GPIO lookup table.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> It's obviously a RFC patch only. It doesn't really implement anything.
>> It DOESN'T COMPILE.
>> 
>> I'd like to know if you like this idea and if it's worth for me to
>> actually spend time implementing it.
> 
> I love the idea. Actually, I'd like to extend that to fixed partition
> definitions (those passed to mtd_device_register()) so that they can be
> parsed by the fixed-partition parser (the one already taking care of
> compatible = "fixed-partitions").

That was something I planned to handle next, so it's great to hear we
share the ideas for further improvements! That will extremely simplify
mtd_device_register().


> One comment below.
> 
>> ---
>>  arch/arm/mach-pxa/corgi.c | 21 +++++++++++++--------
>>  drivers/mtd/mtdpart.c     |  4 ++++
>>  2 files changed, 17 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
>> index 9a5a35e90769..0ff77e078d2b 100644
>> --- a/arch/arm/mach-pxa/corgi.c
>> +++ b/arch/arm/mach-pxa/corgi.c
>> @@ -615,16 +615,8 @@ static struct nand_bbt_descr sharpsl_bbt = {
>>  	.pattern = scan_ff_pattern
>>  };
>> 
>> -static const char * const probes[] = {
>> -	"cmdlinepart",
>> -	"ofpart",
>> -	"sharpslpart",
>> -	NULL,
>> -};
>> -
>>  static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = 
>> {
>>  	.badblock_pattern	= &sharpsl_bbt,
>> -	.part_parsers		= probes,
>>  };
>> 
>>  static struct resource sharpsl_nand_resources[] = {
>> @@ -688,6 +680,16 @@ static struct i2c_board_info __initdata 
>> corgi_i2c_devices[] = {
>>  	{ I2C_BOARD_INFO("wm8731", 0x1b) },
>>  };
>> 
>> +static struct mtd_parser_lookup_table corgi_mtd_parser_lookup_table = 
>> {
>> +	.dev_name = NULL, /* Set with registered name */
> 
> Can't you guess the device name? It's really weird that you have to
> register the lookup table after the device has been registered. I
> already had a similar discussion on GPIO lookup tables registration on
> ams-delta here [1], and I think my comments apply to this case as well.
> 
> (...)
> 
> [1]https://patchwork.ozlabs.org/patch/920798/

Thanks for link to that discussion!

I saw a possible race problem regarding registering lookup table and
probing a platform (?) device/driver. I thought we can just depend on
arch code being executed very early.

Your solution with guessing device name should work in 99% of cases. If
you ls /sys/devices/platform/, you'll see that names are assigned using
name.<index> pattern.

In 99% cases there is only one place in the code registering a given
platform device. As there is only one place registering "sharpsl-nand"
device, we can assume the resulting name will be "sharpsl-nand.0".

If there was another code doing the same, you couldn't know if you
should expect "sharpsl-nand.0" or "sharpsl-nand.1".

I believe we should be fine guessing in these 99% cases and only depend
on order (semi-risking a race) in that 1% (or less).

Re: [PATCH RFC API ONLY] mtd: get parsers from lookup table

[Boris Brezillon]

On Mon, 16 Jul 2018 22:58:19 +0200
Rafał Miłecki <rafal@milecki.pl> wrote:

> On 2018-07-16 18:23, Boris Brezillon wrote:
> > Hi Rafal,
> > 
> > On Mon, 16 Jul 2018 13:17:12 +0200
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >   
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >> 
> >> Existing implementation of specifying flash device parsers became a 
> >> bit
> >> hacky and needs a cleanup. Currently it requires:
> >> 1) Passing parsers in a custom platform data by arch code
> >> or
> >> 2) Hardcoding parsers table in a flash driver
> >> 
> >> The purpose of the new implementation is to:
> >> 1) Clean up flash drivers
> >> 2) Have a generic solution
> >> 3) Avoid code duplication
> >> 
> >> That new implementation assigns table of parsers to a MTD's parent
> >> device. That way flash driver doesn't have to take care of passing
> >> parsers. It's inspired by GPIO lookup table.
> >> 
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >> It's obviously a RFC patch only. It doesn't really implement anything.
> >> It DOESN'T COMPILE.
> >> 
> >> I'd like to know if you like this idea and if it's worth for me to
> >> actually spend time implementing it.  
> > 
> > I love the idea. Actually, I'd like to extend that to fixed partition
> > definitions (those passed to mtd_device_register()) so that they can be
> > parsed by the fixed-partition parser (the one already taking care of
> > compatible = "fixed-partitions").  
> 
> That was something I planned to handle next, so it's great to hear we
> share the ideas for further improvements! That will extremely simplify
> mtd_device_register().

Cool!

> 
> In 99% cases there is only one place in the code registering a given
> platform device. As there is only one place registering "sharpsl-nand"
> device, we can assume the resulting name will be "sharpsl-nand.0".
> 
> If there was another code doing the same, you couldn't know if you
> should expect "sharpsl-nand.0" or "sharpsl-nand.1".

It's only true if you use platform_device->id = PLATFORM_DEVID_AUTO,
and I think we can assume linking a lookup table to a device with ->id
= PLATFORM_DEVID_AUTO is broken. In other cases, the id defined at
platform_device definition time (either PLATFORM_DEVID_NONE or a
statically assigned id).

> 
> I believe we should be fine guessing in these 99% cases and only depend
> on order (semi-risking a race) in that 1% (or less).