Re: [PATCH] spi: Raise limit on number of chip selects

[Jonas Gorski <jonas.gorski@xxxxxxxxx>]

On Tue, 23 Jan 2024 at 18:47, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 1/23/24 08:50, Jonas Gorski wrote:
> > Hi,
> >
> > On Tue, 23 Jan 2024 at 14:56, Mark Brown <broonie@xxxxxxxxxx> wrote:
> >>
> >> On Tue, Jan 23, 2024 at 01:26:04PM +0000, Christophe Leroy wrote:
> >>> Le 23/01/2024 à 14:18, Mark Brown a écrit :
> >>>> On Tue, Jan 23, 2024 at 12:04:30PM +0100, Herve Codina wrote:
> >>
> >>>>> Moving the SPI_CS_CNT_MAX value from 4 to 8 is not enough to handle my case.
> >>>>> Tested moving SPI_CS_CNT_MAX to 16 and it was ok.
> >>
> >>>> OK, I've also heard 12 as a number which this would cover.
> >>
> >>> By the way the comment in include/linux/spi/spi.h is confusing. This
> >>> SPI_CS_CNT_MAX is really not the max number of CS supported per SPI
> >>> device but the max number of CS supported per SPI controller.
> >>
> >> Well, it's a combination of the comment being confusing and the
> >> implementation being a bit broken - we simply shouldn't be limiting the
> >> number of chip selects per controller, the per device limit is much more
> >> reasonable.  So ideally the code would be changed to reflect the
> >> comment.
> >
> > At a first glance at all places using SPI_CS_CNT_MAX I don't see
> > anything being broken / reading out of bounds if a controller has more
> > chipselects than SPI_CS_CNT_MAX.
> >
> > So I think the check of ctrl->num_chipselect in of_spi_parse_dt() is
> > bogus/unnecessary and is in the wrong place, as this is for parsing a
> > spi device node and not a controller node. The following check for the
> > amount of chip selects defined for the spi device should just check
> > against SPI_CS_CNT_MAX instead of ctrl->num_chipselects.
> > __spi_add_device() later will ensure that any chip selects are valid
> > chip selects, so no need for of_spi_parse_dt() to check that either.
> >
> > But I didn't do a very thorough read, or even tested it, so I might
> > have easily missed something.
> >
>
> struct spi_controller {
>         ...
>         char                            last_cs[SPI_CS_CNT_MAX];
>
> does introduce the limit for controllers.

Right, but that is AFAICT bounded by the number of concurrent/parallel
chipselects supported by the API, not the number of chipselects in the
controller. It only ever iterates over it limited by SPI_CS_CNT_MAX,
and never by spi_controller::num_chipselects. So even if
spi_controller::num_chipselects is higher, it would not lead to
accesses larger than SPI_CS_CNT_MAX - 1 to this array.

For some reason we don't store neither the actual number of supported
parallel chipselects in the controller, nor the amount of chipselects
used by the spi device, so all loops always  need to iterate
SPI_CS_CNT_MAX times and check for the chipselect numbers not being
0xff instead of limiting by the (possible to know) actual number of
chip selects in use.

Best Regards,
Jonas