[PATCH] regmap: spi: Don't use spi_write_then_read()

[PATCH] regmap: spi: Don't use spi_write_then_read()

[Mark Brown]

Currently SPI reads are implemented using spi_write_then_read(). This is a
convenience API which as well as constructing a SPI message from parameters
basically the same as for a bytestream read operation also bounces things
into a memory buffer to allow callers to use stack or other non-DMAable
memory. Since regmap should already be ensuring that everything can be
DMAed further up the stack this copy is redundant so switch to using the
underlying spi_sync() API with the buffers provided by the core directly.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-spi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index 14b1d88997cbe406c8900f5032d768fc7484e99f..6a3dbc217e890d25a392fd3157f9ed89188afdaa 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -94,8 +94,15 @@ static int regmap_spi_read(void *context,
 {
 	struct device *dev = context;
 	struct spi_device *spi = to_spi_device(dev);
+	struct spi_message m;
+	struct spi_transfer t[2] = { { .tx_buf = reg, .len = reg_size, },
+				     { .rx_buf = val, .len = val_size, }, };
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t[0], &m);
+	spi_message_add_tail(&t[1], &m);
 
-	return spi_write_then_read(spi, reg, reg_size, val, val_size);
+	return spi_sync(spi, &m);
 }
 
 static const struct regmap_bus regmap_spi = {

---
base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
change-id: 20250320-regmap-spi-write-read-8b2c2aecd027

Best regards,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] regmap: spi: Don't use spi_write_then_read()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Thu, Mar 20, 2025 at 11:00:34PM +0000, Mark Brown wrote:
> Currently SPI reads are implemented using spi_write_then_read(). This is a
> convenience API which as well as constructing a SPI message from parameters
> basically the same as for a bytestream read operation also bounces things
> into a memory buffer to allow callers to use stack or other non-DMAable
> memory. Since regmap should already be ensuring that everything can be
> DMAed further up the stack this copy is redundant so switch to using the
> underlying spi_sync() API with the buffers provided by the core directly.

I think this actually needs to be part of a wider series, but pushing it
out there just now since there's the other regmap paths which also use
spi_sync() and will need any adjustments anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: spi: Don't use spi_write_then_read()

[Arnd Bergmann]

On Fri, Mar 21, 2025, at 00:08, Mark Brown wrote:
> On Thu, Mar 20, 2025 at 11:00:34PM +0000, Mark Brown wrote:
>> Currently SPI reads are implemented using spi_write_then_read(). This is a
>> convenience API which as well as constructing a SPI message from parameters
>> basically the same as for a bytestream read operation also bounces things
>> into a memory buffer to allow callers to use stack or other non-DMAable
>> memory. Since regmap should already be ensuring that everything can be
>> DMAed further up the stack this copy is redundant so switch to using the
>> underlying spi_sync() API with the buffers provided by the core directly.
>
> I think this actually needs to be part of a wider series, but pushing it
> out there just now since there's the other regmap paths which also use
> spi_sync() and will need any adjustments anyway.

I can see some code paths that are potentially broken by this
patch on its own, e.g.

struct sc16is7xx_one {  
        struct uart_port                port;
        struct regmap                   *regmap;
        struct mutex                    efr_lock; /* EFR registers access */
        struct kthread_work             tx_work;
        struct kthread_work             reg_work;
        struct kthread_delayed_work     ms_work;
        struct sc16is7xx_one_config     config;
        unsigned char                   buf[SC16IS7XX_FIFO_SIZE]; /* Rx buffer. */
        unsigned int                    old_mctrl;
        u8                              old_lcr; /* Value before EFR access. */
        bool                            irda_mode;
};                                                     
sc16is7xx_fifo_read(port, one->buf, rxlen)

By no longer going through the spi_write_then_read() bounce
buffer, this will do cache management operations on
sc16is7xx_one->buf, which is not cacheline aligned.

What particular operation is done depends on architecture,
but commonly the dma DMA_TO_DEVICE is a cache flush while
DMA_FROM_DEVICE is a cache invalidation, which can cause
data corruption on the members next to buf[].

If your plan is to have _regmap_raw_read() always do its
own buffering (at least for unaligned buffers on noncoherent
devices), it will still work.

     Arnd

Re: [PATCH] regmap: spi: Don't use spi_write_then_read()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Fri, Mar 21, 2025 at 12:27:01PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 21, 2025, at 00:08, Mark Brown wrote:

> > I think this actually needs to be part of a wider series, but pushing it
> > out there just now since there's the other regmap paths which also use
> > spi_sync() and will need any adjustments anyway.

> I can see some code paths that are potentially broken by this
> patch on its own, e.g.

Yeah, I was pretty sure there were some but hadn't actually gone and
found a specific example yet.

> If your plan is to have _regmap_raw_read() always do its
> own buffering (at least for unaligned buffers on noncoherent
> devices), it will still work.

Either that or having some library that all the buses use (IIRC
at least SoundWire can do DMA), yeah.  We also have I/O that regmap
generates itself as part of cache sync potentially, especially with
rbtree caches.

I remembered what the difference between writes and reads I was thinking
of was BTW - only reads use spi_write_then_read(), writes are already
using spi_sync() directly for gather writes (which are the ones most
likely to use DMA since they'll be for bigger blocks of data, non-gather
writes are usually single register) or otherwise spi_write() which
doesn't bother with a bounce buffer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]