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

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, 27 May 2021 18:42:25 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     linux-kernel@...r.kernel.org,
        Alexander Wellbrock <a.wellbrock@...lbox.org>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Peter Huewe <peterhuewe@....de>,
        Peter Robinson <pbrobinson@...il.com>,
        linux-integrity@...r.kernel.org
Subject: Re: [PATCH] tpm_tis_spi: add missing SPI device ID entries

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