Re: [PATCH v7 1/2] module: add SCMI device table alias support

[Hans de Goede]

Hi Sudeep,

On 18-Sep-26 15:32, Sudeep Holla wrote:

[...]

>> diff --git a/include/linux/device-id/scmi.h b/include/linux/device-id/scmi.h
>> new file mode 100644
>> index 000000000000..1b4ccfa9dcc5
>> --- /dev/null
>> +++ b/include/linux/device-id/scmi.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef LINUX_DEVICE_ID_SCMI_H
>> +#define LINUX_DEVICE_ID_SCMI_H
>> +
>> +#ifdef __KERNEL__
>> +#include <linux/types.h>
>> +#endif
>> +
>> +#define SCMI_NAME_SIZE		32
>> +#define SCMI_MODULE_PREFIX	"scmi:"
>> +
>> +struct scmi_device_id {
>> +	__u8 protocol_id;
>> +	char name[SCMI_NAME_SIZE];
>> +};
>> +
>> +#endif /* ifndef LINUX_DEVICE_ID_SCMI_H */
> 
> You need to include this above file in include/linux/mod_devicetable.h
> to avoid duplication of its inclusion elsewhere. That's the pattern followed
> I see.

That is what I did for v3, but Uwe, who is leading the splitting
of mod_devicetable.h from one large monster include to smaller
per subsystem/bus includes explicitly requested me to follow this
pattern, see:

https://lore.kernel.org/arm-scmi/aow2GFAfy84k-N2B@monoceros/

[...]

>> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
>> index b4178c42d08f..91ec3704ee2b 100644
>> --- a/scripts/mod/devicetable-offsets.c
>> +++ b/scripts/mod/devicetable-offsets.c
>> @@ -1,5 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #define COMPILE_OFFSETS
>> +#include <linux/device-id/scmi.h>
> 
> Drop this for above mentioned reason.

This was explictly added at request of Uwe because the goal is to
get rid of mod_devicetable.h including all the device-id headers
in a big single catch-all include. This is done because currently
many files include mod_devicetable.h and then if any of the device-id
headers changes everything needs to be rebuild, making incremental
kernel builds much slower.

(and the same for the second include you point out)

Also replying to your other question here:

> Not related to above topic, but just wanted to check with Daniel if this
> solves module auto-loading for you. Was there someone else who was testing
> this series and was having issue with earlier versions or its you only
> Hans de Goede and it was with v1/v2 when Bjorn posted them. I remember
> something vaguely and couldn't trace back all the versions easily.

I tested Bjorn's original patch-series but since that makes the kernel
call modprobe it does not work when the scmi bus is enumerated during
the initramfs phase and the modules are only part of the rootfs.

So I've replaced this with a rewrite of Christian's approach to always
instantiate standard protocol devices. The rewrite was done because
of various lifetime management issues with Cristian's original approach
found by Shashiko.

Note that Bjorn's modprobe approach also only used the protocol id
as modalias, which means that if a vendor protocol id of say 0x80
were used and we have multiple vendor protocol drivers in kernel
which share 0x80 as id, modules from both vendors would get loaded.
This is something which I did not realize at the time, but this is
another good reason to go with the new approach from this version
of the series.

As for testing, I was the main reporter of this issue since I'm
trying to get Qualcomm Snapdragon laptops to work OOTB with standard
unmodified Fedora aarch64 live/installer media and scmi_cpufreq
needs a workaround to be manually loaded there, see:

https://fedoraproject.org/wiki/Snapdragon_WoA_Laptop_Install_(Fedora_Beta)

As you can see only a few workarounds are still necessary and
manual SCMI auto-loading is one of them. So I really hope that we
can get this series merged in time for 7.4 .

Regards,

Hans