| Message ID | 20210716133915.14697-1-eajames@linux.ibm.com |
|---|---|
| Headers | show |
| Series | spi: fsi: Reduce max transfer size to 8 bytes | expand |
On Fri, 2021-07-16 at 18:19 +0100, Mark Brown wrote: > On Fri, Jul 16, 2021 at 08:39:14AM -0500, Eddie James wrote: > > Security changes have forced the SPI controllers to be limited to > > 8 byte reads. Refactor the sequencing to just handle 8 bytes at a > > time. Security changes in the SPI controller - in the device microcode. I can reword the commit if you like. Thanks, Eddie > > Which security changes where - somewhere else in Linux?
From: Eddie James > Sent: 16 July 2021 14:39 > > The security restrictions on the FSI-attached SPI controllers have > been applied universally to all controllers, so the controller can no > longer transfer more than 8 bytes for one transfer. Refactor the driver > to remove the looping and support for larger transfers, and remove the > "restricted" compatible string, as all the controllers are now > considered restricted. Aren't there significant performance (and device wear?) penalties for doing short SPI eeprom writes? IIRC (and I'm not getting my serial busses confused) a write request can request an aligned transfer of up to (typically) 32 bytes. At which point you need to wait for the status to indicate 'complete'. So restricting writes to 8 bytes increases block write times by a factor of 4. Increasing the numbers of writes by a factor or 4 may also have an effect on device wear - but that is more likely only affected by erase cycles. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > Security changes in the SPI controller - in the device microcode. I can > reword the commit if you like. How will people end up running this device microcode? Is this a bug fix, or is this going to needlessly reduce performance for people with existing hardware?
On Mon, 2021-07-19 at 16:20 +0100, Mark Brown wrote: > On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > > > Security changes in the SPI controller - in the device microcode. I > > can > > reword the commit if you like. > > How will people end up running this device microcode? Is this a bug > fix, or is this going to needlessly reduce performance for people > with > existing hardware? The hardware is still in development. As part of the development, the device microcode was changed to restrict transfers. The reason for this restriction was "security concerns". This restriction disallows the loop (or branch-if-not-equal-and-increment) sequencer command. It also does not allow the read (or shift in if you prefer) command to specify the number of bytes in the command itself. Rather, the number of bits to shift in must be specified in a separate control register. This effectively means that the controller cannot transfer more than 8 bytes at a time. Therefore I suppose this is effectively a bug fix. There will be no hardware available without these restrictions, so it is not a needless reduction in performance. Every system that can run this driver will run the more restrictive device microcode. Thanks, Eddie
On Sat, 2021-07-17 at 13:46 +0000, David Laight wrote: > From: Eddie James > > Sent: 16 July 2021 14:39 > > > > The security restrictions on the FSI-attached SPI controllers have > > been applied universally to all controllers, so the controller can > > no > > longer transfer more than 8 bytes for one transfer. Refactor the > > driver > > to remove the looping and support for larger transfers, and remove > > the > > "restricted" compatible string, as all the controllers are now > > considered restricted. > > Aren't there significant performance (and device wear?) penalties > for doing short SPI eeprom writes? Probably so. However there is no choice in the matter, as the SPI controller can't process larger reads/writes. (I should note that the controller/driver CAN process up to 40 byte writes. However, the driver must report the minimum of read and write sizes in the max_transfer_size callback, so no client driver should request larger than the max read size - 8 bytes). Thanks, Eddie > > IIRC (and I'm not getting my serial busses confused) a write request > can request an aligned transfer of up to (typically) 32 bytes. > At which point you need to wait for the status to indicate > 'complete'. > > So restricting writes to 8 bytes increases block write times > by a factor of 4. > > Increasing the numbers of writes by a factor or 4 may also have > an effect on device wear - but that is more likely only affected > by erase cycles. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK > Registration No: 1397386 (Wales) >
On Fri, 16 Jul 2021 08:39:13 -0500, Eddie James wrote: > The security restrictions on the FSI-attached SPI controllers have > been applied universally to all controllers, so the controller can no > longer transfer more than 8 bytes for one transfer. Refactor the driver > to remove the looping and support for larger transfers, and remove the > "restricted" compatible string, as all the controllers are now > considered restricted. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: fsi: Reduce max transfer size to 8 bytes commit: 34d34a56a5ea1e54a5af4f34c6ac9df724129351 [2/2] dt-bindings: fsi: Remove ibm,fsi2spi-restricted compatible commit: 2b2d4dfca4e7cb6de70985b1579a6c08c027b8c9 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
From: Eddie James <eajames@linux.ibm.com> > Sent: 19 July 2021 16:47 > To: Mark Brown <broonie@kernel.org> > Cc: devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; robh+dt@kernel.org; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org > Subject: Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes > > On Mon, 2021-07-19 at 16:20 +0100, Mark Brown wrote: > > On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > > > > > Security changes in the SPI controller - in the device microcode. I > > > can > > > reword the commit if you like. > > > > How will people end up running this device microcode? Is this a bug > > fix, or is this going to needlessly reduce performance for people > > with > > existing hardware? > > The hardware is still in development. As part of the development, the > device microcode was changed to restrict transfers. The reason for this > restriction was "security concerns". This restriction disallows the > loop (or branch-if-not-equal-and-increment) sequencer command. It also > does not allow the read (or shift in if you prefer) command to specify > the number of bytes in the command itself. Rather, the number of bits > to shift in must be specified in a separate control register. This > effectively means that the controller cannot transfer more than 8 bytes > at a time. > Therefore I suppose this is effectively a bug fix. There will be no > hardware available without these restrictions, so it is not a needless > reduction in performance. Every system that can run this driver will > run the more restrictive device microcode. So just say that release versions of the hardware won't support more than 8 byte transfers. Having said that, you might want a loop in the driver so that application requests for longer transfers are implemented with multiple hardware requests. I do also wonder why there is support in the main kernel sources for hardware that doesn't actually exist. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jul 20, 2021 at 01:04:38PM +0000, David Laight wrote: > Having said that, you might want a loop in the driver so that > application requests for longer transfers are implemented > with multiple hardware requests. No, that's something that should be and indeed is done in the core - this isn't the only hardware out there with some kind of restriction on length. > I do also wonder why there is support in the main kernel sources > for hardware that doesn't actually exist. We encourage vendors to get support for their devices upstream prior to hardware availability so that users are able to run upstream when they get access to hardware, this means users aren't forced to run out of tree code needlessly and greatly eases deployment.
From: Mark Brown > Sent: 20 July 2021 14:13 > > On Tue, Jul 20, 2021 at 01:04:38PM +0000, David Laight wrote: > > > Having said that, you might want a loop in the driver so that > > application requests for longer transfers are implemented > > with multiple hardware requests. > > No, that's something that should be and indeed is done in the core - > this isn't the only hardware out there with some kind of restriction on > length. Ah, ok, there is another loop before any 'users'. > > I do also wonder why there is support in the main kernel sources > > for hardware that doesn't actually exist. > > We encourage vendors to get support for their devices upstream prior to > hardware availability so that users are able to run upstream when they > get access to hardware, this means users aren't forced to run out of > tree code needlessly and greatly eases deployment. This one just seemed a bit premature. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)