tpm_tis_spi: add missing SPI device ID entries
diff mbox series

Message ID 20210527152352.3835076-1-javierm@redhat.com
State Accepted
Commit c46ed2281bbe4b84e6f3d4bdfb0e4e9ab813fa9d
Headers show
Series
  • tpm_tis_spi: add missing SPI device ID entries
Related show

Commit Message

Javier Martinez Canillas May 27, 2021, 3:23 p.m. UTC
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(+)

Comments

Jason Gunthorpe May 27, 2021, 4:11 p.m. UTC | #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.

Really? Then why do we have of_tis_spi_match and why does spi have an
of_match_table?

Jason
Javier Martinez Canillas May 27, 2021, 4:42 p.m. UTC | #2
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,
Jason Gunthorpe May 28, 2021, 4:54 p.m. UTC | #3
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
Peter Robinson May 29, 2021, 1:50 p.m. UTC | #4
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
>
Jarkko Sakkinen May 31, 2021, 5:37 a.m. UTC | #5
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

Patch
diff mbox series

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 },
 	{}