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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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/