* [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
@ 2024-01-22 22:59 Johan Jonker
2024-01-23 8:33 ` Krzysztof Kozlowski
2024-01-23 9:17 ` Luis de Arquer
0 siblings, 2 replies; 7+ messages in thread
From: Johan Jonker @ 2024-01-22 22:59 UTC (permalink / raw)
To: broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
In the driver spi-rockchip.c max_native_cs is limited to 4 and the
default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
index e4941e9212d1..00d555bcbad3 100644
--- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
@@ -65,6 +65,11 @@ properties:
- const: tx
- const: rx
+ num-cs:
+ default: 1
+ minimum: 1
+ maximum: 4
+
rx-sample-delay-ns:
default: 0
description:
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-22 22:59 [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs Johan Jonker
@ 2024-01-23 8:33 ` Krzysztof Kozlowski
2024-01-23 9:17 ` Luis de Arquer
1 sibling, 0 replies; 7+ messages in thread
From: Krzysztof Kozlowski @ 2024-01-23 8:33 UTC (permalink / raw)
To: Johan Jonker, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
On 22/01/2024 23:59, Johan Jonker wrote:
> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-22 22:59 [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs Johan Jonker
2024-01-23 8:33 ` Krzysztof Kozlowski
@ 2024-01-23 9:17 ` Luis de Arquer
2024-01-23 10:49 ` Johan Jonker
1 sibling, 1 reply; 7+ messages in thread
From: Luis de Arquer @ 2024-01-23 9:17 UTC (permalink / raw)
To: Johan Jonker, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
On 1/22/24 23:59, Johan Jonker wrote:
> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>
Doesn't num-cs include gpio chip selects too?
I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
Luis
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
> Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
> index e4941e9212d1..00d555bcbad3 100644
> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
> @@ -65,6 +65,11 @@ properties:
> - const: tx
> - const: rx
>
> + num-cs:
> + default: 1
> + minimum: 1
> + maximum: 4
> +
> rx-sample-delay-ns:
> default: 0
> description:
> --
> 2.39.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-23 9:17 ` Luis de Arquer
@ 2024-01-23 10:49 ` Johan Jonker
2024-01-23 13:51 ` Luis de Arquer
2024-01-23 17:45 ` Robin Murphy
0 siblings, 2 replies; 7+ messages in thread
From: Johan Jonker @ 2024-01-23 10:49 UTC (permalink / raw)
To: Luis de Arquer, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
On 1/23/24 10:17, Luis de Arquer wrote:
> On 1/22/24 23:59, Johan Jonker wrote:
>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>
>
> Doesn't num-cs include gpio chip selects too?
> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
Given that bindings and Linux drivers capabilities are 2 separate things.
However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
Currently that limit is set to 4 in the mainline driver.
You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.
Johan
>
> Luis
>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>> Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> index e4941e9212d1..00d555bcbad3 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> @@ -65,6 +65,11 @@ properties:
>> - const: tx
>> - const: rx
>>
>> + num-cs:
>> + default: 1
>> + minimum: 1
>> + maximum: 4
>> +
>> rx-sample-delay-ns:
>> default: 0
>> description:
>> --
>> 2.39.2
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-23 10:49 ` Johan Jonker
@ 2024-01-23 13:51 ` Luis de Arquer
2024-01-23 17:45 ` Robin Murphy
1 sibling, 0 replies; 7+ messages in thread
From: Luis de Arquer @ 2024-01-23 13:51 UTC (permalink / raw)
To: Johan Jonker, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
On 1/23/24 11:49, Johan Jonker wrote:
>
>
> On 1/23/24 10:17, Luis de Arquer wrote:
>> On 1/22/24 23:59, Johan Jonker wrote:
>>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>>
>>
>
>> Doesn't num-cs include gpio chip selects too?
>> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
>
> Given that bindings and Linux drivers capabilities are 2 separate things.
> However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
> Currently that limit is set to 4 in the mainline driver.
> You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.
Hi Johan,
OK to that, I was not aware a driver could limit num-cs (I thought it
could be extended with as many gpios as needed without involving the
controller driver). I thought of num-cs as a spi subsystem generic thing.
In fact, I was reviewing the setup I mentioned, and even it defines 12
cs in the controller with no driver complaints, so far only 3 of them
are mapped to devices (I'll have to review the driver before going any
futher!)
Luis
>
> Johan
>
>>
>> Luis
>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>> Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> index e4941e9212d1..00d555bcbad3 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> @@ -65,6 +65,11 @@ properties:
>>> - const: tx
>>> - const: rx
>>>
>>> + num-cs:
>>> + default: 1
>>> + minimum: 1
>>> + maximum: 4
>>> +
>>> rx-sample-delay-ns:
>>> default: 0
>>> description:
>>> --
>>> 2.39.2
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-23 10:49 ` Johan Jonker
2024-01-23 13:51 ` Luis de Arquer
@ 2024-01-23 17:45 ` Robin Murphy
2024-01-26 19:23 ` Luis de Arquer
1 sibling, 1 reply; 7+ messages in thread
From: Robin Murphy @ 2024-01-23 17:45 UTC (permalink / raw)
To: Johan Jonker, Luis de Arquer, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
On 23/01/2024 10:49 am, Johan Jonker wrote:
>
>
> On 1/23/24 10:17, Luis de Arquer wrote:
>> On 1/22/24 23:59, Johan Jonker wrote:
>>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>>
>>
>
>> Doesn't num-cs include gpio chip selects too?
>> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
>
> Given that bindings and Linux drivers capabilities are 2 separate things.
Er, that's the whole point - bindings and drivers *are* separate things,
and bindings do not describe drivers. Not least since the fundamental
model is to have one canonical binding for multiple different drivers to
consume.
There seems to be some ambiguity as to whether the common "num-cs"
property is supposed to describe the number of dedicated hardware
chipselects or the total number including additional GPIOs, but either
way this change appears to be objectively wrong - if it's the former
than the comment in the driver plus a survey of a few TRMs implies that
the maximum number of hardware lines is 2; if it's the latter then
obviously it's valid for a platform to wire up 3 or more additional
GPIOs as desired, and for a DT to accurately describe that, regardless
of whether any particular consumer happens to support it yet or not. For
example, AFAICS the U-Boot and FreeBSD drivers for Rockchip SPI appear
not to support GPIO chipselects at all.
Thanks,
Robin.
> However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
> Currently that limit is set to 4 in the mainline driver.
> You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.
>
> Johan
>
>>
>> Luis
>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>> Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> index e4941e9212d1..00d555bcbad3 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> @@ -65,6 +65,11 @@ properties:
>>> - const: tx
>>> - const: rx
>>>
>>> + num-cs:
>>> + default: 1
>>> + minimum: 1
>>> + maximum: 4
>>> +
>>> rx-sample-delay-ns:
>>> default: 0
>>> description:
>>> --
>>> 2.39.2
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs
2024-01-23 17:45 ` Robin Murphy
@ 2024-01-26 19:23 ` Luis de Arquer
0 siblings, 0 replies; 7+ messages in thread
From: Luis de Arquer @ 2024-01-26 19:23 UTC (permalink / raw)
To: Robin Murphy, Johan Jonker, broonie
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, heiko, linux-spi,
devicetree, linux-arm-kernel, linux-rockchip
Hi Robin,
On 1/23/24 18:45, Robin Murphy wrote:
> On 23/01/2024 10:49 am, Johan Jonker wrote:
>>
>>
>> On 1/23/24 10:17, Luis de Arquer wrote:
>>> On 1/22/24 23:59, Johan Jonker wrote:
>>>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>>>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>>>
>>>
>>
>>> Doesn't num-cs include gpio chip selects too?
>>> I have a setup with num-cs = <12> which uses non-native cs-gpios just
>>> fine
>>
>> Given that bindings and Linux drivers capabilities are 2 separate things.
>
> Er, that's the whole point - bindings and drivers *are* separate things,
> and bindings do not describe drivers. Not least since the fundamental
> model is to have one canonical binding for multiple different drivers to
> consume.
>
> There seems to be some ambiguity as to whether the common "num-cs"
> property is supposed to describe the number of dedicated hardware
> chipselects or the total number including additional GPIOs, but either
> way this change appears to be objectively wrong - if it's the former
> than the comment in the driver plus a survey of a few TRMs implies that
> the maximum number of hardware lines is 2; if it's the latter then
> obviously it's valid for a platform to wire up 3 or more additional
> GPIOs as desired, and for a DT to accurately describe that, regardless
> of whether any particular consumer happens to support it yet or not. For
> example, AFAICS the U-Boot and FreeBSD drivers for Rockchip SPI appear
> not to support GPIO chipselects at all.
>
I always understood num-cs was a spi subsystem thing, which can be
extended with as many gpios as needed.
However, it looks spi-rockchip may be limiting num-cs to 4 after all.
It keeps a copy of the state of the chip selects into an array
'cs_asserted' of size 4. It wouldn't be a problem if it wasn't because
the driver sets the flag SPI_CONTROLLER_GPIO_SS, which makes driver's
set_cs() function to be called even for gpio lines.
All this leads to an out of bounds access when num-cs > 4.
I was able to reproduce the issue with 6 spidev devices (all with gpio
cs) adding 2 guard u8 variables in 'struct rockchip_spi' right after
'cs_asserted' array, and they got overwritten when accessing devices
spidev0.4 and spidev0.5
Even though I did the test with a downstream kernel (I need more time to
test on mainline), the driver is mostly identical, and the problem seems
to persist in mainline.
I reviewed and prepared a fix on my system. I am building the fix on
linux-rockchip tree, and I will try to post it for review soon.
Luis
> Thanks,
> Robin.
>
>> However this document has also a purpose that must notify mainline
>> maintainers if users submit bogus DT values.
>> Currently that limit is set to 4 in the mainline driver.
>> You are free to submit a real board file/patch serie afterwords as
>> proof for review with 12 spi chips and then adjust this limit and
>> increase ROCKCHIP_SPI_MAX_CS_NUM.
>>
>> Johan
>>
>>>
>>> Luis
>>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>>> Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>>> b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>>> index e4941e9212d1..00d555bcbad3 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>>> @@ -65,6 +65,11 @@ properties:
>>>> - const: tx
>>>> - const: rx
>>>>
>>>> + num-cs:
>>>> + default: 1
>>>> + minimum: 1
>>>> + maximum: 4
>>>> +
>>>> rx-sample-delay-ns:
>>>> default: 0
>>>> description:
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-rockchip mailing list
>>>> Linux-rockchip@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-26 19:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-22 22:59 [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs Johan Jonker
2024-01-23 8:33 ` Krzysztof Kozlowski
2024-01-23 9:17 ` Luis de Arquer
2024-01-23 10:49 ` Johan Jonker
2024-01-23 13:51 ` Luis de Arquer
2024-01-23 17:45 ` Robin Murphy
2024-01-26 19:23 ` Luis de Arquer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).