[PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Ezequiel Garcia]

In a recent commit:

  commit f455578dd961087a5cf94730d9f6489bb1d355f0
  Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
  Date:   Mon Aug 12 14:14:53 2013 -0300

  mtd: nand: pxa3xx: Remove hardcoded mtd name

  There's no advantage in using a hardcoded name for the mtd device.
  Instead use the provided by the platform_device.

The MTD name was changed to use the one provided by the platform_device.
However, this can be problematic as some users want to set partitions
using the kernel parameter 'mtdpart', where the name is needed.

Therefore, to avoid regressions in users relying in 'mtdpart' we revert
the change and use the previous one 'pxa3xx_nand-0'.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
v1->v2:
  * Use exactly the same name as before, and avoid regressions
    instead of using any seemingly better or cleaner one.

 drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index aa75adf..4d53e8e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -46,6 +46,8 @@
  */
 #define INIT_BUFFER_SIZE	256
 
+#define DRIVER_NAME		"pxa3xx_nand-0"
+
 /* registers and bit definitions */
 #define NDCR		(0x00) /* Control register */
 #define NDTR0CS0	(0x04) /* Timing Parameter 0 for CS0 */
@@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		struct mtd_info *mtd = info->host[cs]->mtd;
 
-		mtd->name = pdev->name;
+		mtd->name = DRIVER_NAME;
 		info->cs = cs;
 		ret = pxa3xx_nand_scan(mtd);
 		if (ret) {
@@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 
 static struct platform_driver pxa3xx_nand_driver = {
 	.driver = {
-		.name	= "pxa3xx-nand",
+		.name	= DRIVER_NAME,
 		.of_match_table = pxa3xx_nand_dt_ids,
 	},
 	.probe		= pxa3xx_nand_probe,
-- 
1.8.1.5

Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Brian Norris]

On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> In a recent commit:
>
>   commit f455578dd961087a5cf94730d9f6489bb1d355f0
>   Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>   Date:   Mon Aug 12 14:14:53 2013 -0300
>
>   mtd: nand: pxa3xx: Remove hardcoded mtd name
>
>   There's no advantage in using a hardcoded name for the mtd device.
>   Instead use the provided by the platform_device.
>
> The MTD name was changed to use the one provided by the platform_device.
> However, this can be problematic as some users want to set partitions
> using the kernel parameter 'mtdpart', where the name is needed.
>
> Therefore, to avoid regressions in users relying in 'mtdpart' we revert
> the change and use the previous one 'pxa3xx_nand-0'.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Should this be marked for -stable (or squeezed into 3.12-rc now), so
we don't leave the regression in 3.12?

> ---
> v1->v2:
>   * Use exactly the same name as before, and avoid regressions
>     instead of using any seemingly better or cleaner one.
>
>  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index aa75adf..4d53e8e 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -46,6 +46,8 @@
>   */
>  #define INIT_BUFFER_SIZE       256
>
> +#define DRIVER_NAME            "pxa3xx_nand-0"
> +
>  /* registers and bit definitions */
>  #define NDCR           (0x00) /* Control register */
>  #define NDTR0CS0       (0x04) /* Timing Parameter 0 for CS0 */
> @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
>         for (cs = 0; cs < pdata->num_cs; cs++) {
>                 struct mtd_info *mtd = info->host[cs]->mtd;
>
> -               mtd->name = pdev->name;
> +               mtd->name = DRIVER_NAME;
>                 info->cs = cs;
>                 ret = pxa3xx_nand_scan(mtd);
>                 if (ret) {
> @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
>
>  static struct platform_driver pxa3xx_nand_driver = {
>         .driver = {
> -               .name   = "pxa3xx-nand",
> +               .name   = DRIVER_NAME,
>                 .of_match_table = pxa3xx_nand_dt_ids,
>         },
>         .probe          = pxa3xx_nand_probe,

For mtdparts' sake, do we need both the driver name and the MTD name
to be the same? Or could we just stick the  "pxa3xx_nand-0" directly
in mtd->name only, where it's needed for compatibility, and allow the
driver name to be cleaner? Or is that a silly idea?

Brian

Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Ezequiel Garcia]

On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > In a recent commit:
> >
> >   commit f455578dd961087a5cf94730d9f6489bb1d355f0
> >   Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >   Date:   Mon Aug 12 14:14:53 2013 -0300
> >
> >   mtd: nand: pxa3xx: Remove hardcoded mtd name
> >
> >   There's no advantage in using a hardcoded name for the mtd device.
> >   Instead use the provided by the platform_device.
> >
> > The MTD name was changed to use the one provided by the platform_device.
> > However, this can be problematic as some users want to set partitions
> > using the kernel parameter 'mtdpart', where the name is needed.
> >
> > Therefore, to avoid regressions in users relying in 'mtdpart' we revert
> > the change and use the previous one 'pxa3xx_nand-0'.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> Should this be marked for -stable (or squeezed into 3.12-rc now), so
> we don't leave the regression in 3.12?
> 

Yes, I think that if we're still on time to fix v3.12 it would be great.
(Sorry for taking care of my own crap so late!).

> > ---
> > v1->v2:
> >   * Use exactly the same name as before, and avoid regressions
> >     instead of using any seemingly better or cleaner one.
> >
> >  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index aa75adf..4d53e8e 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -46,6 +46,8 @@
> >   */
> >  #define INIT_BUFFER_SIZE       256
> >
> > +#define DRIVER_NAME            "pxa3xx_nand-0"
> > +
> >  /* registers and bit definitions */
> >  #define NDCR           (0x00) /* Control register */
> >  #define NDTR0CS0       (0x04) /* Timing Parameter 0 for CS0 */
> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> >         for (cs = 0; cs < pdata->num_cs; cs++) {
> >                 struct mtd_info *mtd = info->host[cs]->mtd;
> >
> > -               mtd->name = pdev->name;
> > +               mtd->name = DRIVER_NAME;
> >                 info->cs = cs;
> >                 ret = pxa3xx_nand_scan(mtd);
> >                 if (ret) {
> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
> >
> >  static struct platform_driver pxa3xx_nand_driver = {
> >         .driver = {
> > -               .name   = "pxa3xx-nand",
> > +               .name   = DRIVER_NAME,
> >                 .of_match_table = pxa3xx_nand_dt_ids,
> >         },
> >         .probe          = pxa3xx_nand_probe,
> 
> For mtdparts' sake, do we need both the driver name and the MTD name
> to be the same? Or could we just stick the  "pxa3xx_nand-0" directly
> in mtd->name only, where it's needed for compatibility, and allow the
> driver name to be cleaner? Or is that a silly idea?
> 

Hm... What's the impact of the driver's name?

I know it's just a small bloat, but still I don't like to see two stupid
strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
that's why I chose a common macro :-)

What do you think?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Brian Norris]

On Fri, Oct 18, 2013 at 12:32 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
>> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
>> <ezequiel.garcia@free-electrons.com> wrote:
[...]
>> >  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> > index aa75adf..4d53e8e 100644
>> > --- a/drivers/mtd/nand/pxa3xx_nand.c
>> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> > @@ -46,6 +46,8 @@
>> >   */
>> >  #define INIT_BUFFER_SIZE       256
>> >
>> > +#define DRIVER_NAME            "pxa3xx_nand-0"
>> > +
>> >  /* registers and bit definitions */
>> >  #define NDCR           (0x00) /* Control register */
>> >  #define NDTR0CS0       (0x04) /* Timing Parameter 0 for CS0 */
>> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
>> >         for (cs = 0; cs < pdata->num_cs; cs++) {
>> >                 struct mtd_info *mtd = info->host[cs]->mtd;
>> >
>> > -               mtd->name = pdev->name;
>> > +               mtd->name = DRIVER_NAME;
>> >                 info->cs = cs;
>> >                 ret = pxa3xx_nand_scan(mtd);
>> >                 if (ret) {
>> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
>> >
>> >  static struct platform_driver pxa3xx_nand_driver = {
>> >         .driver = {
>> > -               .name   = "pxa3xx-nand",
>> > +               .name   = DRIVER_NAME,
>> >                 .of_match_table = pxa3xx_nand_dt_ids,
>> >         },
>> >         .probe          = pxa3xx_nand_probe,
>>
>> For mtdparts' sake, do we need both the driver name and the MTD name
>> to be the same? Or could we just stick the  "pxa3xx_nand-0" directly
>> in mtd->name only, where it's needed for compatibility, and allow the
>> driver name to be cleaner? Or is that a silly idea?
>>
>
> Hm... What's the impact of the driver's name?

I meant to ask you the exact same question, because I'm lazy :)

I'm not sure exactly. I thought it affected the device name (which is
important to users; it's exposed in sysfs in potentially important
ways), but that actually comes from the platform-device allocation or
from device tree. The most visible way I know of is that it prefixes
the messages seen via the dev_{printk,err,info,warn,..}() print
functions. This could be annoying, but not horrible.

The driver name does show up in /sys/bus/platform/drivers, but I'm not
sure if that's ever expected to be stable.

> I know it's just a small bloat, but still I don't like to see two stupid
> strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
> that's why I chose a common macro :-)

That's fine.

> What do you think?

I'm fine with just making both use DRIVER_NAME. So I'd take this patch as-is.

One possible consequence: it makes things a little more fragile if
people want to change one or both in the future, not noticing that
while the driver name may (?) be changed harmlessly, it can cause
mtdparts=... breakage.

Brian

Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Ezequiel Garcia]

On Fri, Oct 18, 2013 at 06:37:06PM -0700, Brian Norris wrote:
> On Fri, Oct 18, 2013 at 12:32 PM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
> >> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
> >> <ezequiel.garcia@free-electrons.com> wrote:
> [...]
> >> >  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
> >> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> >> > index aa75adf..4d53e8e 100644
> >> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> >> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> >> > @@ -46,6 +46,8 @@
> >> >   */
> >> >  #define INIT_BUFFER_SIZE       256
> >> >
> >> > +#define DRIVER_NAME            "pxa3xx_nand-0"
> >> > +
> >> >  /* registers and bit definitions */
> >> >  #define NDCR           (0x00) /* Control register */
> >> >  #define NDTR0CS0       (0x04) /* Timing Parameter 0 for CS0 */
> >> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> >> >         for (cs = 0; cs < pdata->num_cs; cs++) {
> >> >                 struct mtd_info *mtd = info->host[cs]->mtd;
> >> >
> >> > -               mtd->name = pdev->name;
> >> > +               mtd->name = DRIVER_NAME;
> >> >                 info->cs = cs;
> >> >                 ret = pxa3xx_nand_scan(mtd);
> >> >                 if (ret) {
> >> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
> >> >
> >> >  static struct platform_driver pxa3xx_nand_driver = {
> >> >         .driver = {
> >> > -               .name   = "pxa3xx-nand",
> >> > +               .name   = DRIVER_NAME,
> >> >                 .of_match_table = pxa3xx_nand_dt_ids,
> >> >         },
> >> >         .probe          = pxa3xx_nand_probe,
> >>
> >> For mtdparts' sake, do we need both the driver name and the MTD name
> >> to be the same? Or could we just stick the  "pxa3xx_nand-0" directly
> >> in mtd->name only, where it's needed for compatibility, and allow the
> >> driver name to be cleaner? Or is that a silly idea?
> >>
> >
> > Hm... What's the impact of the driver's name?
> 
> I meant to ask you the exact same question, because I'm lazy :)
> 

I guess that makes the two of us lazy :)

> I'm not sure exactly. I thought it affected the device name (which is
> important to users; it's exposed in sysfs in potentially important
> ways), but that actually comes from the platform-device allocation or
> from device tree. The most visible way I know of is that it prefixes
> the messages seen via the dev_{printk,err,info,warn,..}() print
> functions. This could be annoying, but not horrible.
> 

Good point. I'm not sure my OCD will resist seeing "pxa3xx_nand-0: blabla"
printed.

> The driver name does show up in /sys/bus/platform/drivers, but I'm not
> sure if that's ever expected to be stable.
> 
> > I know it's just a small bloat, but still I don't like to see two stupid
> > strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
> > that's why I chose a common macro :-)
> 
> That's fine.
> 
> > What do you think?
> 
> I'm fine with just making both use DRIVER_NAME. So I'd take this patch as-is.
> 
> One possible consequence: it makes things a little more fragile if
> people want to change one or both in the future, not noticing that
> while the driver name may (?) be changed harmlessly, it can cause
> mtdparts=... breakage.
> 

Hm.. another good point. Let me send a v3.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com