| Message ID | 54119DB6.8020807@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 We already have a perfectly good way of sending patches.
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@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 > > 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.
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.
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.
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.
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.
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)