Re: [PATCH v1] spi: dt-bindings: spi-rockchip: restrict num-cs

[Luis de Arquer <ldearquer@gmail.com>]

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