SPI and module auto-loading

SPI and module auto-loading

[Javier Martinez Canillas]

Hello Mark,

We found an issue with module auto-loading on an I2C driver [0] and it turned
out to be a problem on how the I2C subsystem reports the module alias to
user-space. It always report modalias as "i2c:<dev_id>" even when the driver
is probed via DT. The problem with this particular driver is that the I2C
device ID table didn't contain an entry that matched what the I2C core was
reporting as the MODALIAS uevent env var.

I looked at the SPI core and it does the same. It always reports to udev as
modalias "spi:<dev_id>" so the aliases filled in a module from the OF table
are never used.

This can be easily worked around (and probably why it never was an issue) if
the OF and SPI tables are kept in sync but I don't know if that is a hard
requirement for all use-cases (e.g: a SPI driver that is DT only?).

Now, changing this behavior will break module auto-loading for a lot of
drivers relies on the current behavior and don't define a struct of_device_id
or are not using the MODULE_DEVICE_TABLE(of,..) macro but I wonder if that
means that the drivers are broken and should be fixed?

I'm sending an RFC patch [1] to know what you think about it.

Thanks a lot and best regards,
Javier

[0]: https://lkml.org/lkml/2014/9/11/127
[1]
>From a7cd35209a597a578df6c801e5ff7b63b584bf3e Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Thu, 11 Sep 2014 14:31:04 +0200
Subject: [PATCH RFC] spi: core: report OF style modalias when probing using DT

An SPI driver that supports both OF and legacy platforms, will have
both an OF and SPI ID table. This means that when built as a module,
the aliases will be filled from both tables, e.g:

$ modinfo cros_ec_spi | grep alias
alias:          spi:cros-ec-spi
alias:          of:N*T*Cgoogle,cros-ec-spi*

But currently always an alias of the form spi:<dev_id> is reported
even when the it was probed by matching the OF compatible string.

$ cat /sys/devices/12d40000.spi/spi_master/spi2/spi2.0/modalias
spi:cros-ec-spi

Drivers for IP blocks that are included in DT-only platforms, may not
have an updated SPI ID table so if a device is probed by matching its
compatible string, udev can get a MODALIAS uevent env var that doesn't
match with one of the valid aliases so the module won't be auto-loaded
by libkmod/modprobe.

This patch reports OF related uevent env vars (e.g: OF_COMPATIBLE) and
so the module can be auto-loaded and also report the correctly on sysfs:

$ cat /sys/devices/12d40000.spi/spi_master/spi2/spi2.0/modalias
of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

NOTE: this will break module auto-loading for all drivers that rely on the
current behavior so it should not be applied unless is part of a series that
fix all the broken drivers.

 drivers/spi/spi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 95cfe3b..154e1d6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -63,6 +63,10 @@ modalias_show(struct device *dev, struct device_attribute
*a, char *buf)
 	const struct spi_device	*spi = to_spi_device(dev);
 	int len;

+	len = of_device_get_modalias(dev, buf, PAGE_SIZE - 1);
+	if (len != -ENODEV)
+		return len;
+
 	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
 	if (len != -ENODEV)
 		return len;
