All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd: Fix misuses of of_match_ptr()
@ 2022-01-27 11:06 Miquel Raynal
  2022-01-27 11:18 ` Paul Cercueil
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Miquel Raynal @ 2022-01-27 11:06 UTC (permalink / raw)
  To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
	Pratyush Yadav, Michael Walle, linux-mtd
  Cc: Nicolas Ferre, Alexandre Belloni, Ludovic Desroches,
	Paul Cercueil, Harvey Hunt, Matthias Brugger, Uwe Kleine-Konig,
	Miquel Raynal, kernel test robot

of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
otherwise. There are several drivers using this macro which keep their
of_device_id array enclosed within an #ifdef CONFIG_OF check, these are
considered fine. However, When misused, the of_device_id array pointed
by this macro will produce a warning because it is finally unused when
compiled without OF support.

A number of fixes are possible:
- Always depend on CONFIG_OF, but this will not always work and may
  break boards.
- Enclose the compatible array by #ifdef's, this may save a bit of
  memory but will reduce build coverage.
- Tell the compiler the array may be unused, if this can be avoided,
  let's not do this.
- Just drop the macro, setting the of_device_id array for a non OF
  enabled platform is not an issue, it will just be unused.

The latter solution seems the more appropriate, so let's use it.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/devices/mchp23k256.c                | 2 +-
 drivers/mtd/devices/mchp48l640.c                | 2 +-
 drivers/mtd/nand/raw/atmel/nand-controller.c    | 2 +-
 drivers/mtd/nand/raw/atmel/pmecc.c              | 2 +-
 drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
 drivers/mtd/nand/raw/ingenic/jz4780_bch.c       | 2 +-
 drivers/mtd/nand/raw/mtk_ecc.c                  | 2 +-
 drivers/mtd/nand/raw/omap2.c                    | 2 +-
 drivers/mtd/nand/raw/renesas-nand-controller.c  | 2 +-
 drivers/mtd/nand/raw/sh_flctl.c                 | 2 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c
index a8b31bddf14b..bf51eebf052c 100644
--- a/drivers/mtd/devices/mchp23k256.c
+++ b/drivers/mtd/devices/mchp23k256.c
@@ -234,7 +234,7 @@ MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
 static struct spi_driver mchp23k256_driver = {
 	.driver = {
 		.name	= "mchp23k256",
-		.of_match_table = of_match_ptr(mchp23k256_of_table),
+		.of_match_table = mchp23k256_of_table,
 	},
 	.probe		= mchp23k256_probe,
 	.remove		= mchp23k256_remove,
diff --git a/drivers/mtd/devices/mchp48l640.c b/drivers/mtd/devices/mchp48l640.c
index 231a10790196..4ec505b3f4a6 100644
--- a/drivers/mtd/devices/mchp48l640.c
+++ b/drivers/mtd/devices/mchp48l640.c
@@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, mchp48l640_of_table);
 static struct spi_driver mchp48l640_driver = {
 	.driver = {
 		.name	= "mchp48l640",
-		.of_match_table = of_match_ptr(mchp48l640_of_table),
+		.of_match_table = mchp48l640_of_table,
 	},
 	.probe		= mchp48l640_probe,
 	.remove		= mchp48l640_remove,
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
index f3276ee9e4fe..4ecbaccf5526 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -2648,7 +2648,7 @@ static SIMPLE_DEV_PM_OPS(atmel_nand_controller_pm_ops, NULL,
 static struct platform_driver atmel_nand_controller_driver = {
 	.driver = {
 		.name = "atmel-nand-controller",
-		.of_match_table = of_match_ptr(atmel_nand_controller_of_ids),
+		.of_match_table = atmel_nand_controller_of_ids,
 		.pm = &atmel_nand_controller_pm_ops,
 	},
 	.probe = atmel_nand_controller_probe,
diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c b/drivers/mtd/nand/raw/atmel/pmecc.c
index 498e41ccabbd..43ebd78816c0 100644
--- a/drivers/mtd/nand/raw/atmel/pmecc.c
+++ b/drivers/mtd/nand/raw/atmel/pmecc.c
@@ -1003,7 +1003,7 @@ static int atmel_pmecc_probe(struct platform_device *pdev)
 static struct platform_driver atmel_pmecc_driver = {
 	.driver = {
 		.name = "atmel-pmecc",
-		.of_match_table = of_match_ptr(atmel_pmecc_match),
+		.of_match_table = atmel_pmecc_match,
 	},
 	.probe = atmel_pmecc_probe,
 };
diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index b18861bdcdc8..ff26c10f295d 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -567,7 +567,7 @@ static struct platform_driver ingenic_nand_driver = {
 	.remove		= ingenic_nand_remove,
 	.driver	= {
 		.name	= DRV_NAME,
-		.of_match_table = of_match_ptr(ingenic_nand_dt_match),
+		.of_match_table = ingenic_nand_dt_match,
 	},
 };
 module_platform_driver(ingenic_nand_driver);
diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
index d67dbfff76cc..12b5b0484fe9 100644
--- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
+++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
@@ -260,7 +260,7 @@ static struct platform_driver jz4780_bch_driver = {
 	.probe		= jz4780_bch_probe,
 	.driver	= {
 		.name	= "jz4780-bch",
-		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
+		.of_match_table = jz4780_bch_dt_match,
 	},
 };
 module_platform_driver(jz4780_bch_driver);
diff --git a/drivers/mtd/nand/raw/mtk_ecc.c b/drivers/mtd/nand/raw/mtk_ecc.c
index 1b47964cb6da..e7df3dac705e 100644
--- a/drivers/mtd/nand/raw/mtk_ecc.c
+++ b/drivers/mtd/nand/raw/mtk_ecc.c
@@ -579,7 +579,7 @@ static struct platform_driver mtk_ecc_driver = {
 	.probe  = mtk_ecc_probe,
 	.driver = {
 		.name  = "mtk-ecc",
-		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
+		.of_match_table = mtk_ecc_dt_match,
 #ifdef CONFIG_PM_SLEEP
 		.pm = &mtk_ecc_pm_ops,
 #endif
diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
index f0bbbe401e76..58c32a11792e 100644
--- a/drivers/mtd/nand/raw/omap2.c
+++ b/drivers/mtd/nand/raw/omap2.c
@@ -2298,7 +2298,7 @@ static struct platform_driver omap_nand_driver = {
 	.remove		= omap_nand_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
-		.of_match_table = of_match_ptr(omap_nand_ids),
+		.of_match_table = omap_nand_ids,
 	},
 };
 
diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c b/drivers/mtd/nand/raw/renesas-nand-controller.c
index 428e08362956..6db063b230a9 100644
--- a/drivers/mtd/nand/raw/renesas-nand-controller.c
+++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
@@ -1412,7 +1412,7 @@ MODULE_DEVICE_TABLE(of, rnandc_id_table);
 static struct platform_driver rnandc_driver = {
 	.driver = {
 		.name = "renesas-nandc",
-		.of_match_table = of_match_ptr(rnandc_id_table),
+		.of_match_table = rnandc_id_table,
 	},
 	.probe = rnandc_probe,
 	.remove = rnandc_remove,
diff --git a/drivers/mtd/nand/raw/sh_flctl.c b/drivers/mtd/nand/raw/sh_flctl.c
index 13df4bdf792a..b85b9c6fcc42 100644
--- a/drivers/mtd/nand/raw/sh_flctl.c
+++ b/drivers/mtd/nand/raw/sh_flctl.c
@@ -1220,7 +1220,7 @@ static struct platform_driver flctl_driver = {
 	.remove		= flctl_remove,
 	.driver = {
 		.name	= "sh_flctl",
-		.of_match_table = of_match_ptr(of_flctl_match),
+		.of_match_table = of_flctl_match,
 	},
 };
 
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
@ 2022-01-27 11:18 ` Paul Cercueil
  2022-01-27 11:32   ` Alexandre Belloni
  2022-01-28  8:44 ` Alexandre Belloni
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2022-01-27 11:18 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
	Pratyush Yadav, Michael Walle, linux-mtd, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Harvey Hunt,
	Matthias Brugger, Uwe Kleine-Konig, kernel test robot

