| Message ID | 20240122-spi-multi-cs-max-v1-1-a7e98cd5f6c7@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 985bfd36b988cd0e3254c5f4aa7cbe2a848403a9 |
| Headers | show |
| Series | spi: Raise limit on number of chip selects | expand |
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 <broonie@kernel.org> 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 <linux@roeck-us.net> > Suggested-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > 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 <broonie@kernel.org> 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 <broonie@kernel.org> 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 <linux@roeck-us.net> wrote: > > On 1/23/24 08:50, Jonas Gorski wrote: > > Hi, > > > > On Tue, 23 Jan 2024 at 14:56, Mark Brown <broonie@kernel.org> 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
... > 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)
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.
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.
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;
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 <linux@roeck-us.net> Suggested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Mark Brown <broonie@kernel.org> --- include/linux/spi/spi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 65163d16fcaef37733b5f273ffe4d00d731b34de change-id: 20240121-spi-multi-cs-max-23e82c815c6d Best regards,