@@ -124,6 +128,10 @@ static int spi_uevent(struct device *dev, struct
kobj_uevent_env *env)
 	const struct spi_device		*spi = to_spi_device(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;
-- 
2.1.0

Re: SPI and module auto-loading

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Thu, Sep 11, 2014 at 03:03:50PM +0200, Javier Martinez Canillas wrote:

> This can be easily worked around (and probably why it never was an issue) if
> the OF and SPI tables are kept in sync but I don't know if that is a hard
> requirement for all use-cases (e.g: a SPI driver that is DT only?).

I'm not sure I see that as an interesting use case, it seems better to
have drivers usable without DT and it's trivial to do so.

> I'm sending an RFC patch [1] to know what you think about it.
> 
> Thanks a lot and best regards,
> Javier
> 
> [0]: https://lkml.org/lkml/2014/9/11/127
> [1]
> From a7cd35209a597a578df6c801e5ff7b63b584bf3e Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Date: Thu, 11 Sep 2014 14:31:04 +0200
> Subject: [PATCH RFC] spi: core: report OF style modalias when probing using DT

We already have a perfectly good way of sending patches.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: SPI and module auto-loading

[Javier Martinez Canillas]

On 09/11/2014 09:33 PM, Mark Brown wrote:
> On Thu, Sep 11, 2014 at 03:03:50PM +0200, Javier Martinez Canillas wrote:
> 
>> This can be easily worked around (and probably why it never was an issue) if
>> the OF and SPI tables are kept in sync but I don't know if that is a hard
>> requirement for all use-cases (e.g: a SPI driver that is DT only?).
> 
> I'm not sure I see that as an interesting use case, it seems better to
> have drivers usable without DT and it's trivial to do so.
>

Yes, it's trivial but seems like an unnecessary duplication for me. AFAICT the
OF tables are only used to match the devices in spi_match_device() but if both
the OF and SPI tables must be kept in sync to properly report the module
aliases to user-space then I wonder if the OF tables shouldn't just be removed
from the SPI drivers since spi_match_device() will succeed anyways when
calling spi_match_id().

>> I'm sending an RFC patch [1] to know what you think about it.
>> [1]
>> From a7cd35209a597a578df6c801e5ff7b63b584bf3e Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>> Date: Thu, 11 Sep 2014 14:31:04 +0200
>> Subject: [PATCH RFC] spi: core: report OF style modalias when probing using DT
> 
> We already have a perfectly good way of sending patches.
> 

Of course I know how to post patches properly but proposing a patch was not my
intention here since as I said this could break module auto-loading for many
drivers that rely on the current behavior. What I wanted was to explain with
code how the SPI core could report uevents in order to be consistent with what
other subsystems do (e.g: platform drivers):

http://lxr.free-electrons.com/source/drivers/base/platform.c#L717

I should probably had used sharing instead of sending but as a non-native
english speaker sometimes I don't always choose the best wording.

Anyway, I was just raising the issue because if a driver only defines an OF
table and not a SPI table, the driver will be probed correctly but module
auto-loading will not work. So even when it looks like having a SPI id table
is not a requirement for OF, it really is and I think that is not documented.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SPI and module auto-loading

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Fri, Sep 12, 2014 at 11:50:23AM +0200, Javier Martinez Canillas wrote:
> On 09/11/2014 09:33 PM, Mark Brown wrote:

> > I'm not sure I see that as an interesting use case, it seems better to
> > have drivers usable without DT and it's trivial to do so.

> Yes, it's trivial but seems like an unnecessary duplication for me. AFAICT the
> OF tables are only used to match the devices in spi_match_device() but if both
> the OF and SPI tables must be kept in sync to properly report the module
> aliases to user-space then I wonder if the OF tables shouldn't just be removed
> from the SPI drivers since spi_match_device() will succeed anyways when
> calling spi_match_id().

The vendor identifier is an important part of the OF device ID, vendors
can and do end up with different devices of the same name.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: SPI and module auto-loading

[Sjoerd Simons]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

On Fri, 2014-09-12 at 11:14 +0100, Mark Brown wrote:
> On Fri, Sep 12, 2014 at 11:50:23AM +0200, Javier Martinez Canillas wrote:
> > On 09/11/2014 09:33 PM, Mark Brown wrote:
> 
> > > I'm not sure I see that as an interesting use case, it seems better to
> > > have drivers usable without DT and it's trivial to do so.
> 
> > Yes, it's trivial but seems like an unnecessary duplication for me. AFAICT the
> > OF tables are only used to match the devices in spi_match_device() but if both
> > the OF and SPI tables must be kept in sync to properly report the module
> > aliases to user-space then I wonder if the OF tables shouldn't just be removed
> > from the SPI drivers since spi_match_device() will succeed anyways when
> > calling spi_match_id().
> 
> The vendor identifier is an important part of the OF device ID, vendors
> can and do end up with different devices of the same name.

Indeed, which actually points at a problem with module loading for SPI
as the vendor prefix gets dropped when generating the modalias for the
uevent. So if there is a case where we have two SPI devices with the
same device name (but a different vendor identifier), module loading
can't work as the generated modalias for both will be spi:devicename
even if OF can properly distinguish between both.

The core of the "issue" here really is that the way userspace matches an
SPI device to a kernel module and how the kernel matches an SPI to a
driver don't match up (same issue as there is with I2C). While the
kernel can take advantage of the OF table, userspace needs to resolve
the simpler spi: alias, thus essentially using the SPI table. Where as
for the ACPI case, both userspace and kernel will use the ACPI table.

So for things to be consistent for both cases the options are to either:
a) the generated MODALIAS uevent variable should be an OF based alias
  + Upside is that both kernel and userspace can use the full OF
    information for matching
  + Downside is that that would mean adding OF match tables to all
    drivers that can possible used on a DT based system otherwise module
    auto-loading for those will be broken.

