linux-kernel - Re: [PATCH] regmap: spi: Don't use spi_write_then

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <31f54b9e-6140-4a21-bfb3-ce07febe4e36@app.fastmail.com>
Date: Fri, 21 Mar 2025 12:27:01 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Mark Brown" <broonie@...nel.org>, "Petr Tesarik" <ptesarik@...e.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regmap: spi: Don't use spi_write_then_read()

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