Re: [PATCH] mtd: Fix misuses of of_match_ptr()

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	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>,
	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 16:44:25 +0100	[thread overview]
Message-ID: <20220127164425.755fe5cd@xps13> (raw)
In-Reply-To: <SI9D6R.T149KFA13SNK2@crapouillou.net>

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/