* [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes
@ 2021-07-16 13:39 Eddie James
2021-07-16 13:39 ` [PATCH 1/2] " Eddie James
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Eddie James @ 2021-07-16 13:39 UTC (permalink / raw)
To: linux-spi
Cc: devicetree, openbmc, Eddie James, linux-kernel, robh+dt, broonie
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.
Eddie James (2):
spi: fsi: Reduce max transfer size to 8 bytes
dt-bindings: fsi: Remove ibm,fsi2spi-restricted compatible
.../devicetree/bindings/fsi/ibm,fsi2spi.yaml | 1 -
drivers/spi/spi-fsi.c | 125 +++---------------
2 files changed, 22 insertions(+), 104 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 13:39 [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes Eddie James @ 2021-07-16 13:39 ` Eddie James 2021-07-16 17:19 ` Mark Brown 2021-07-16 13:39 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm, fsi2spi-restricted compatible Eddie James ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Eddie James @ 2021-07-16 13:39 UTC (permalink / raw) To: linux-spi Cc: devicetree, openbmc, Eddie James, linux-kernel, robh+dt, broonie 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. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/spi/spi-fsi.c | 125 ++++++++---------------------------------- 1 file changed, 22 insertions(+), 103 deletions(-) diff --git a/drivers/spi/spi-fsi.c b/drivers/spi/spi-fsi.c index 87f8829c3995..829770b8ec74 100644 --- a/drivers/spi/spi-fsi.c +++ b/drivers/spi/spi-fsi.c @@ -25,16 +25,11 @@ #define SPI_FSI_BASE 0x70000 #define SPI_FSI_INIT_TIMEOUT_MS 1000 -#define SPI_FSI_MAX_XFR_SIZE 2048 -#define SPI_FSI_MAX_XFR_SIZE_RESTRICTED 8 +#define SPI_FSI_MAX_RX_SIZE 8 +#define SPI_FSI_MAX_TX_SIZE 40 #define SPI_FSI_ERROR 0x0 #define SPI_FSI_COUNTER_CFG 0x1 -#define SPI_FSI_COUNTER_CFG_LOOPS(x) (((u64)(x) & 0xffULL) << 32) -#define SPI_FSI_COUNTER_CFG_N2_RX BIT_ULL(8) -#define SPI_FSI_COUNTER_CFG_N2_TX BIT_ULL(9) -#define SPI_FSI_COUNTER_CFG_N2_IMPLICIT BIT_ULL(10) -#define SPI_FSI_COUNTER_CFG_N2_RELOAD BIT_ULL(11) #define SPI_FSI_CFG1 0x2 #define SPI_FSI_CLOCK_CFG 0x3 #define SPI_FSI_CLOCK_CFG_MM_ENABLE BIT_ULL(32) @@ -76,8 +71,6 @@ struct fsi_spi { struct device *dev; /* SPI controller device */ struct fsi_device *fsi; /* FSI2SPI CFAM engine device */ u32 base; - size_t max_xfr_size; - bool restricted; }; struct fsi_spi_sequence { @@ -241,7 +234,7 @@ static int fsi_spi_reset(struct fsi_spi *ctx) return fsi_spi_write_reg(ctx, SPI_FSI_STATUS, 0ULL); } -static int fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) +static void fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) { /* * Add the next byte of instruction to the 8-byte sequence register. @@ -251,8 +244,6 @@ static int fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) */ seq->data |= (u64)val << seq->bit; seq->bit -= 8; - - return ((64 - seq->bit) / 8) - 2; } static void fsi_spi_sequence_init(struct fsi_spi_sequence *seq) @@ -261,71 +252,11 @@ static void fsi_spi_sequence_init(struct fsi_spi_sequence *seq) seq->data = 0ULL; } -static int fsi_spi_sequence_transfer(struct fsi_spi *ctx, - struct fsi_spi_sequence *seq, - struct spi_transfer *transfer) -{ - int loops; - int idx; - int rc; - u8 val = 0; - u8 len = min(transfer->len, 8U); - u8 rem = transfer->len % len; - - loops = transfer->len / len; - - if (transfer->tx_buf) { - val = SPI_FSI_SEQUENCE_SHIFT_OUT(len); - idx = fsi_spi_sequence_add(seq, val); - - if (rem) - rem = SPI_FSI_SEQUENCE_SHIFT_OUT(rem); - } else if (transfer->rx_buf) { - val = SPI_FSI_SEQUENCE_SHIFT_IN(len); - idx = fsi_spi_sequence_add(seq, val); - - if (rem) - rem = SPI_FSI_SEQUENCE_SHIFT_IN(rem); - } else { - return -EINVAL; - } - - if (ctx->restricted && loops > 1) { - dev_warn(ctx->dev, - "Transfer too large; no branches permitted.\n"); - return -EINVAL; - } - - if (loops > 1) { - u64 cfg = SPI_FSI_COUNTER_CFG_LOOPS(loops - 1); - - fsi_spi_sequence_add(seq, SPI_FSI_SEQUENCE_BRANCH(idx)); - - if (transfer->rx_buf) - cfg |= SPI_FSI_COUNTER_CFG_N2_RX | - SPI_FSI_COUNTER_CFG_N2_TX | - SPI_FSI_COUNTER_CFG_N2_IMPLICIT | - SPI_FSI_COUNTER_CFG_N2_RELOAD; - - rc = fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, cfg); - if (rc) - return rc; - } else { - fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, 0ULL); - } - - if (rem) - fsi_spi_sequence_add(seq, rem); - - return 0; -} - static int fsi_spi_transfer_data(struct fsi_spi *ctx, struct spi_transfer *transfer) { int rc = 0; u64 status = 0ULL; - u64 cfg = 0ULL; if (transfer->tx_buf) { int nb; @@ -363,16 +294,6 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx, u64 in = 0ULL; u8 *rx = transfer->rx_buf; - rc = fsi_spi_read_reg(ctx, SPI_FSI_COUNTER_CFG, &cfg); - if (rc) - return rc; - - if (cfg & SPI_FSI_COUNTER_CFG_N2_IMPLICIT) { - rc = fsi_spi_write_reg(ctx, SPI_FSI_DATA_TX, 0); - if (rc) - return rc; - } - while (transfer->len > recv) { do { rc = fsi_spi_read_reg(ctx, SPI_FSI_STATUS, @@ -439,6 +360,10 @@ static int fsi_spi_transfer_init(struct fsi_spi *ctx) } } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE)); + rc = fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, 0ULL); + if (rc) + return rc; + rc = fsi_spi_read_reg(ctx, SPI_FSI_CLOCK_CFG, &clock_cfg); if (rc) return rc; @@ -459,6 +384,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, { int rc; u8 seq_slave = SPI_FSI_SEQUENCE_SEL_SLAVE(mesg->spi->chip_select + 1); + unsigned int len; struct spi_transfer *transfer; struct fsi_spi *ctx = spi_controller_get_devdata(ctlr); @@ -471,8 +397,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, struct spi_transfer *next = NULL; /* Sequencer must do shift out (tx) first. */ - if (!transfer->tx_buf || - transfer->len > (ctx->max_xfr_size + 8)) { + if (!transfer->tx_buf || transfer->len > SPI_FSI_MAX_TX_SIZE) { rc = -EINVAL; goto error; } @@ -486,9 +411,13 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, fsi_spi_sequence_init(&seq); fsi_spi_sequence_add(&seq, seq_slave); - rc = fsi_spi_sequence_transfer(ctx, &seq, transfer); - if (rc) - goto error; + len = transfer->len; + while (len > 8) { + fsi_spi_sequence_add(&seq, + SPI_FSI_SEQUENCE_SHIFT_OUT(8)); + len -= 8; + } + fsi_spi_sequence_add(&seq, SPI_FSI_SEQUENCE_SHIFT_OUT(len)); if (!list_is_last(&transfer->transfer_list, &mesg->transfers)) { @@ -496,7 +425,9 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, /* Sequencer can only do shift in (rx) after tx. */ if (next->rx_buf) { - if (next->len > ctx->max_xfr_size) { + u8 shift; + + if (next->len > SPI_FSI_MAX_RX_SIZE) { rc = -EINVAL; goto error; } @@ -504,10 +435,8 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, dev_dbg(ctx->dev, "Sequence rx of %d bytes.\n", next->len); - rc = fsi_spi_sequence_transfer(ctx, &seq, - next); - if (rc) - goto error; + shift = SPI_FSI_SEQUENCE_SHIFT_IN(next->len); + fsi_spi_sequence_add(&seq, shift); } else { next = NULL; } @@ -541,9 +470,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, static size_t fsi_spi_max_transfer_size(struct spi_device *spi) { - struct fsi_spi *ctx = spi_controller_get_devdata(spi->controller); - - return ctx->max_xfr_size; + return SPI_FSI_MAX_RX_SIZE; } static int fsi_spi_probe(struct device *dev) @@ -582,14 +509,6 @@ static int fsi_spi_probe(struct device *dev) ctx->fsi = fsi; ctx->base = base + SPI_FSI_BASE; - if (of_device_is_compatible(np, "ibm,fsi2spi-restricted")) { - ctx->restricted = true; - ctx->max_xfr_size = SPI_FSI_MAX_XFR_SIZE_RESTRICTED; - } else { - ctx->restricted = false; - ctx->max_xfr_size = SPI_FSI_MAX_XFR_SIZE; - } - rc = devm_spi_register_controller(dev, ctlr); if (rc) spi_controller_put(ctlr); -- 2.27.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 13:39 ` [PATCH 1/2] " Eddie James @ 2021-07-16 17:19 ` Mark Brown 2021-07-16 18:34 ` Eddie James 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2021-07-16 17:19 UTC (permalink / raw) To: Eddie James; +Cc: devicetree, openbmc, linux-kernel, linux-spi, robh+dt [-- Attachment #1: Type: text/plain, Size: 262 bytes --] 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. Which security changes where - somewhere else in Linux? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 17:19 ` Mark Brown @ 2021-07-16 18:34 ` Eddie James 2021-07-19 15:20 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: Eddie James @ 2021-07-16 18:34 UTC (permalink / raw) To: Mark Brown; +Cc: devicetree, openbmc, robh+dt, linux-kernel, linux-spi 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 18:34 ` Eddie James @ 2021-07-19 15:20 ` Mark Brown 2021-07-19 15:46 ` Eddie James 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2021-07-19 15:20 UTC (permalink / raw) To: Eddie James; +Cc: devicetree, openbmc, robh+dt, linux-kernel, linux-spi [-- Attachment #1: Type: text/plain, Size: 329 bytes --] 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? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-19 15:20 ` Mark Brown @ 2021-07-19 15:46 ` Eddie James 2021-07-20 13:04 ` David Laight 0 siblings, 1 reply; 14+ messages in thread From: Eddie James @ 2021-07-19 15:46 UTC (permalink / raw) To: Mark Brown; +Cc: devicetree, openbmc, robh+dt, linux-kernel, linux-spi 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-19 15:46 ` Eddie James @ 2021-07-20 13:04 ` David Laight 2021-07-20 13:13 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: David Laight @ 2021-07-20 13:04 UTC (permalink / raw) To: 'Eddie James', Mark Brown Cc: devicetree, openbmc, robh+dt, linux-kernel, linux-spi 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-20 13:04 ` David Laight @ 2021-07-20 13:13 ` Mark Brown 2021-07-20 17:32 ` David Laight 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2021-07-20 13:13 UTC (permalink / raw) To: David Laight Cc: devicetree, openbmc, 'Eddie James', linux-kernel, linux-spi, robh+dt [-- Attachment #1: Type: text/plain, Size: 748 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-20 13:13 ` Mark Brown @ 2021-07-20 17:32 ` David Laight 0 siblings, 0 replies; 14+ messages in thread From: David Laight @ 2021-07-20 17:32 UTC (permalink / raw) To: 'Mark Brown' Cc: devicetree, openbmc, 'Eddie James', linux-kernel, linux-spi, robh+dt 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] dt-bindings: fsi: Remove ibm, fsi2spi-restricted compatible 2021-07-16 13:39 [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes Eddie James 2021-07-16 13:39 ` [PATCH 1/2] " Eddie James @ 2021-07-16 13:39 ` Eddie James 2021-07-19 15:54 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm,fsi2spi-restricted compatible Mark Brown 2021-07-17 13:46 ` [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes David Laight 2021-07-19 18:00 ` Mark Brown 3 siblings, 1 reply; 14+ messages in thread From: Eddie James @ 2021-07-16 13:39 UTC (permalink / raw) To: linux-spi Cc: devicetree, openbmc, Eddie James, linux-kernel, robh+dt, broonie Remove this compatible string from the FSI SPI controller documentation, since the security restrictions have been universally applied to the controllers. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- Documentation/devicetree/bindings/fsi/ibm,fsi2spi.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/fsi/ibm,fsi2spi.yaml b/Documentation/devicetree/bindings/fsi/ibm,fsi2spi.yaml index e425278653f5..e2ca0b000471 100644 --- a/Documentation/devicetree/bindings/fsi/ibm,fsi2spi.yaml +++ b/Documentation/devicetree/bindings/fsi/ibm,fsi2spi.yaml @@ -19,7 +19,6 @@ properties: compatible: enum: - ibm,fsi2spi - - ibm,fsi2spi-restricted reg: items: -- 2.27.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: fsi: Remove ibm,fsi2spi-restricted compatible 2021-07-16 13:39 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm, fsi2spi-restricted compatible Eddie James @ 2021-07-19 15:54 ` Mark Brown 0 siblings, 0 replies; 14+ messages in thread From: Mark Brown @ 2021-07-19 15:54 UTC (permalink / raw) To: Eddie James; +Cc: devicetree, openbmc, linux-kernel, linux-spi, robh+dt [-- Attachment #1: Type: text/plain, Size: 553 bytes --] On Fri, Jul 16, 2021 at 08:39:15AM -0500, Eddie James wrote: > Remove this compatible string from the FSI SPI controller > documentation, since the security restrictions have been > universally applied to the controllers. Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 13:39 [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes Eddie James 2021-07-16 13:39 ` [PATCH 1/2] " Eddie James 2021-07-16 13:39 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm, fsi2spi-restricted compatible Eddie James @ 2021-07-17 13:46 ` David Laight 2021-07-19 15:57 ` Eddie James 2021-07-19 18:00 ` Mark Brown 3 siblings, 1 reply; 14+ messages in thread From: David Laight @ 2021-07-17 13:46 UTC (permalink / raw) To: 'Eddie James', linux-spi Cc: devicetree, openbmc, linux-kernel, robh+dt, broonie 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-17 13:46 ` [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes David Laight @ 2021-07-19 15:57 ` Eddie James 0 siblings, 0 replies; 14+ messages in thread From: Eddie James @ 2021-07-19 15:57 UTC (permalink / raw) To: David Laight, linux-spi Cc: devicetree, openbmc, linux-kernel, robh+dt, broonie 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) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes 2021-07-16 13:39 [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes Eddie James ` (2 preceding siblings ...) 2021-07-17 13:46 ` [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes David Laight @ 2021-07-19 18:00 ` Mark Brown 3 siblings, 0 replies; 14+ messages in thread From: Mark Brown @ 2021-07-19 18:00 UTC (permalink / raw) To: linux-spi, Eddie James Cc: devicetree, openbmc, linux-kernel, robh+dt, Mark Brown 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-07-20 17:32 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-16 13:39 [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes Eddie James 2021-07-16 13:39 ` [PATCH 1/2] " Eddie James 2021-07-16 17:19 ` Mark Brown 2021-07-16 18:34 ` Eddie James 2021-07-19 15:20 ` Mark Brown 2021-07-19 15:46 ` Eddie James 2021-07-20 13:04 ` David Laight 2021-07-20 13:13 ` Mark Brown 2021-07-20 17:32 ` David Laight 2021-07-16 13:39 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm, fsi2spi-restricted compatible Eddie James 2021-07-19 15:54 ` [PATCH 2/2] dt-bindings: fsi: Remove ibm,fsi2spi-restricted compatible Mark Brown 2021-07-17 13:46 ` [PATCH 0/2] spi: fsi: Reduce max transfer size to 8 bytes David Laight 2021-07-19 15:57 ` Eddie James 2021-07-19 18:00 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).