| Message ID | 20211109225920.1158920-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spidev: Make probe to fail early if a spidev compatible is used | expand |
Hello, On Tue, Nov 09, 2021 at 11:59:20PM +0100, Javier Martinez Canillas wrote: > Some Device Trees don't use a real device name in the compatible string > for SPI devices nodes, abusing the fact that the spidev driver name is > used to match as a fallback when a SPI device ID table is not defined. > > But since commit 6840615f85f6 ("spi: spidev: Add SPI ID table") a table > for SPI device IDs was added to the driver breaking the assumption that > these DTs were relying on. > > There has been a warning message for some time since commit 956b200a846e > ("spi: spidev: Warn loudly if instantiated from DT as "spidev""), making > quite clear that this case is not really supported by the spidev driver. > > Since these devices won't match anyways after the mentioned commit, there > is no point to continue if an spidev compatible is used. Let's just make > the driver probe to fail early. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Up to 6840615f85f6 the choices you had to use the spidev driver were (assuing a dt machine): a) Use compatible = "spidev" and ignore the warning b) Use compatible = $chipname and add $chipname to the list of supported devices for the spidev driver. (e.g. "rohm,dh2228fv") c) Use compatible = $chipname and force binding the spidev driver using echo spidev > /sys/bus/spi/devices/spiX.Y/driver_override echo spiX.Y > /sys/bus/spi/drivers/spidev/bind Commit 6840615f85f6 changed that in situation a) you had to switch to c) (well, or b) adding "spidev" to the spi id list). With the change introduced by this patch, you make it impossible to bind the spidev driver to such a device (without kernel source changes) even using approach c). I wonder if this is too harsh given that changing the dtb is difficult on some machines. I think I prefer the status quo that people who use plain "spidev" as compatible now have to do c) and get the warning. Maybe they notice that they could switch to using the right chipname now to improve their situation where the procedure to use the device is identical but the warning is gone. Best regards Uwe
Hello Uwe, On 11/10/21 08:42, Uwe Kleine-König wrote: > Hello, > > On Tue, Nov 09, 2021 at 11:59:20PM +0100, Javier Martinez Canillas wrote: >> Some Device Trees don't use a real device name in the compatible string >> for SPI devices nodes, abusing the fact that the spidev driver name is >> used to match as a fallback when a SPI device ID table is not defined. >> >> But since commit 6840615f85f6 ("spi: spidev: Add SPI ID table") a table >> for SPI device IDs was added to the driver breaking the assumption that >> these DTs were relying on. >> >> There has been a warning message for some time since commit 956b200a846e >> ("spi: spidev: Warn loudly if instantiated from DT as "spidev""), making >> quite clear that this case is not really supported by the spidev driver. >> >> Since these devices won't match anyways after the mentioned commit, there >> is no point to continue if an spidev compatible is used. Let's just make >> the driver probe to fail early. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Up to 6840615f85f6 the choices you had to use the spidev driver were > (assuing a dt machine): > > a) Use compatible = "spidev" and ignore the warning > b) Use compatible = $chipname and add $chipname to the list of > supported devices for the spidev driver. (e.g. "rohm,dh2228fv") > c) Use compatible = $chipname and force binding the spidev driver using > > echo spidev > /sys/bus/spi/devices/spiX.Y/driver_override > echo spiX.Y > /sys/bus/spi/drivers/spidev/bind > > Commit 6840615f85f6 changed that in situation a) you had to switch to c) > (well, or b) adding "spidev" to the spi id list). > > With the change introduced by this patch, you make it impossible to bind > the spidev driver to such a device (without kernel source changes) even > using approach c). I wonder if this is too harsh given that changing the > dtb is difficult on some machines. > Right. I completely forgot about driver_override. I wonder if the warning should mention that, so users can know how to get it to match again after commit 6840615f85f6. Because currently they would notice a change in behavior but may not know how to make it to work again. Honestly I would just stop supporting it, since as mentioned it was really an abuse on the driver model device matching. But I believe that should be made clear what the situation is. What's actually supported and what's not. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, Nov 10, 2021 at 08:42:47AM +0100, Uwe Kleine-König wrote: > Up to 6840615f85f6 the choices you had to use the spidev driver were > (assuing a dt machine): > a) Use compatible = "spidev" and ignore the warning > b) Use compatible = $chipname and add $chipname to the list of > supported devices for the spidev driver. (e.g. "rohm,dh2228fv") > c) Use compatible = $chipname and force binding the spidev driver using > echo spidev > /sys/bus/spi/devices/spiX.Y/driver_override > echo spiX.Y > /sys/bus/spi/drivers/spidev/bind > Commit 6840615f85f6 changed that in situation a) you had to switch to c) > (well, or b) adding "spidev" to the spi id list). > With the change introduced by this patch, you make it impossible to bind > the spidev driver to such a device (without kernel source changes) even > using approach c). I wonder if this is too harsh given that changing the > dtb is difficult on some machines. Following up from discussion on IRC: it's not clear to me how option c is affected? The change only causes an error if of_device_is_compatible() is true and driver_override works with spi_device_id not compatibles (I didn't actually test, in the middle of some other stuff right now).
On Thu, Nov 18, 2021 at 06:21:27PM +0000, Mark Brown wrote: > On Wed, Nov 10, 2021 at 08:42:47AM +0100, Uwe Kleine-König wrote: > > > Up to 6840615f85f6 the choices you had to use the spidev driver were > > (assuing a dt machine): > > > a) Use compatible = "spidev" and ignore the warning > > b) Use compatible = $chipname and add $chipname to the list of > > supported devices for the spidev driver. (e.g. "rohm,dh2228fv") > > c) Use compatible = $chipname and force binding the spidev driver using > > > > echo spidev > /sys/bus/spi/devices/spiX.Y/driver_override > > echo spiX.Y > /sys/bus/spi/drivers/spidev/bind > > > Commit 6840615f85f6 changed that in situation a) you had to switch to c) > > (well, or b) adding "spidev" to the spi id list). > > > With the change introduced by this patch, you make it impossible to bind > > the spidev driver to such a device (without kernel source changes) even > > using approach c). I wonder if this is too harsh given that changing the > > dtb is difficult on some machines. > > Following up from discussion on IRC: it's not clear to me how option c > is affected? The change only causes an error if of_device_is_compatible() > is true and driver_override works with spi_device_id not compatibles (I > didn't actually test, in the middle of some other stuff right now). It affects c) only if the device tree has a device with compatible = "spidev". For such a device the history is: - Before 956b200a846e ("spi: spidev: Warn loudly if instantiated from DT as "spidev"") in v4.1-rc1: Just bound silently - After 956b200a846e up to 6840615f85f6 ("spi: spidev: Add SPI ID table") in v5.15-rc6: The device was automatically bound with a warning - After 6840615f85f6: The device doesn't bind automatically, when using driver_override you get a warning. - With the proposed patch: The device cannot be bound even using driver_override Not this affects also devices that use compatible = "myvender,devicename", "spidev"; . Best regards Uwe
Hello Uwe, On 11/19/21 08:40, Uwe Kleine-König wrote: [snip] > > It affects c) only if the device tree has a device with compatible = > "spidev". For such a device the history is: > > - Before 956b200a846e ("spi: spidev: Warn loudly if instantiated from > DT as "spidev"") in v4.1-rc1: > Just bound silently > > - After 956b200a846e up to 6840615f85f6 ("spi: spidev: Add SPI ID > table") in v5.15-rc6: > The device was automatically bound with a warning > > - After 6840615f85f6: > The device doesn't bind automatically, when using driver_override > you get a warning. > > - With the proposed patch: > The device cannot be bound even using driver_override > My understanding is that there's an agreement that using "spidev" as the specific compatible string is something that should not be supported. > Not this affects also devices that use > > compatible = "myvender,devicename", "spidev"; > This is indeed a corner case and I'm less sure what the kernel should do about it. I just learned now that of_device_is_compatible() return value is not a boolean but instead a "score": https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L455 I wonder if we could add another helper that returns the index instead, and do: of_device_is_compatible_index(spi->dev.of_node, "spidev") == 0 Another option is to add an of_device_is_compatible_specific() helper. Or just consider DT nodes with a general "spidev" compatible string to also not be valid. I would lean towards this one I think. Best regards,
On Fri, Nov 19, 2021 at 09:32:32AM +0100, Javier Martinez Canillas wrote: > On 11/19/21 08:40, Uwe Kleine-König wrote: > > Not this affects also devices that use > > compatible = "myvender,devicename", "spidev"; > This is indeed a corner case and I'm less sure what the kernel should do > about it. I just learned now that of_device_is_compatible() return value TBH I feel like that falls into the same bucket as any other uses of spidev so I'm not overly worried. Grepping around it looks like we have no examples of this in tree, only a few plain spidevs in DTs for older platforms that were most likely converted from board files and *probably* aren't too relevant at this point. > Or just consider DT nodes with a general "spidev" compatible string to > also not be valid. I would lean towards this one I think. Yes, I think so. Your other options are worth exploring if it turns out to be an issue but hopefully it's not.
Hello Mark, On 11/23/21 15:55, Mark Brown wrote: > On Fri, Nov 19, 2021 at 09:32:32AM +0100, Javier Martinez Canillas wrote: >> On 11/19/21 08:40, Uwe Kleine-König wrote: > >>> Not this affects also devices that use > >>> compatible = "myvender,devicename", "spidev"; > >> This is indeed a corner case and I'm less sure what the kernel should do >> about it. I just learned now that of_device_is_compatible() return value > > TBH I feel like that falls into the same bucket as any other uses of > spidev so I'm not overly worried. Grepping around it looks like we have > no examples of this in tree, only a few plain spidevs in DTs for older > platforms that were most likely converted from board files and *probably* > aren't too relevant at this point. > Agreed. >> Or just consider DT nodes with a general "spidev" compatible string to >> also not be valid. I would lean towards this one I think. > > Yes, I think so. Your other options are worth exploring if it turns out > to be an issue but hopefully it's not. > I think that's a good plan. Best regards,
diff --git drivers/spi/spidev.c drivers/spi/spidev.c index 1bd73e322b7b..4cfa250f16d8 100644 --- drivers/spi/spidev.c +++ drivers/spi/spidev.c @@ -751,9 +751,10 @@ static int spidev_probe(struct spi_device *spi) * compatible string, it is a Linux implementation thing * rather than a description of the hardware. */ - WARN(spi->dev.of_node && - of_device_is_compatible(spi->dev.of_node, "spidev"), - "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); + if (spi->dev.of_node && of_device_is_compatible(spi->dev.of_node, "spidev")) { + dev_err(&spi->dev, "spidev listed directly in DT is not supported\n"); + return -EINVAL; + } spidev_probe_acpi(spi);
Some Device Trees don't use a real device name in the compatible string for SPI devices nodes, abusing the fact that the spidev driver name is used to match as a fallback when a SPI device ID table is not defined. But since commit 6840615f85f6 ("spi: spidev: Add SPI ID table") a table for SPI device IDs was added to the driver breaking the assumption that these DTs were relying on. There has been a warning message for some time since commit 956b200a846e ("spi: spidev: Warn loudly if instantiated from DT as "spidev""), making quite clear that this case is not really supported by the spidev driver. Since these devices won't match anyways after the mentioned commit, there is no point to continue if an spidev compatible is used. Let's just make the driver probe to fail early. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- This patch has only been built tested. I'm posting after a conversation with Mark and Uwe on IRC. Best regards, Javier drivers/spi/spidev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)