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

["Arnd Bergmann" <arnd@arndb.de>]

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