[PATCH] spidev: Make probe to fail early if a spidev compatible is used

[PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Javier Martinez Canillas]

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(-)

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);
 
-- 
2.33.1

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Uwe Kleine-König]

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Javier Martinez Canillas]

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

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Mark Brown]

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

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Uwe Kleine-König]

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

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




-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Mark Brown]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spidev: Make probe to fail early if a spidev compatible is used

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat