Linux-mtd Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Cercueil <paul@crapouillou.net>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Pratyush Yadav <p.yadav@ti.com>, Michael Walle <michael@walle.cc>,
	linux-mtd@lists.infradead.org,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Ludovic Desroches <ludovic.desroches@microchip.com>,
	Harvey Hunt <harveyhuntnexus@gmail.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH] mtd: Fix misuses of of_match_ptr()
Date: Thu, 27 Jan 2022 11:18:27 +0000	[thread overview]
Message-ID: <RQ8D6R.EBO0P9DUGF812@crapouillou.net> (raw)
In-Reply-To: <20220127110631.1064705-1-miquel.raynal@bootlin.com>

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/

  reply	other threads:[~2022-01-27 11:21 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-27 11:06 [PATCH] mtd: Fix misuses of of_match_ptr() Miquel Raynal
2022-01-27 11:18 ` Paul Cercueil [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=RQ8D6R.EBO0P9DUGF812@crapouillou.net \
    --to=paul@crapouillou.net \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=harveyhuntnexus@gmail.com \
    --cc=linux-mtd@lists.infradead.org \
    --cc=lkp@intel.com \
    --cc=ludovic.desroches@microchip.com \
    --cc=matthias.bgg@gmail.com \
    --cc=michael@walle.cc \
    --cc=miquel.raynal@bootlin.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=p.yadav@ti.com \
    --cc=richard@nod.at \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox