[PATCH] mtd: Fix misuses of of_match_ptr()

[Paul Cercueil]

Hi Miquel,

Le jeu., janv. 27 2022 at 16:44:25 +0100, Miquel Raynal 
<miquel.raynal at bootlin.com> a écrit :
> Hi Paul,
> 
> paul at 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 at 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 at 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 at crapouillou.net>

Cheers,
-Paul