[PATCH] tpm_tis_spi: add missing SPI device ID entries

[PATCH] tpm_tis_spi: add missing SPI device ID entries

[Javier Martinez Canillas]

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

Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

[Jason Gunthorpe]

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

Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer
New Platform Technologies Enablement team
RHEL Engineering

Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

[Jason Gunthorpe]

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

Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

[Peter Robinson]

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
>

Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

[Jarkko Sakkinen]

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