As reported by Guenter the limit we've got on the number of chip selects is
set too low for some systems, raise the limit. We should really remove the
hard coded limit but this is needed as a fix so let's do the simple thing
and raise the limit for now.
Fixes: 4d8ff6b0991d ("spi: Add multi-cs memories support in SPI core")
Reported-by: Guenter Roeck <[email protected]>
Suggested-by: Guenter Roeck <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
include/linux/spi/spi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 471fe2ff9066..d71483bf253a 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -21,7 +21,7 @@
#include <uapi/linux/spi/spi.h>
/* Max no. of CS supported per spi device */
-#define SPI_CS_CNT_MAX 4
+#define SPI_CS_CNT_MAX 8
struct dma_chan;
struct software_node;
---
base-commit: 65163d16fcaef37733b5f273ffe4d00d731b34de
change-id: 20240121-spi-multi-cs-max-23e82c815c6d
Best regards,
--
Mark Brown <[email protected]>
On Mon, 22 Jan 2024 01:21:46 +0000, Mark Brown wrote:
> As reported by Guenter the limit we've got on the number of chip selects is
> set too low for some systems, raise the limit. We should really remove the
> hard coded limit but this is needed as a fix so let's do the simple thing
> and raise the limit for now.
>
>
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/1] spi: Raise limit on number of chip selects
commit: 985bfd36b988cd0e3254c5f4aa7cbe2a848403a9
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Hi Mark,
On Mon, 22 Jan 2024 01:21:46 +0000
Mark Brown <[email protected]> wrote:
> As reported by Guenter the limit we've got on the number of chip selects is
> set too low for some systems, raise the limit. We should really remove the
> hard coded limit but this is needed as a fix so let's do the simple thing
> and raise the limit for now.
>
> Fixes: 4d8ff6b0991d ("spi: Add multi-cs memories support in SPI core")
> Reported-by: Guenter Roeck <[email protected]>
> Suggested-by: Guenter Roeck <[email protected]>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> include/linux/spi/spi.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 471fe2ff9066..d71483bf253a 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -21,7 +21,7 @@
> #include <uapi/linux/spi/spi.h>
>
> /* Max no. of CS supported per spi device */
> -#define SPI_CS_CNT_MAX 4
> +#define SPI_CS_CNT_MAX 8
>
> struct dma_chan;
> struct software_node;
>
I got also the issue related to SPI_CS_CNT_MAX introduced by 4d8ff6b0991d ("spi:
Add multi-cs memories support in SPI core").
Errors like the following one are raised at boot for each of my SPI devices:
spi_master spi0: No. of CS is more than max. no. of supported CS
spi_master spi0: Failed to create SPI device for /soc@ff000000/cpm@9c0/spi@a80/idt821034@0
and none of my SPI devices were probed.
On my system, 9 SPI devices are present on my SPI bus and so, I have 9 chip-selects
(gpio chip-selects in my case).
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.
Best regards,
Hervé
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.
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.
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.
Hi,
On Tue, 23 Jan 2024 at 14:56, Mark Brown <[email protected]> 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.
Best Regards,
Jonas
On 1/23/24 08:50, Jonas Gorski wrote:
> Hi,
>
> On Tue, 23 Jan 2024 at 14:56, Mark Brown <[email protected]> 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.
Guenter
On Tue, 23 Jan 2024 at 18:47, Guenter Roeck <[email protected]> wrote:
>
> On 1/23/24 08:50, Jonas Gorski wrote:
> > Hi,
> >
> > On Tue, 23 Jan 2024 at 14:56, Mark Brown <[email protected]> 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
On Wed, Jan 24, 2024 at 02:41:03PM +0100, Jonas Gorski wrote:
> 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.
Yes, we really can do a lot better here if we keep a bit more data
around.
...
> 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.
Or even the highest one ever used.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Le 24/01/2024 à 15:59, Mark Brown a écrit :
> On Wed, Jan 24, 2024 at 02:41:03PM +0100, Jonas Gorski wrote:
>
>> 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.
>
> Yes, we really can do a lot better here if we keep a bit more data
> around.
When I see all those loops over SPI_CS_CNT_MAX I have the feeling it
could have been done a lot easier, for instance by using bitmaps.
Should we revert that commit 4d8ff6b0991d ("spi: Add multi-cs memories
support in SPI core") and implement something simpler ?
Also have the impression that the commit is doing several things at once
and should have been split in several commits, for instance that 'if
((of_property_read_bool(nc, "parallel-memories")) ' stuff seems
unrelated to the implementation of the generic support of multi-chipselects.
Christophe
On Wed, Jan 24, 2024 at 03:28:32PM +0000, Christophe Leroy wrote:
> Should we revert that commit 4d8ff6b0991d ("spi: Add multi-cs memories
> support in SPI core") and implement something simpler ?
I really don't want to keep going through the pain with having people
constantly adding access for chip select that bypass the helpers if we
can avoid it, there's been a constant need for fixups which have just
added to the pain with the multi CS stuff. My thinking was to get the
API in place and then improve the implementation behind it.