b) Stop using OF style matching and rely solely on the SPI id table
  + Downside here is that the vendor prefix isn't used anymore for
    matching. Otoh that's the current status quo for drivers without an
    OF match table and for how userspace matches modules currently.
  + Upside is that no extra work is required for drivers that currently
    work with DT even if they don't have any direct OF support.



-- 
Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Collabora Ltd.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6170 bytes --]

Re: SPI and module auto-loading

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On Mon, Sep 15, 2014 at 10:10:12AM +0200, Sjoerd Simons wrote:
> On Fri, 2014-09-12 at 11:14 +0100, Mark Brown wrote:

> > The vendor identifier is an important part of the OF device ID, vendors
> > can and do end up with different devices of the same name.

> Indeed, which actually points at a problem with module loading for SPI
> as the vendor prefix gets dropped when generating the modalias for the
> uevent. So if there is a case where we have two SPI devices with the
> same device name (but a different vendor identifier), module loading
> can't work as the generated modalias for both will be spi:devicename
> even if OF can properly distinguish between both.

It's a relatively minor risk on that front though.

> So for things to be consistent for both cases the options are to either:
> a) the generated MODALIAS uevent variable should be an OF based alias
>   + Upside is that both kernel and userspace can use the full OF
>     information for matching
>   + Downside is that that would mean adding OF match tables to all
>     drivers that can possible used on a DT based system otherwise module
>     auto-loading for those will be broken.

This isn't a disadvantage for the drivers, anything being used with OF
should have an explicit match table defined.

> b) Stop using OF style matching and rely solely on the SPI id table
>   + Downside here is that the vendor prefix isn't used anymore for
>     matching. Otoh that's the current status quo for drivers without an
>     OF match table and for how userspace matches modules currently.
>   + Upside is that no extra work is required for drivers that currently
>     work with DT even if they don't have any direct OF support.

There's also the option of providing both bits of information in the
event which is less disruptive all round.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: SPI and module auto-loading

[Sjoerd Simons]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

On Mon, 2014-09-15 at 15:58 -0700, Mark Brown wrote:
> On Mon, Sep 15, 2014 at 10:10:12AM +0200, Sjoerd Simons wrote:
> > On Fri, 2014-09-12 at 11:14 +0100, Mark Brown wrote:

> > So for things to be consistent for both cases the options are to either:
> > a) the generated MODALIAS uevent variable should be an OF based alias
> >   + Upside is that both kernel and userspace can use the full OF
> >     information for matching
> >   + Downside is that that would mean adding OF match tables to all
> >     drivers that can possible used on a DT based system otherwise module
> >     auto-loading for those will be broken.
> 
> This isn't a disadvantage for the drivers, anything being used with OF
> should have an explicit match table defined.

Doing a very rough grep for (spi) drivers that do use OF (or are
mentioned in the devicetree documentation or any of the upstream .dts
files use them) but which don't have an OF table. It looks like there
are about 10-15 drivers which are broken in this regard.

Most commonly used and probably trickiest offender seems to be the
m25p80, which exposes a ~125 different SPI Nor chips (all including
driver data) via an id_table but has no OF table to match.


> > b) Stop using OF style matching and rely solely on the SPI id table
> >   + Downside here is that the vendor prefix isn't used anymore for
> >     matching. Otoh that's the current status quo for drivers without an
> >     OF match table and for how userspace matches modules currently.
> >   + Upside is that no extra work is required for drivers that currently
> >     work with DT even if they don't have any direct OF support.
> 
> There's also the option of providing both bits of information in the
> event which is less disruptive all round.

Well neither option i mentioned prevents you from having the same
information in both tables and that doesn't matter at for the kernel
internally (as that can gracefully fall back). 

However for module loading, unless there are userspace changes, you have
to pick between of: and spi: ahead of time as the modalias is expected
to just be a single item.

So i guess we have our next steps :).. First fix up the drivers and then
we can properly submit Javiers strawman patch that was inlined in the
mail that started this tread.
 
-- 
Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Collabora Ltd.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6170 bytes --]