Hi Miquel,

Le jeu., janv. 27 2022 at 12:06:31 +0100, Miquel Raynal 
<miquel.raynal@bootlin.com> a écrit :
> of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
> otherwise. There are several drivers using this macro which keep their
> of_device_id array enclosed within an #ifdef CONFIG_OF check, these 
> are
> considered fine. However, When misused, the of_device_id array pointed
> by this macro will produce a warning because it is finally unused when
> compiled without OF support.
> 
> A number of fixes are possible:
> - Always depend on CONFIG_OF, but this will not always work and may
>   break boards.
> - Enclose the compatible array by #ifdef's, this may save a bit of
>   memory but will reduce build coverage.
> - Tell the compiler the array may be unused, if this can be avoided,
>   let's not do this.
> - Just drop the macro, setting the of_device_id array for a non OF
>   enabled platform is not an issue, it will just be unused.
> 
> The latter solution seems the more appropriate, so let's use it.

I disagree. The proper solution would be to not have of_match_ptr() 
conditionally defined.

Right now it's defined basically like this:
#ifdef CONFIG_OF
#define of_match_ptr(_ptr) (_ptr)
#else
#define of_match_ptr(_ptr) NULL
#endif

This is bad, because in the !CONFIG_OF case, the pointer is never 
referenced, and the compiler complains about it, as you can notice.

Instead, it should be defined like this:
#define of_match_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_OF), (_ptr))

Then in the !CONFIG_OF case the compiler will see the array as 
effectively unused, and drop it as needed.

We are doing the exact same work with the PM callbacks, with the new 
pm_ptr() macro.

Note that I don't know the behaviour of MODULE_DEVICE_TABLE(of, ...), 
it might be a good idea to make it a NOP if !CONFIG_OF so that the 
array is removed by the compiler as dead code (if it's not the case 
already).

Cheers,
-Paul

> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/devices/mchp23k256.c                | 2 +-
>  drivers/mtd/devices/mchp48l640.c                | 2 +-
>  drivers/mtd/nand/raw/atmel/nand-controller.c    | 2 +-
>  drivers/mtd/nand/raw/atmel/pmecc.c              | 2 +-
>  drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
>  drivers/mtd/nand/raw/ingenic/jz4780_bch.c       | 2 +-
>  drivers/mtd/nand/raw/mtk_ecc.c                  | 2 +-
>  drivers/mtd/nand/raw/omap2.c                    | 2 +-
>  drivers/mtd/nand/raw/renesas-nand-controller.c  | 2 +-
>  drivers/mtd/nand/raw/sh_flctl.c                 | 2 +-
>  10 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mchp23k256.c 
> b/drivers/mtd/devices/mchp23k256.c
> index a8b31bddf14b..bf51eebf052c 100644
> --- a/drivers/mtd/devices/mchp23k256.c
> +++ b/drivers/mtd/devices/mchp23k256.c
> @@ -234,7 +234,7 @@ MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
>  static struct spi_driver mchp23k256_driver = {
>  	.driver = {
>  		.name	= "mchp23k256",
> -		.of_match_table = of_match_ptr(mchp23k256_of_table),
> +		.of_match_table = mchp23k256_of_table,
>  	},
>  	.probe		= mchp23k256_probe,
>  	.remove		= mchp23k256_remove,
> diff --git a/drivers/mtd/devices/mchp48l640.c 
> b/drivers/mtd/devices/mchp48l640.c
> index 231a10790196..4ec505b3f4a6 100644
> --- a/drivers/mtd/devices/mchp48l640.c
> +++ b/drivers/mtd/devices/mchp48l640.c
> @@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, mchp48l640_of_table);
>  static struct spi_driver mchp48l640_driver = {
>  	.driver = {
>  		.name	= "mchp48l640",
> -		.of_match_table = of_match_ptr(mchp48l640_of_table),
> +		.of_match_table = mchp48l640_of_table,
>  	},
>  	.probe		= mchp48l640_probe,
>  	.remove		= mchp48l640_remove,
> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c 
> b/drivers/mtd/nand/raw/atmel/nand-controller.c
> index f3276ee9e4fe..4ecbaccf5526 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -2648,7 +2648,7 @@ static 
> SIMPLE_DEV_PM_OPS(atmel_nand_controller_pm_ops, NULL,
>  static struct platform_driver atmel_nand_controller_driver = {
>  	.driver = {
>  		.name = "atmel-nand-controller",
> -		.of_match_table = of_match_ptr(atmel_nand_controller_of_ids),
> +		.of_match_table = atmel_nand_controller_of_ids,
>  		.pm = &atmel_nand_controller_pm_ops,
>  	},
>  	.probe = atmel_nand_controller_probe,
> diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c 
> b/drivers/mtd/nand/raw/atmel/pmecc.c
> index 498e41ccabbd..43ebd78816c0 100644
> --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> @@ -1003,7 +1003,7 @@ static int atmel_pmecc_probe(struct 
> platform_device *pdev)
>  static struct platform_driver atmel_pmecc_driver = {
>  	.driver = {
>  		.name = "atmel-pmecc",
> -		.of_match_table = of_match_ptr(atmel_pmecc_match),
> +		.of_match_table = atmel_pmecc_match,
>  	},
>  	.probe = atmel_pmecc_probe,
>  };
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c 
> b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> index b18861bdcdc8..ff26c10f295d 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -567,7 +567,7 @@ static struct platform_driver ingenic_nand_driver 
> = {
>  	.remove		= ingenic_nand_remove,
>  	.driver	= {
>  		.name	= DRV_NAME,
> -		.of_match_table = of_match_ptr(ingenic_nand_dt_match),
> +		.of_match_table = ingenic_nand_dt_match,
>  	},
>  };
>  module_platform_driver(ingenic_nand_driver);
> diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c 
> b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> index d67dbfff76cc..12b5b0484fe9 100644
> --- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> +++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> @@ -260,7 +260,7 @@ static struct platform_driver jz4780_bch_driver = 
> {
>  	.probe		= jz4780_bch_probe,
>  	.driver	= {
>  		.name	= "jz4780-bch",
> -		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
> +		.of_match_table = jz4780_bch_dt_match,
>  	},
>  };
>  module_platform_driver(jz4780_bch_driver);
> diff --git a/drivers/mtd/nand/raw/mtk_ecc.c 
> b/drivers/mtd/nand/raw/mtk_ecc.c
> index 1b47964cb6da..e7df3dac705e 100644
> --- a/drivers/mtd/nand/raw/mtk_ecc.c
> +++ b/drivers/mtd/nand/raw/mtk_ecc.c
> @@ -579,7 +579,7 @@ static struct platform_driver mtk_ecc_driver = {
>  	.probe  = mtk_ecc_probe,
>  	.driver = {
>  		.name  = "mtk-ecc",
> -		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
> +		.of_match_table = mtk_ecc_dt_match,
>  #ifdef CONFIG_PM_SLEEP
>  		.pm = &mtk_ecc_pm_ops,
>  #endif
> diff --git a/drivers/mtd/nand/raw/omap2.c 
> b/drivers/mtd/nand/raw/omap2.c
> index f0bbbe401e76..58c32a11792e 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -2298,7 +2298,7 @@ static struct platform_driver omap_nand_driver 
> = {
>  	.remove		= omap_nand_remove,
>  	.driver		= {
>  		.name	= DRIVER_NAME,
> -		.of_match_table = of_match_ptr(omap_nand_ids),
> +		.of_match_table = omap_nand_ids,
>  	},
>  };
> 
> diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c 
> b/drivers/mtd/nand/raw/renesas-nand-controller.c
> index 428e08362956..6db063b230a9 100644
> --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
> +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
> @@ -1412,7 +1412,7 @@ MODULE_DEVICE_TABLE(of, rnandc_id_table);
>  static struct platform_driver rnandc_driver = {
>  	.driver = {
>  		.name = "renesas-nandc",
> -		.of_match_table = of_match_ptr(rnandc_id_table),
> +		.of_match_table = rnandc_id_table,
>  	},
>  	.probe = rnandc_probe,
>  	.remove = rnandc_remove,
> diff --git a/drivers/mtd/nand/raw/sh_flctl.c 
> b/drivers/mtd/nand/raw/sh_flctl.c
> index 13df4bdf792a..b85b9c6fcc42 100644
> --- a/drivers/mtd/nand/raw/sh_flctl.c
> +++ b/drivers/mtd/nand/raw/sh_flctl.c
> @@ -1220,7 +1220,7 @@ static struct platform_driver flctl_driver = {
>  	.remove		= flctl_remove,
>  	.driver = {
>  		.name	= "sh_flctl",
> -		.of_match_table = of_match_ptr(of_flctl_match),
> +		.of_match_table = of_flctl_match,
>  	},
>  };
> 
> --
> 2.27.0
> 



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:18 ` Paul Cercueil
@ 2022-01-27 11:32   ` Alexandre Belloni
  2022-01-27 11:35     ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2022-01-27 11:32 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd,
	Nicolas Ferre, Ludovic Desroches, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot

On 27/01/2022 11:18:27+0000, Paul Cercueil wrote:
> Hi Miquel,
> 
> Le jeu., janv. 27 2022 at 12:06:31 +0100, Miquel Raynal
> <miquel.raynal@bootlin.com> a écrit :
> > of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
> > otherwise. There are several drivers using this macro which keep their
> > of_device_id array enclosed within an #ifdef CONFIG_OF check, these are
> > considered fine. However, When misused, the of_device_id array pointed
> > by this macro will produce a warning because it is finally unused when
> > compiled without OF support.
> > 
> > A number of fixes are possible:
> > - Always depend on CONFIG_OF, but this will not always work and may
> >   break boards.
> > - Enclose the compatible array by #ifdef's, this may save a bit of
> >   memory but will reduce build coverage.
> > - Tell the compiler the array may be unused, if this can be avoided,
> >   let's not do this.
> > - Just drop the macro, setting the of_device_id array for a non OF
> >   enabled platform is not an issue, it will just be unused.
> > 
> > The latter solution seems the more appropriate, so let's use it.
> 
> I disagree. The proper solution would be to not have of_match_ptr()
> conditionally defined.
> 

I disagree...

> Right now it's defined basically like this:
> #ifdef CONFIG_OF
> #define of_match_ptr(_ptr) (_ptr)
> #else
> #define of_match_ptr(_ptr) NULL
> #endif
> 
> This is bad, because in the !CONFIG_OF case, the pointer is never
> referenced, and the compiler complains about it, as you can notice.
> 
> Instead, it should be defined like this:
> #define of_match_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_OF), (_ptr))
> 
> Then in the !CONFIG_OF case the compiler will see the array as effectively
> unused, and drop it as needed.
> 
> We are doing the exact same work with the PM callbacks, with the new
> pm_ptr() macro.
> 
> Note that I don't know the behaviour of MODULE_DEVICE_TABLE(of, ...), it
> might be a good idea to make it a NOP if !CONFIG_OF so that the array is
> removed by the compiler as dead code (if it's not the case already).
> 

... because ACPI platforms can use the OF table to probe drivers even
when they don't have OF support.

> Cheers,
> -Paul
> 
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/devices/mchp23k256.c                | 2 +-
> >  drivers/mtd/devices/mchp48l640.c                | 2 +-
> >  drivers/mtd/nand/raw/atmel/nand-controller.c    | 2 +-
> >  drivers/mtd/nand/raw/atmel/pmecc.c              | 2 +-
> >  drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
> >  drivers/mtd/nand/raw/ingenic/jz4780_bch.c       | 2 +-
> >  drivers/mtd/nand/raw/mtk_ecc.c                  | 2 +-
> >  drivers/mtd/nand/raw/omap2.c                    | 2 +-
> >  drivers/mtd/nand/raw/renesas-nand-controller.c  | 2 +-
> >  drivers/mtd/nand/raw/sh_flctl.c                 | 2 +-
> >  10 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/mchp23k256.c
> > b/drivers/mtd/devices/mchp23k256.c
> > index a8b31bddf14b..bf51eebf052c 100644
> > --- a/drivers/mtd/devices/mchp23k256.c
> > +++ b/drivers/mtd/devices/mchp23k256.c
> > @@ -234,7 +234,7 @@ MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
> >  static struct spi_driver mchp23k256_driver = {
> >  	.driver = {
> >  		.name	= "mchp23k256",
> > -		.of_match_table = of_match_ptr(mchp23k256_of_table),
> > +		.of_match_table = mchp23k256_of_table,
> >  	},
> >  	.probe		= mchp23k256_probe,
> >  	.remove		= mchp23k256_remove,
> > diff --git a/drivers/mtd/devices/mchp48l640.c
> > b/drivers/mtd/devices/mchp48l640.c
> > index 231a10790196..4ec505b3f4a6 100644
> > --- a/drivers/mtd/devices/mchp48l640.c
> > +++ b/drivers/mtd/devices/mchp48l640.c
> > @@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, mchp48l640_of_table);
> >  static struct spi_driver mchp48l640_driver = {
> >  	.driver = {
> >  		.name	= "mchp48l640",
> > -		.of_match_table = of_match_ptr(mchp48l640_of_table),
> > +		.of_match_table = mchp48l640_of_table,
> >  	},
> >  	.probe		= mchp48l640_probe,
> >  	.remove		= mchp48l640_remove,
> > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c
> > b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > index f3276ee9e4fe..4ecbaccf5526 100644
> > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > @@ -2648,7 +2648,7 @@ static
> > SIMPLE_DEV_PM_OPS(atmel_nand_controller_pm_ops, NULL,
> >  static struct platform_driver atmel_nand_controller_driver = {
> >  	.driver = {
> >  		.name = "atmel-nand-controller",
> > -		.of_match_table = of_match_ptr(atmel_nand_controller_of_ids),
> > +		.of_match_table = atmel_nand_controller_of_ids,
> >  		.pm = &atmel_nand_controller_pm_ops,
> >  	},
> >  	.probe = atmel_nand_controller_probe,
> > diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c
> > b/drivers/mtd/nand/raw/atmel/pmecc.c
> > index 498e41ccabbd..43ebd78816c0 100644
> > --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> > +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> > @@ -1003,7 +1003,7 @@ static int atmel_pmecc_probe(struct
> > platform_device *pdev)
> >  static struct platform_driver atmel_pmecc_driver = {
> >  	.driver = {
> >  		.name = "atmel-pmecc",
> > -		.of_match_table = of_match_ptr(atmel_pmecc_match),
> > +		.of_match_table = atmel_pmecc_match,
> >  	},
> >  	.probe = atmel_pmecc_probe,
> >  };
> > diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> > b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> > index b18861bdcdc8..ff26c10f295d 100644
> > --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> > +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> > @@ -567,7 +567,7 @@ static struct platform_driver ingenic_nand_driver =
> > {
> >  	.remove		= ingenic_nand_remove,
> >  	.driver	= {
> >  		.name	= DRV_NAME,
> > -		.of_match_table = of_match_ptr(ingenic_nand_dt_match),
> > +		.of_match_table = ingenic_nand_dt_match,
> >  	},
> >  };
> >  module_platform_driver(ingenic_nand_driver);
> > diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> > b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> > index d67dbfff76cc..12b5b0484fe9 100644
> > --- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> > +++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> > @@ -260,7 +260,7 @@ static struct platform_driver jz4780_bch_driver = {
> >  	.probe		= jz4780_bch_probe,
> >  	.driver	= {
> >  		.name	= "jz4780-bch",
> > -		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
> > +		.of_match_table = jz4780_bch_dt_match,
> >  	},
> >  };
> >  module_platform_driver(jz4780_bch_driver);
> > diff --git a/drivers/mtd/nand/raw/mtk_ecc.c
> > b/drivers/mtd/nand/raw/mtk_ecc.c
> > index 1b47964cb6da..e7df3dac705e 100644
> > --- a/drivers/mtd/nand/raw/mtk_ecc.c
> > +++ b/drivers/mtd/nand/raw/mtk_ecc.c
> > @@ -579,7 +579,7 @@ static struct platform_driver mtk_ecc_driver = {
> >  	.probe  = mtk_ecc_probe,
> >  	.driver = {
> >  		.name  = "mtk-ecc",
> > -		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
> > +		.of_match_table = mtk_ecc_dt_match,
> >  #ifdef CONFIG_PM_SLEEP
> >  		.pm = &mtk_ecc_pm_ops,
> >  #endif
> > diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> > index f0bbbe401e76..58c32a11792e 100644
> > --- a/drivers/mtd/nand/raw/omap2.c
> > +++ b/drivers/mtd/nand/raw/omap2.c
> > @@ -2298,7 +2298,7 @@ static struct platform_driver omap_nand_driver = {
> >  	.remove		= omap_nand_remove,
> >  	.driver		= {
> >  		.name	= DRIVER_NAME,
> > -		.of_match_table = of_match_ptr(omap_nand_ids),
> > +		.of_match_table = omap_nand_ids,
> >  	},
> >  };
> > 
> > diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c
> > b/drivers/mtd/nand/raw/renesas-nand-controller.c
> > index 428e08362956..6db063b230a9 100644
> > --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
> > +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
> > @@ -1412,7 +1412,7 @@ MODULE_DEVICE_TABLE(of, rnandc_id_table);
> >  static struct platform_driver rnandc_driver = {
> >  	.driver = {
> >  		.name = "renesas-nandc",
> > -		.of_match_table = of_match_ptr(rnandc_id_table),
> > +		.of_match_table = rnandc_id_table,
> >  	},
> >  	.probe = rnandc_probe,
> >  	.remove = rnandc_remove,
> > diff --git a/drivers/mtd/nand/raw/sh_flctl.c
> > b/drivers/mtd/nand/raw/sh_flctl.c
> > index 13df4bdf792a..b85b9c6fcc42 100644
> > --- a/drivers/mtd/nand/raw/sh_flctl.c
> > +++ b/drivers/mtd/nand/raw/sh_flctl.c
> > @@ -1220,7 +1220,7 @@ static struct platform_driver flctl_driver = {
> >  	.remove		= flctl_remove,
> >  	.driver = {
> >  		.name	= "sh_flctl",
> > -		.of_match_table = of_match_ptr(of_flctl_match),
> > +		.of_match_table = of_flctl_match,
> >  	},
> >  };
> > 
> > --
> > 2.27.0
> > 
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:32   ` Alexandre Belloni
@ 2022-01-27 11:35     ` Paul Cercueil
  2022-01-27 15:44       ` Miquel Raynal
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2022-01-27 11:35 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd,
	Nicolas Ferre, Ludovic Desroches, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot



Le jeu., janv. 27 2022 at 12:32:05 +0100, Alexandre Belloni 
<alexandre.belloni@bootlin.com> a écrit :
> On 27/01/2022 11:18:27+0000, Paul Cercueil wrote:
>>  Hi Miquel,
>> 
>>  Le jeu., janv. 27 2022 at 12:06:31 +0100, Miquel Raynal
>>  <miquel.raynal@bootlin.com> a écrit :
>>  > of_match_ptr() either expands to NULL if !CONFIG_OF, or is 
>> transparent
>>  > otherwise. There are several drivers using this macro which keep 
>> their
>>  > of_device_id array enclosed within an #ifdef CONFIG_OF check, 
>> these are
>>  > considered fine. However, When misused, the of_device_id array 
>> pointed
>>  > by this macro will produce a warning because it is finally unused 
>> when
>>  > compiled without OF support.
>>  >
>>  > A number of fixes are possible:
>>  > - Always depend on CONFIG_OF, but this will not always work and 
>> may
>>  >   break boards.
>>  > - Enclose the compatible array by #ifdef's, this may save a bit of
>>  >   memory but will reduce build coverage.
>>  > - Tell the compiler the array may be unused, if this can be 
>> avoided,
>>  >   let's not do this.
>>  > - Just drop the macro, setting the of_device_id array for a non OF
>>  >   enabled platform is not an issue, it will just be unused.
>>  >
>>  > The latter solution seems the more appropriate, so let's use it.
>> 
>>  I disagree. The proper solution would be to not have of_match_ptr()
>>  conditionally defined.
>> 
> 
> I disagree...
> 
>>  Right now it's defined basically like this:
>>  #ifdef CONFIG_OF
>>  #define of_match_ptr(_ptr) (_ptr)
>>  #else
>>  #define of_match_ptr(_ptr) NULL
>>  #endif
>> 
>>  This is bad, because in the !CONFIG_OF case, the pointer is never
>>  referenced, and the compiler complains about it, as you can notice.
>> 
>>  Instead, it should be defined like this:
>>  #define of_match_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_OF), (_ptr))
>> 
>>  Then in the !CONFIG_OF case the compiler will see the array as 
>> effectively
>>  unused, and drop it as needed.
>> 
>>  We are doing the exact same work with the PM callbacks, with the new
>>  pm_ptr() macro.
>> 
>>  Note that I don't know the behaviour of MODULE_DEVICE_TABLE(of, 
>> ...), it
>>  might be a good idea to make it a NOP if !CONFIG_OF so that the 
>> array is
>>  removed by the compiler as dead code (if it's not the case already).
>> 
> 
> ... because ACPI platforms can use the OF table to probe drivers even
> when they don't have OF support.

Fair enough. I didn't think about this use-case.

Cheers,
-Paul

>> 
>>  >
>>  > Reported-by: kernel test robot <lkp@intel.com>
>>  > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>  > ---
>>  >  drivers/mtd/devices/mchp23k256.c                | 2 +-
>>  >  drivers/mtd/devices/mchp48l640.c                | 2 +-
>>  >  drivers/mtd/nand/raw/atmel/nand-controller.c    | 2 +-
>>  >  drivers/mtd/nand/raw/atmel/pmecc.c              | 2 +-
>>  >  drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
>>  >  drivers/mtd/nand/raw/ingenic/jz4780_bch.c       | 2 +-
>>  >  drivers/mtd/nand/raw/mtk_ecc.c                  | 2 +-
>>  >  drivers/mtd/nand/raw/omap2.c                    | 2 +-
>>  >  drivers/mtd/nand/raw/renesas-nand-controller.c  | 2 +-
>>  >  drivers/mtd/nand/raw/sh_flctl.c                 | 2 +-
>>  >  10 files changed, 10 insertions(+), 10 deletions(-)
>>  >
>>  > diff --git a/drivers/mtd/devices/mchp23k256.c
>>  > b/drivers/mtd/devices/mchp23k256.c
>>  > index a8b31bddf14b..bf51eebf052c 100644
>>  > --- a/drivers/mtd/devices/mchp23k256.c
>>  > +++ b/drivers/mtd/devices/mchp23k256.c
>>  > @@ -234,7 +234,7 @@ MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
>>  >  static struct spi_driver mchp23k256_driver = {
>>  >  	.driver = {
>>  >  		.name	= "mchp23k256",
>>  > -		.of_match_table = of_match_ptr(mchp23k256_of_table),
>>  > +		.of_match_table = mchp23k256_of_table,
>>  >  	},
>>  >  	.probe		= mchp23k256_probe,
>>  >  	.remove		= mchp23k256_remove,
>>  > diff --git a/drivers/mtd/devices/mchp48l640.c
>>  > b/drivers/mtd/devices/mchp48l640.c
>>  > index 231a10790196..4ec505b3f4a6 100644
>>  > --- a/drivers/mtd/devices/mchp48l640.c
>>  > +++ b/drivers/mtd/devices/mchp48l640.c
>>  > @@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, mchp48l640_of_table);
>>  >  static struct spi_driver mchp48l640_driver = {
>>  >  	.driver = {
>>  >  		.name	= "mchp48l640",
>>  > -		.of_match_table = of_match_ptr(mchp48l640_of_table),
>>  > +		.of_match_table = mchp48l640_of_table,
>>  >  	},
>>  >  	.probe		= mchp48l640_probe,
>>  >  	.remove		= mchp48l640_remove,
>>  > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c
>>  > b/drivers/mtd/nand/raw/atmel/nand-controller.c
>>  > index f3276ee9e4fe..4ecbaccf5526 100644
>>  > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
>>  > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
>>  > @@ -2648,7 +2648,7 @@ static
>>  > SIMPLE_DEV_PM_OPS(atmel_nand_controller_pm_ops, NULL,
>>  >  static struct platform_driver atmel_nand_controller_driver = {
>>  >  	.driver = {
>>  >  		.name = "atmel-nand-controller",
>>  > -		.of_match_table = of_match_ptr(atmel_nand_controller_of_ids),
>>  > +		.of_match_table = atmel_nand_controller_of_ids,
>>  >  		.pm = &atmel_nand_controller_pm_ops,
>>  >  	},
>>  >  	.probe = atmel_nand_controller_probe,
>>  > diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c
>>  > b/drivers/mtd/nand/raw/atmel/pmecc.c
>>  > index 498e41ccabbd..43ebd78816c0 100644
>>  > --- a/drivers/mtd/nand/raw/atmel/pmecc.c
>>  > +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
>>  > @@ -1003,7 +1003,7 @@ static int atmel_pmecc_probe(struct
>>  > platform_device *pdev)
>>  >  static struct platform_driver atmel_pmecc_driver = {
>>  >  	.driver = {
>>  >  		.name = "atmel-pmecc",
>>  > -		.of_match_table = of_match_ptr(atmel_pmecc_match),
>>  > +		.of_match_table = atmel_pmecc_match,
>>  >  	},
>>  >  	.probe = atmel_pmecc_probe,
>>  >  };
>>  > diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
>>  > b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
>>  > index b18861bdcdc8..ff26c10f295d 100644
>>  > --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
>>  > +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
>>  > @@ -567,7 +567,7 @@ static struct platform_driver 
>> ingenic_nand_driver =
>>  > {
>>  >  	.remove		= ingenic_nand_remove,
>>  >  	.driver	= {
>>  >  		.name	= DRV_NAME,
>>  > -		.of_match_table = of_match_ptr(ingenic_nand_dt_match),
>>  > +		.of_match_table = ingenic_nand_dt_match,
>>  >  	},
>>  >  };
>>  >  module_platform_driver(ingenic_nand_driver);
>>  > diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
>>  > b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
>>  > index d67dbfff76cc..12b5b0484fe9 100644
>>  > --- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
>>  > +++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
>>  > @@ -260,7 +260,7 @@ static struct platform_driver 
>> jz4780_bch_driver = {
>>  >  	.probe		= jz4780_bch_probe,
>>  >  	.driver	= {
>>  >  		.name	= "jz4780-bch",
>>  > -		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
>>  > +		.of_match_table = jz4780_bch_dt_match,
>>  >  	},
>>  >  };
>>  >  module_platform_driver(jz4780_bch_driver);
>>  > diff --git a/drivers/mtd/nand/raw/mtk_ecc.c
>>  > b/drivers/mtd/nand/raw/mtk_ecc.c
>>  > index 1b47964cb6da..e7df3dac705e 100644
>>  > --- a/drivers/mtd/nand/raw/mtk_ecc.c
>>  > +++ b/drivers/mtd/nand/raw/mtk_ecc.c
>>  > @@ -579,7 +579,7 @@ static struct platform_driver mtk_ecc_driver 
>> = {
>>  >  	.probe  = mtk_ecc_probe,
>>  >  	.driver = {
>>  >  		.name  = "mtk-ecc",
>>  > -		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
>>  > +		.of_match_table = mtk_ecc_dt_match,
>>  >  #ifdef CONFIG_PM_SLEEP
>>  >  		.pm = &mtk_ecc_pm_ops,
>>  >  #endif
>>  > diff --git a/drivers/mtd/nand/raw/omap2.c 
>> b/drivers/mtd/nand/raw/omap2.c
>>  > index f0bbbe401e76..58c32a11792e 100644
>>  > --- a/drivers/mtd/nand/raw/omap2.c
>>  > +++ b/drivers/mtd/nand/raw/omap2.c
>>  > @@ -2298,7 +2298,7 @@ static struct platform_driver 
>> omap_nand_driver = {
>>  >  	.remove		= omap_nand_remove,
>>  >  	.driver		= {
>>  >  		.name	= DRIVER_NAME,
>>  > -		.of_match_table = of_match_ptr(omap_nand_ids),
>>  > +		.of_match_table = omap_nand_ids,
>>  >  	},
>>  >  };
>>  >
>>  > diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c
>>  > b/drivers/mtd/nand/raw/renesas-nand-controller.c
>>  > index 428e08362956..6db063b230a9 100644
>>  > --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
>>  > +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
>>  > @@ -1412,7 +1412,7 @@ MODULE_DEVICE_TABLE(of, rnandc_id_table);
>>  >  static struct platform_driver rnandc_driver = {
>>  >  	.driver = {
>>  >  		.name = "renesas-nandc",
>>  > -		.of_match_table = of_match_ptr(rnandc_id_table),
>>  > +		.of_match_table = rnandc_id_table,
>>  >  	},
>>  >  	.probe = rnandc_probe,
>>  >  	.remove = rnandc_remove,
>>  > diff --git a/drivers/mtd/nand/raw/sh_flctl.c
>>  > b/drivers/mtd/nand/raw/sh_flctl.c
>>  > index 13df4bdf792a..b85b9c6fcc42 100644
>>  > --- a/drivers/mtd/nand/raw/sh_flctl.c
>>  > +++ b/drivers/mtd/nand/raw/sh_flctl.c
>>  > @@ -1220,7 +1220,7 @@ static struct platform_driver flctl_driver 
>> = {
>>  >  	.remove		= flctl_remove,
>>  >  	.driver = {
>>  >  		.name	= "sh_flctl",
>>  > -		.of_match_table = of_match_ptr(of_flctl_match),
>>  > +		.of_match_table = of_flctl_match,
>>  >  	},
>>  >  };
>>  >
>>  > --
>>  > 2.27.0
>>  >
>> 
>> 
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:35     ` Paul Cercueil
@ 2022-01-27 15:44       ` Miquel Raynal
  2022-01-27 16:30         ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2022-01-27 15:44 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Alexandre Belloni, Richard Weinberger, Vignesh Raghavendra,
	Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd,
	Nicolas Ferre, Ludovic Desroches, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot

Hi Paul,

paul@crapouillou.net wrote on Thu, 27 Jan 2022 11:35:16 +0000:

> Le jeu., janv. 27 2022 at 12:32:05 +0100, Alexandre Belloni <alexandre.belloni@bootlin.com> a écrit :
> > On 27/01/2022 11:18:27+0000, Paul Cercueil wrote:  
> >>  Hi Miquel,  
> >> >>  Le jeu., janv. 27 2022 at 12:06:31 +0100, Miquel Raynal  
> >>  <miquel.raynal@bootlin.com> a écrit :  
> >>  > of_match_ptr() either expands to NULL if !CONFIG_OF, or is >> transparent
> >>  > otherwise. There are several drivers using this macro which keep >> their
> >>  > of_device_id array enclosed within an #ifdef CONFIG_OF check, >> these are
> >>  > considered fine. However, When misused, the of_device_id array >> pointed
> >>  > by this macro will produce a warning because it is finally unused >> when
> >>  > compiled without OF support.
> >>  >
> >>  > A number of fixes are possible:
> >>  > - Always depend on CONFIG_OF, but this will not always work and >> may
> >>  >   break boards.
> >>  > - Enclose the compatible array by #ifdef's, this may save a bit of
> >>  >   memory but will reduce build coverage.
> >>  > - Tell the compiler the array may be unused, if this can be >> avoided,
> >>  >   let's not do this.
> >>  > - Just drop the macro, setting the of_device_id array for a non OF
> >>  >   enabled platform is not an issue, it will just be unused.
> >>  >
> >>  > The latter solution seems the more appropriate, so let's use it.  
> >> >>  I disagree. The proper solution would be to not have of_match_ptr()  
> >>  conditionally defined.  
> >> > > I disagree...  
> >   
> >>  Right now it's defined basically like this:
> >>  #ifdef CONFIG_OF
> >>  #define of_match_ptr(_ptr) (_ptr)
> >>  #else
> >>  #define of_match_ptr(_ptr) NULL
> >>  #endif  
> >> >>  This is bad, because in the !CONFIG_OF case, the pointer is never  
> >>  referenced, and the compiler complains about it, as you can notice.  
> >> >>  Instead, it should be defined like this:  
> >>  #define of_match_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_OF), (_ptr))  
> >> >>  Then in the !CONFIG_OF case the compiler will see the array as >> effectively  
> >>  unused, and drop it as needed.  
> >> >>  We are doing the exact same work with the PM callbacks, with the new  
> >>  pm_ptr() macro.  
> >> >>  Note that I don't know the behaviour of MODULE_DEVICE_TABLE(of, >> ...), it  
> >>  might be a good idea to make it a NOP if !CONFIG_OF so that the >> array is
> >>  removed by the compiler as dead code (if it's not the case already).  
> >> > > ... because ACPI platforms can use the OF table to probe drivers even  
> > when they don't have OF support.  
> 
> Fair enough. I didn't think about this use-case.

So shall I drop it entirely in the end? Or do it like in several other
drivers: enclose the of_device_id array in a #ifdef?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 15:44       ` Miquel Raynal
@ 2022-01-27 16:30         ` Paul Cercueil
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2022-01-27 16:30 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Alexandre Belloni, Richard Weinberger, Vignesh Raghavendra,
	Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd,
	Nicolas Ferre, Ludovic Desroches, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot

Hi Miquel,

Le jeu., janv. 27 2022 at 16:44:25 +0100, Miquel Raynal 
<miquel.raynal@bootlin.com> a écrit :
> Hi Paul,
> 
> paul@crapouillou.net wrote on Thu, 27 Jan 2022 11:35:16 +0000:
> 
>>  Le jeu., janv. 27 2022 at 12:32:05 +0100, Alexandre Belloni 
>> <alexandre.belloni@bootlin.com> a écrit :
>>  > On 27/01/2022 11:18:27+0000, Paul Cercueil wrote:
>>  >>  Hi Miquel,
>>  >> >>  Le jeu., janv. 27 2022 at 12:06:31 +0100, Miquel Raynal
>>  >>  <miquel.raynal@bootlin.com> a écrit :
>>  >>  > of_match_ptr() either expands to NULL if !CONFIG_OF, or is >> 
>> transparent
>>  >>  > otherwise. There are several drivers using this macro which 
>> keep >> their
>>  >>  > of_device_id array enclosed within an #ifdef CONFIG_OF check, 
>> >> these are
>>  >>  > considered fine. However, When misused, the of_device_id 
>> array >> pointed
>>  >>  > by this macro will produce a warning because it is finally 
>> unused >> when
>>  >>  > compiled without OF support.
>>  >>  >
>>  >>  > A number of fixes are possible:
>>  >>  > - Always depend on CONFIG_OF, but this will not always work 
>> and >> may
>>  >>  >   break boards.
>>  >>  > - Enclose the compatible array by #ifdef's, this may save a 
>> bit of
>>  >>  >   memory but will reduce build coverage.
>>  >>  > - Tell the compiler the array may be unused, if this can be 
>> >> avoided,
>>  >>  >   let's not do this.
>>  >>  > - Just drop the macro, setting the of_device_id array for a 
>> non OF
>>  >>  >   enabled platform is not an issue, it will just be unused.
>>  >>  >
>>  >>  > The latter solution seems the more appropriate, so let's use 
>> it.
>>  >> >>  I disagree. The proper solution would be to not have 
>> of_match_ptr()
>>  >>  conditionally defined.
>>  >> > > I disagree...
>>  >
>>  >>  Right now it's defined basically like this:
>>  >>  #ifdef CONFIG_OF
>>  >>  #define of_match_ptr(_ptr) (_ptr)
>>  >>  #else
>>  >>  #define of_match_ptr(_ptr) NULL
>>  >>  #endif
>>  >> >>  This is bad, because in the !CONFIG_OF case, the pointer is 
>> never
>>  >>  referenced, and the compiler complains about it, as you can 
>> notice.
>>  >> >>  Instead, it should be defined like this:
>>  >>  #define of_match_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_OF), (_ptr))
>>  >> >>  Then in the !CONFIG_OF case the compiler will see the array 
>> as >> effectively
>>  >>  unused, and drop it as needed.
>>  >> >>  We are doing the exact same work with the PM callbacks, with 
>> the new
>>  >>  pm_ptr() macro.
>>  >> >>  Note that I don't know the behaviour of 
>> MODULE_DEVICE_TABLE(of, >> ...), it
>>  >>  might be a good idea to make it a NOP if !CONFIG_OF so that the 
>> >> array is
>>  >>  removed by the compiler as dead code (if it's not the case 
>> already).
>>  >> > > ... because ACPI platforms can use the OF table to probe 
>> drivers even
>>  > when they don't have OF support.
>> 
>>  Fair enough. I didn't think about this use-case.
> 
> So shall I drop it entirely in the end? Or do it like in several other
> drivers: enclose the of_device_id array in a #ifdef?

No, your patch is fine. It's not like wrapping them would save much 
space anyway.

Acked-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
  2022-01-27 11:18 ` Paul Cercueil
@ 2022-01-28  8:44 ` Alexandre Belloni
  2022-01-28 18:09 ` Pratyush Yadav
  2022-01-31 16:21 ` Miquel Raynal
  3 siblings, 0 replies; 9+ messages in thread
From: Alexandre Belloni @ 2022-01-28  8:44 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
	Pratyush Yadav, Michael Walle, linux-mtd, Nicolas Ferre,
	Ludovic Desroches, Paul Cercueil, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot

On 27/01/2022 12:06:31+0100, Miquel Raynal wrote:
> of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
> otherwise. There are several drivers using this macro which keep their
> of_device_id array enclosed within an #ifdef CONFIG_OF check, these are
> considered fine. However, When misused, the of_device_id array pointed
> by this macro will produce a warning because it is finally unused when
> compiled without OF support.
> 
> A number of fixes are possible:
> - Always depend on CONFIG_OF, but this will not always work and may
>   break boards.
> - Enclose the compatible array by #ifdef's, this may save a bit of
>   memory but will reduce build coverage.
> - Tell the compiler the array may be unused, if this can be avoided,
>   let's not do this.
> - Just drop the macro, setting the of_device_id array for a non OF
>   enabled platform is not an issue, it will just be unused.
> 
> The latter solution seems the more appropriate, so let's use it.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/mtd/devices/mchp23k256.c                | 2 +-
>  drivers/mtd/devices/mchp48l640.c                | 2 +-
>  drivers/mtd/nand/raw/atmel/nand-controller.c    | 2 +-
>  drivers/mtd/nand/raw/atmel/pmecc.c              | 2 +-
>  drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 2 +-
>  drivers/mtd/nand/raw/ingenic/jz4780_bch.c       | 2 +-
>  drivers/mtd/nand/raw/mtk_ecc.c                  | 2 +-
>  drivers/mtd/nand/raw/omap2.c                    | 2 +-
>  drivers/mtd/nand/raw/renesas-nand-controller.c  | 2 +-
>  drivers/mtd/nand/raw/sh_flctl.c                 | 2 +-
>  10 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c
> index a8b31bddf14b..bf51eebf052c 100644
> --- a/drivers/mtd/devices/mchp23k256.c
> +++ b/drivers/mtd/devices/mchp23k256.c
> @@ -234,7 +234,7 @@ MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
>  static struct spi_driver mchp23k256_driver = {
>  	.driver = {
>  		.name	= "mchp23k256",
> -		.of_match_table = of_match_ptr(mchp23k256_of_table),
> +		.of_match_table = mchp23k256_of_table,
>  	},
>  	.probe		= mchp23k256_probe,
>  	.remove		= mchp23k256_remove,
> diff --git a/drivers/mtd/devices/mchp48l640.c b/drivers/mtd/devices/mchp48l640.c
> index 231a10790196..4ec505b3f4a6 100644
> --- a/drivers/mtd/devices/mchp48l640.c
> +++ b/drivers/mtd/devices/mchp48l640.c
> @@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, mchp48l640_of_table);
>  static struct spi_driver mchp48l640_driver = {
>  	.driver = {
>  		.name	= "mchp48l640",
> -		.of_match_table = of_match_ptr(mchp48l640_of_table),
> +		.of_match_table = mchp48l640_of_table,
>  	},
>  	.probe		= mchp48l640_probe,
>  	.remove		= mchp48l640_remove,
> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
> index f3276ee9e4fe..4ecbaccf5526 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -2648,7 +2648,7 @@ static SIMPLE_DEV_PM_OPS(atmel_nand_controller_pm_ops, NULL,
>  static struct platform_driver atmel_nand_controller_driver = {
>  	.driver = {
>  		.name = "atmel-nand-controller",
> -		.of_match_table = of_match_ptr(atmel_nand_controller_of_ids),
> +		.of_match_table = atmel_nand_controller_of_ids,
>  		.pm = &atmel_nand_controller_pm_ops,
>  	},
>  	.probe = atmel_nand_controller_probe,
> diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c b/drivers/mtd/nand/raw/atmel/pmecc.c
> index 498e41ccabbd..43ebd78816c0 100644
> --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> @@ -1003,7 +1003,7 @@ static int atmel_pmecc_probe(struct platform_device *pdev)
>  static struct platform_driver atmel_pmecc_driver = {
>  	.driver = {
>  		.name = "atmel-pmecc",
> -		.of_match_table = of_match_ptr(atmel_pmecc_match),
> +		.of_match_table = atmel_pmecc_match,
>  	},
>  	.probe = atmel_pmecc_probe,
>  };
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> index b18861bdcdc8..ff26c10f295d 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -567,7 +567,7 @@ static struct platform_driver ingenic_nand_driver = {
>  	.remove		= ingenic_nand_remove,
>  	.driver	= {
>  		.name	= DRV_NAME,
> -		.of_match_table = of_match_ptr(ingenic_nand_dt_match),
> +		.of_match_table = ingenic_nand_dt_match,
>  	},
>  };
>  module_platform_driver(ingenic_nand_driver);
> diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> index d67dbfff76cc..12b5b0484fe9 100644
> --- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> +++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> @@ -260,7 +260,7 @@ static struct platform_driver jz4780_bch_driver = {
>  	.probe		= jz4780_bch_probe,
>  	.driver	= {
>  		.name	= "jz4780-bch",
> -		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
> +		.of_match_table = jz4780_bch_dt_match,
>  	},
>  };
>  module_platform_driver(jz4780_bch_driver);
> diff --git a/drivers/mtd/nand/raw/mtk_ecc.c b/drivers/mtd/nand/raw/mtk_ecc.c
> index 1b47964cb6da..e7df3dac705e 100644
> --- a/drivers/mtd/nand/raw/mtk_ecc.c
> +++ b/drivers/mtd/nand/raw/mtk_ecc.c
> @@ -579,7 +579,7 @@ static struct platform_driver mtk_ecc_driver = {
>  	.probe  = mtk_ecc_probe,
>  	.driver = {
>  		.name  = "mtk-ecc",
> -		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
> +		.of_match_table = mtk_ecc_dt_match,
>  #ifdef CONFIG_PM_SLEEP
>  		.pm = &mtk_ecc_pm_ops,
>  #endif
> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> index f0bbbe401e76..58c32a11792e 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -2298,7 +2298,7 @@ static struct platform_driver omap_nand_driver = {
>  	.remove		= omap_nand_remove,
>  	.driver		= {
>  		.name	= DRIVER_NAME,
> -		.of_match_table = of_match_ptr(omap_nand_ids),
> +		.of_match_table = omap_nand_ids,
>  	},
>  };
>  
> diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c b/drivers/mtd/nand/raw/renesas-nand-controller.c
> index 428e08362956..6db063b230a9 100644
> --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
> +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
> @@ -1412,7 +1412,7 @@ MODULE_DEVICE_TABLE(of, rnandc_id_table);
>  static struct platform_driver rnandc_driver = {
>  	.driver = {
>  		.name = "renesas-nandc",
> -		.of_match_table = of_match_ptr(rnandc_id_table),
> +		.of_match_table = rnandc_id_table,
>  	},
>  	.probe = rnandc_probe,
>  	.remove = rnandc_remove,
> diff --git a/drivers/mtd/nand/raw/sh_flctl.c b/drivers/mtd/nand/raw/sh_flctl.c
> index 13df4bdf792a..b85b9c6fcc42 100644
> --- a/drivers/mtd/nand/raw/sh_flctl.c
> +++ b/drivers/mtd/nand/raw/sh_flctl.c
> @@ -1220,7 +1220,7 @@ static struct platform_driver flctl_driver = {
>  	.remove		= flctl_remove,
>  	.driver = {
>  		.name	= "sh_flctl",
> -		.of_match_table = of_match_ptr(of_flctl_match),
> +		.of_match_table = of_flctl_match,
>  	},
>  };
>  
> -- 
> 2.27.0
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
  2022-01-27 11:18 ` Paul Cercueil
  2022-01-28  8:44 ` Alexandre Belloni
@ 2022-01-28 18:09 ` Pratyush Yadav
  2022-01-31 16:21 ` Miquel Raynal
  3 siblings, 0 replies; 9+ messages in thread
From: Pratyush Yadav @ 2022-01-28 18:09 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
	Michael Walle, linux-mtd, Nicolas Ferre, Alexandre Belloni,
	Ludovic Desroches, Paul Cercueil, Harvey Hunt, Matthias Brugger,
	Uwe Kleine-Konig, kernel test robot

On 27/01/22 12:06PM, Miquel Raynal wrote:
> of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
> otherwise. There are several drivers using this macro which keep their
> of_device_id array enclosed within an #ifdef CONFIG_OF check, these are
> considered fine. However, When misused, the of_device_id array pointed
> by this macro will produce a warning because it is finally unused when
> compiled without OF support.
> 
> A number of fixes are possible:
> - Always depend on CONFIG_OF, but this will not always work and may
>   break boards.
> - Enclose the compatible array by #ifdef's, this may save a bit of
>   memory but will reduce build coverage.
> - Tell the compiler the array may be unused, if this can be avoided,
>   let's not do this.
> - Just drop the macro, setting the of_device_id array for a non OF
>   enabled platform is not an issue, it will just be unused.
> 
> The latter solution seems the more appropriate, so let's use it.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: Fix misuses of of_match_ptr()
  2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
                   ` (2 preceding siblings ...)
  2022-01-28 18:09 ` Pratyush Yadav
@ 2022-01-31 16:21 ` Miquel Raynal
  3 siblings, 0 replies; 9+ messages in thread
From: Miquel Raynal @ 2022-01-31 16:21 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd
  Cc: Nicolas Ferre, Alexandre Belloni, Ludovic Desroches,
	Paul Cercueil, Harvey Hunt, Matthias Brugger, Uwe Kleine-Konig,
	kernel test robot

On Thu, 2022-01-27 at 11:06:31 UTC, Miquel Raynal wrote:
> of_match_ptr() either expands to NULL if !CONFIG_OF, or is transparent
> otherwise. There are several drivers using this macro which keep their
> of_device_id array enclosed within an #ifdef CONFIG_OF check, these are
> considered fine. However, When misused, the of_device_id array pointed
> by this macro will produce a warning because it is finally unused when
> compiled without OF support.
> 
> A number of fixes are possible:
> - Always depend on CONFIG_OF, but this will not always work and may
>   break boards.
> - Enclose the compatible array by #ifdef's, this may save a bit of
>   memory but will reduce build coverage.
> - Tell the compiler the array may be unused, if this can be avoided,
>   let's not do this.
> - Just drop the macro, setting the of_device_id array for a non OF
>   enabled platform is not an issue, it will just be unused.
> 
> The latter solution seems the more appropriate, so let's use it.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-31 16:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
2022-01-27 11:18 ` Paul Cercueil
2022-01-27 11:32   ` Alexandre Belloni
2022-01-27 11:35     ` Paul Cercueil
2022-01-27 15:44       ` Miquel Raynal
2022-01-27 16:30         ` Paul Cercueil
2022-01-28  8:44 ` Alexandre Belloni
2022-01-28 18:09 ` Pratyush Yadav
2022-01-31 16:21 ` Miquel Raynal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.