linux-kernel - Re: [PATCH 1/3] platform: olpc: mark SPI related data as maybe unused

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Mar 2023 15:07:10 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Thadeu Lima de Souza Cascardo <cascardo@...oscopio.com>,
        Daniel Oliveira Nascimento <don@...t.com.br>,
        Mattia Dongili <malattia@...ux.it>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Javier Martinez Canillas <javierm@...hat.com>
Subject: Re: [PATCH 1/3] platform: olpc: mark SPI related data as maybe unused

On 16/03/2023 13:50, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 3/12/23 14:26, Krzysztof Kozlowski wrote:
>> The driver can be compile tested as built-in making certain data unused:
>>
>>   drivers/platform/olpc/olpc-xo175-ec.c:737:35: error: ‘olpc_xo175_ec_id_table’ defined but not used [-Werror=unused-const-variable=]
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> ---
>>  drivers/platform/olpc/olpc-xo175-ec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
>> index 4823bd2819f6..04573495ef5a 100644
>> --- a/drivers/platform/olpc/olpc-xo175-ec.c
>> +++ b/drivers/platform/olpc/olpc-xo175-ec.c
>> @@ -734,7 +734,7 @@ static const struct of_device_id olpc_xo175_ec_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
>>  
>> -static const struct spi_device_id olpc_xo175_ec_id_table[] = {
>> +static const struct spi_device_id olpc_xo175_ec_id_table[] __maybe_unused = {
>>  	{ "xo1.75-ec", 0 },
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(spi, olpc_xo175_ec_id_table);
> 
> The whole presence of the olpc_xo175_ec_id_table[] seems to make little sense.
> 
> This should be referenced by:
> 
> static struct spi_driver olpc_xo175_ec_spi_driver = {
> 
> Like this:
> 
>         .probe          = olpc_xo175_ec_probe,
>         .remove         = olpc_xo175_ec_remove,
> +       .id_table       = olpc_xo175_ec_id_table,

Yes, it should.

> 
> Otherwise those ids cannot be used to load the driver the non DT/of way. Since the driver assumingly does actually bind already this means that it is only used the DT/of way and it seems to me that the whole olpc_xo175_ec_id_table[] can be removed entirely.
> 
> Exposing modaliases for a non supported way of binding the driver does not really seem useful ?

However binding the device and module loading (uevent) uses sometimes
different pieces. Maybe something changed in kernel, but sometime ago
certain buses where sending uevent for module loading with one ID (e.g.
platform or spi bus) but device matching would be according to OF. Thus
if you did not have entries in spi_device_id, the module would not autoload.

It was exactly the case for example here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c46ed2281bbe4b84e6f3d4bdfb0e4e9ab813fa9d&context=30&ignorews=0&dt=0

You needed spi_device_id for proper module autoloading.

Unless something change in between in the kernel?

(+CC Javier, author of that commit)

Best regards,
Krzysztof