diff mbox series

spi: Revert modalias changes

Message ID 20210921173222.46514-1-broonie@kernel.org
State Accepted
Commit 96c8395e2166efa86082f3b71567ffd84936439b
Headers show
Series spi: Revert modalias changes | expand

Commit Message

Mark Brown Sept. 21, 2021, 5:32 p.m. UTC
During the v5.13 cycle we updated the SPI subsystem to generate OF style
modaliases for SPI devices, replacing the old Linux style modalises we
used to generate based on spi_device_id which are the DT style name with
the vendor removed.  Unfortunately this means that we start only
reporting OF style modalises and not the old ones and there is nothing
that ensures that drivers list every possible OF compatible string in
their OF ID table.  The result is that there are systems which have been
relying on loading modules based on the old style that are now broken,
as found by Russell King with spi-nor on Macchiatobin.

spi-nor is a particularly problematic case for this, it only lists a
single generic DT compatible jedec,spi-nor in the driver but supports a
huge raft of device specific compatibles, with a large set of part
numbers many of which are offered by multiple vendors.  Russell's
searches of upstream device trees has turned up examples with vendor
names written in non-standard ways too.  To make matters worse up until
8ff16cf77ce3 ("Documentation: devicetree: m25p80: add "nor-jedec"
binding") the generic compatible was not part of the binding so there
are device trees out there written to that binding version which don't
list it all.  The sheer number of parts supported together with our
previous approach of ignoring the vendor ID makes robustly fixing this
by adding compatibles to the spi-nor driver seem problematic, the
current DT binding document does not list all the parts supported by the
driver at the minute (further patches will fix this).

I've also investigated supporting both formats of modalias
simultaneously but that doesn't seem possible, especially without
breaking our userspace ABI which is obviously not viable.

Instead revert the relevant changes for now:

e09f2ab8eecc ("spi: update modalias_show after of_device_uevent_modalias support")
3ce6c9e2617e ("spi: add of_device_uevent_modalias support")

This will unfortunately mean that any system which had started having
modules autoload based on the OF compatibles for drivers that list
things there but not in the spi_device_ids will now not have those
modules load which is itself a regression.  Since it affects a narrower
time window and the particularly problematic spi-nor driver may be
critical to system boot on smaller systems this seems the best of a
series of bad options.  I will start an audit of SPI drivers to identify
and fix cases where things won't autoload using spi_device_id, this is
not great but seems to be the best way forward that anyone has been able
to identify.

Thanks to Russell for both his report and the additional diagnostic and
analysis work he has done here, the detailed research above was his
work.

Fixes: e09f2ab8eecc ("spi: update modalias_show after of_device_uevent_modalias support")
Fixes: 3ce6c9e2617e ("spi: add of_device_uevent_modalias support")
Reported-by: Russell King (Oracle) <linux@armlinux.org.uk>
Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Mark Brown <broonie@kernel.org>

Cc: Andreas Schwab <schwab@suse.de>
Cc: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/spi/spi.c | 8 --------
 1 file changed, 8 deletions(-)

-- 
2.20.1

Comments

Russell King (Oracle) Sept. 21, 2021, 6:06 p.m. UTC | #1
On Tue, Sep 21, 2021 at 06:32:22PM +0100, Mark Brown wrote:
> During the v5.13 cycle we updated the SPI subsystem to generate OF style

> modaliases for SPI devices, replacing the old Linux style modalises we

> used to generate based on spi_device_id which are the DT style name with

> the vendor removed.  Unfortunately this means that we start only

> reporting OF style modalises and not the old ones and there is nothing

> that ensures that drivers list every possible OF compatible string in

> their OF ID table.  The result is that there are systems which have been

> relying on loading modules based on the old style that are now broken,

> as found by Russell King with spi-nor on Macchiatobin.

> 

> spi-nor is a particularly problematic case for this, it only lists a

> single generic DT compatible jedec,spi-nor in the driver but supports a

> huge raft of device specific compatibles, with a large set of part

> numbers many of which are offered by multiple vendors.  Russell's

> searches of upstream device trees has turned up examples with vendor

> names written in non-standard ways too.  To make matters worse up until

> 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add "nor-jedec"

> binding") the generic compatible was not part of the binding so there

> are device trees out there written to that binding version which don't

> list it all.  The sheer number of parts supported together with our

> previous approach of ignoring the vendor ID makes robustly fixing this

> by adding compatibles to the spi-nor driver seem problematic, the

> current DT binding document does not list all the parts supported by the

> driver at the minute (further patches will fix this).

> 

> I've also investigated supporting both formats of modalias

> simultaneously but that doesn't seem possible, especially without

> breaking our userspace ABI which is obviously not viable.

> 

> Instead revert the relevant changes for now:

> 

> e09f2ab8eecc ("spi: update modalias_show after of_device_uevent_modalias support")

> 3ce6c9e2617e ("spi: add of_device_uevent_modalias support")

> 

> This will unfortunately mean that any system which had started having

> modules autoload based on the OF compatibles for drivers that list

> things there but not in the spi_device_ids will now not have those

> modules load which is itself a regression.  Since it affects a narrower

> time window and the particularly problematic spi-nor driver may be

> critical to system boot on smaller systems this seems the best of a

> series of bad options.  I will start an audit of SPI drivers to identify

> and fix cases where things won't autoload using spi_device_id, this is

> not great but seems to be the best way forward that anyone has been able

> to identify.

> 

> Thanks to Russell for both his report and the additional diagnostic and

> analysis work he has done here, the detailed research above was his

> work.

> 

> Fixes: e09f2ab8eecc ("spi: update modalias_show after of_device_uevent_modalias support")

> Fixes: 3ce6c9e2617e ("spi: add of_device_uevent_modalias support")

> Reported-by: Russell King (Oracle) <linux@armlinux.org.uk>

> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>


This is exactly the change I have in my local tree to fix the issue,
so...

Tested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>


Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Mark Brown Sept. 22, 2021, 2:21 p.m. UTC | #2
On Tue, 21 Sep 2021 18:32:22 +0100, Mark Brown wrote:
> During the v5.13 cycle we updated the SPI subsystem to generate OF style

> modaliases for SPI devices, replacing the old Linux style modalises we

> used to generate based on spi_device_id which are the DT style name with

> the vendor removed.  Unfortunately this means that we start only

> reporting OF style modalises and not the old ones and there is nothing

> that ensures that drivers list every possible OF compatible string in

> their OF ID table.  The result is that there are systems which have been

> relying on loading modules based on the old style that are now broken,

> as found by Russell King with spi-nor on Macchiatobin.

> 

> [...]


Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: Revert modalias changes
      commit: 96c8395e2166efa86082f3b71567ffd84936439b

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 57e2499ec1ed..aea037c65985 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,10 +58,6 @@  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_modalias(dev, buf, PAGE_SIZE);
-	if (len != -ENODEV)
-		return len;
-
 	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
 	if (len != -ENODEV)
 		return len;
@@ -367,10 +363,6 @@  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;