| Message ID | 20210527152352.3835076-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_tis_spi: add missing SPI device ID entries | expand |
On Thu, May 27, 2021 at 05:23:52PM +0200, Javier Martinez Canillas wrote: > The SPI core always reports a "MODALIAS=spi:<foo>", even if the device was > registered via OF. This means that this module won't auto-load if a DT has > for example has a node with a compatible "infineon,slb9670" string. Really? Then why do we have of_tis_spi_match and why does spi have an of_match_table? Jason
Hello Jason, On 5/27/21 6:11 PM, Jason Gunthorpe wrote: > On Thu, May 27, 2021 at 05:23:52PM +0200, Javier Martinez Canillas wrote: >> The SPI core always reports a "MODALIAS=spi:<foo>", even if the device was >> registered via OF. This means that this module won't auto-load if a DT has >> for example has a node with a compatible "infineon,slb9670" string. > > Really? Then why do we have of_tis_spi_match and why does spi have an > of_match_table? > It's correctly used to match drivers with devices registered by OF, so it makes sense to have them. It's only the MODALIAS uevent reporting in the SPI core that doesn't do the correct thing: static int spi_uevent(struct device *dev, struct kobj_uevent_env *env) { const struct spi_device *spi = to_spi_device(dev); int rc; rc = acpi_device_uevent_modalias(dev, env); if (rc != -ENODEV) return rc; return add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias); } where spi->modalias contain the device portion of the compatible string in the DT node. Now compare with what the I2C subsystem does for example: static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); int rc; rc = of_device_uevent_modalias(dev, env); if (rc != -ENODEV) return rc; rc = acpi_device_uevent_modalias(dev, env); if (rc != -ENODEV) return rc; return add_uevent_var(env, "MODALIAS=%s%s", I2C_MODULE_PREFIX, client->name); } Fixing the spi_uevent() function would be pretty trivial but that would break all the drivers and platforms that are relying on the current behaviour. So until we have fixed all the SPI drivers and make sure that have a proper OF device ID table to match against the reported OF modalises, we will need to add these workarounds to the SPI drivers. It's true that we could get rid of the OF device ID tables in the SPI drivers, but that would mean that: a) The manufacturer portion of the compatible string would never be used to match a device to a driver, so "foo,bar" and "baz,bar" could match to the wrong driver. b) We will be even more far from eventually fix the SPI core modalias reporting since SPI drivers won't have OF aliases in their modules. > Jason > Best regards,
On Thu, May 27, 2021 at 06:42:25PM +0200, Javier Martinez Canillas wrote: > Fixing the spi_uevent() function would be pretty trivial but that > would break all the drivers and platforms that are relying on the > current behaviour. Oh this makes me sad to read after all these years :( It shure would be nice if multiple modaliases could be reported for situations like this. Jason
On Thu, May 27, 2021 at 4:24 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > > The SPI core always reports a "MODALIAS=spi:<foo>", even if the device was > registered via OF. This means that this module won't auto-load if a DT has > for example has a node with a compatible "infineon,slb9670" string. > > In that case kmod will expect a "MODALIAS=of:N*T*Cinfineon,slb9670" uevent > but instead will get a "MODALIAS=spi:slb9670", which is not present in the > kernel module aliases: > > $ modinfo drivers/char/tpm/tpm_tis_spi.ko | grep alias > alias: of:N*T*Cgoogle,cr50C* > alias: of:N*T*Cgoogle,cr50 > alias: of:N*T*Ctcg,tpm_tis-spiC* > alias: of:N*T*Ctcg,tpm_tis-spi > alias: of:N*T*Cinfineon,slb9670C* > alias: of:N*T*Cinfineon,slb9670 > alias: of:N*T*Cst,st33htpm-spiC* > alias: of:N*T*Cst,st33htpm-spi > alias: spi:cr50 > alias: spi:tpm_tis_spi > alias: acpi*:SMO0768:* > > To workaround this issue, add in the SPI device ID table all the entries > that are present in the OF device ID table. > > Reported-by: Alexander Wellbrock <a.wellbrock@mailbox.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Tested-by: Peter Robinson <pbrobinson@gmail.com> Tested on a Raspberry Pi4, with an Iridium 9670 module on 5.12.7 with tpm_tis_spi built as a module. > --- > > drivers/char/tpm/tpm_tis_spi_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index 3856f6ebcb3..de4209003a4 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -260,6 +260,8 @@ static int tpm_tis_spi_remove(struct spi_device *dev) > } > > static const struct spi_device_id tpm_tis_spi_id[] = { > + { "st33htpm-spi", (unsigned long)tpm_tis_spi_probe }, > + { "slb9670", (unsigned long)tpm_tis_spi_probe }, > { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe }, > { "cr50", (unsigned long)cr50_spi_probe }, > {} > -- > 2.31.1 >
On Thu, May 27, 2021 at 05:23:52PM +0200, Javier Martinez Canillas wrote: > The SPI core always reports a "MODALIAS=spi:<foo>", even if the device was > registered via OF. This means that this module won't auto-load if a DT has > for example has a node with a compatible "infineon,slb9670" string. > > In that case kmod will expect a "MODALIAS=of:N*T*Cinfineon,slb9670" uevent > but instead will get a "MODALIAS=spi:slb9670", which is not present in the > kernel module aliases: > > $ modinfo drivers/char/tpm/tpm_tis_spi.ko | grep alias > alias: of:N*T*Cgoogle,cr50C* > alias: of:N*T*Cgoogle,cr50 > alias: of:N*T*Ctcg,tpm_tis-spiC* > alias: of:N*T*Ctcg,tpm_tis-spi > alias: of:N*T*Cinfineon,slb9670C* > alias: of:N*T*Cinfineon,slb9670 > alias: of:N*T*Cst,st33htpm-spiC* > alias: of:N*T*Cst,st33htpm-spi > alias: spi:cr50 > alias: spi:tpm_tis_spi > alias: acpi*:SMO0768:* > > To workaround this issue, add in the SPI device ID table all the entries > that are present in the OF device ID table. > > Reported-by: Alexander Wellbrock <a.wellbrock@mailbox.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > drivers/char/tpm/tpm_tis_spi_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index 3856f6ebcb3..de4209003a4 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -260,6 +260,8 @@ static int tpm_tis_spi_remove(struct spi_device *dev) > } > > static const struct spi_device_id tpm_tis_spi_id[] = { > + { "st33htpm-spi", (unsigned long)tpm_tis_spi_probe }, > + { "slb9670", (unsigned long)tpm_tis_spi_probe }, > { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe }, > { "cr50", (unsigned long)cr50_spi_probe }, > {} > -- > 2.31.1 > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c index 3856f6ebcb3..de4209003a4 100644 --- a/drivers/char/tpm/tpm_tis_spi_main.c +++ b/drivers/char/tpm/tpm_tis_spi_main.c @@ -260,6 +260,8 @@ static int tpm_tis_spi_remove(struct spi_device *dev) } static const struct spi_device_id tpm_tis_spi_id[] = { + { "st33htpm-spi", (unsigned long)tpm_tis_spi_probe }, + { "slb9670", (unsigned long)tpm_tis_spi_probe }, { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe }, { "cr50", (unsigned long)cr50_spi_probe }, {}
The SPI core always reports a "MODALIAS=spi:<foo>", even if the device was registered via OF. This means that this module won't auto-load if a DT has for example has a node with a compatible "infineon,slb9670" string. In that case kmod will expect a "MODALIAS=of:N*T*Cinfineon,slb9670" uevent but instead will get a "MODALIAS=spi:slb9670", which is not present in the kernel module aliases: $ modinfo drivers/char/tpm/tpm_tis_spi.ko | grep alias alias: of:N*T*Cgoogle,cr50C* alias: of:N*T*Cgoogle,cr50 alias: of:N*T*Ctcg,tpm_tis-spiC* alias: of:N*T*Ctcg,tpm_tis-spi alias: of:N*T*Cinfineon,slb9670C* alias: of:N*T*Cinfineon,slb9670 alias: of:N*T*Cst,st33htpm-spiC* alias: of:N*T*Cst,st33htpm-spi alias: spi:cr50 alias: spi:tpm_tis_spi alias: acpi*:SMO0768:* To workaround this issue, add in the SPI device ID table all the entries that are present in the OF device ID table. Reported-by: Alexander Wellbrock <a.wellbrock@mailbox.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/char/tpm/tpm_tis_spi_main.c | 2 ++ 1 file changed, 2 insertions(+)