| Message ID | acc4ff4b-811a-4a6d-8f58-9d8da3be40bb@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] spi: dt-bindings: spi-rockchip: restrict num-cs | expand |
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
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 >>
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
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:
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(+) -- 2.39.2