The TL;DR summary is that the regmap_noinc_write spills over the data
that are correctly written to the HW also to the following registers in
the regcache. As a result, regcache then contains user-controlled
garbage which will be used later for bit updates on unrelated registers.

This patch is a "wrong" fix; a real fix would involve fixing regmap
and/or regcache, but that code has too many indirections for my little
mind.

I was investigating a regression that happened somewhere between 5.12.4
(plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
MAX14830 UART would work fine the first time, but when our application
opens the UART the second time it just wouldn't send anything over the
physical TX pin. With the help of a logical analyzer, I found out that
the kernel was sending value 0xcd to the MODE1 register, which on this
chip is a request to set the UART's TX pin to the Hi-Z mode and to
switch off RX completely. That's certainly not the intention of the
code, but that's what I was seeing on the physical SPI bus, and also in
the log when I instrumented the regmap layer.

It turned out that one of the *data* bytes which were sent over the UART
was 0xdd, and that this *data byte* somehow ended up in the regcache's
idea about the value within the MODE1 register. When the UART is opened
up the next time and max310x_startup updates a single unrelated bit in
MODE1, that code consults the regcache, notices the 0xdd data byte in
there, and ends up sending 0xcd over SPI.

Here's what dump_stack() shows:

 max310x spi1.2: regcache_write: reg 0x9 value 0xdd
 max310x spi1.2: PWNED
 CPU: 1 PID: 26 Comm: kworker/1:1 Not tainted 6.5.9-7-g9e090fe75fd8 #7
 Hardware name: Marvell Armada 380/385 (Device Tree)
 Workqueue: events max310x_tx_proc
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x40/0x4c
  dump_stack_lvl from regcache_write+0xc0/0xc4
  regcache_write from _regmap_raw_write_impl+0x178/0x828
  _regmap_raw_write_impl from _regmap_raw_write+0xb8/0x134
  _regmap_raw_write from regmap_noinc_write+0x130/0x178
  regmap_noinc_write from max310x_tx_proc+0xd4/0x1a4
  max310x_tx_proc from process_one_work+0x21c/0x4e4
  process_one_work from worker_thread+0x50/0x54c
  worker_thread from kthread+0xe0/0xfc
  kthread from ret_from_fork+0x14/0x28

Clearly, regmap_noinc_write of a register 0x00 (that's the TX FIFO on
this chip) has no business updating register 0x09, but that's what was
happening here. The regmap_config is already set up in a way that
register 0x00 is marked precious and volatile, so it has no business
going through the cache at all. Also, the documentation for
regmap_noinc_write suggests that this driver was using the regmap
infrastructure correctly, and that the real bug is somewhere in
regmap/regcache where a call to regmap_noinc_write end up updating an
unrelated register in regcache.

Until regmap/regcache is fixed, let's just use an adapted version of the
old code that bypasses regmap altogether, and just sends out an SPI
transaction.

This is related to my commit 3f42b142ea1171967e40e10e4b0241c0d6d28d41
("serial: max310x: fix IO data corruption in batched operations") which
introduced usage of regmap_noinc_write() to this driver. That commit is
a fixup of commit 285e76fc049c4d32c772eea9460a7ef28a193802 ("serial:
max310x: use regmap methods for SPI batch operations") which started
using regmap_raw_write(), which was however also a wrong function.

Fixes: 3f42b142ea11 ("serial: max310x: fix IO data corruption in batched operations")
Fixes: 285e76fc049c ("serial: max310x: use regmap methods for SPI batch operations")
Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
To: Mark Brown <broonie@kernel.org>
To: Cosmin Tanislav <cosmin.tanislav@analog.com>
To: linux-serial@vger.kernel.org
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/tty/serial/max310x.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index c44237470bee..79797b573723 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -663,16 +663,34 @@ static u32 max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
 
 static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
 {
-	struct max310x_one *one = to_max310x_port(port);
-
-	regmap_noinc_write(one->regmap, MAX310X_THR_REG, txbuf, len);
+	const u8 header = (port->iobase * 0x20 + MAX310X_THR_REG) | MAX310X_WRITE_BIT;
+	struct spi_transfer xfer[] = {
+		{
+			.tx_buf = &header,
+			.len = 1,
+		},
+		{
+			.tx_buf = txbuf,
+			.len = len,
+		},
+	};
+	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
 }
 
 static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
 {
-	struct max310x_one *one = to_max310x_port(port);
-
-	regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
+	const u8 header = port->iobase * 0x20 + MAX310X_RHR_REG;
+	struct spi_transfer xfer[] = {
+		{
+			.tx_buf = &header,
+			.len = 1,
+		},
+		{
+			.rx_buf = rxbuf,
+			.len = len,
+		},
+	};
+	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
 }
 
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
-- 
2.42.0

On Fri, 1 Dec 2023 15:51:51 +0100
Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> The TL;DR summary is that the regmap_noinc_write spills over the data
> that are correctly written to the HW also to the following registers in
> the regcache. As a result, regcache then contains user-controlled
> garbage which will be used later for bit updates on unrelated registers.
> 
> This patch is a "wrong" fix; a real fix would involve fixing regmap
> and/or regcache, but that code has too many indirections for my little
> mind.
> 
> I was investigating a regression that happened somewhere between 5.12.4
> (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
> MAX14830 UART would work fine the first time, but when our application
> opens the UART the second time it just wouldn't send anything over the
> physical TX pin. With the help of a logical analyzer, I found out that
> the kernel was sending value 0xcd to the MODE1 register, which on this
> chip is a request to set the UART's TX pin to the Hi-Z mode and to
> switch off RX completely. That's certainly not the intention of the
> code, but that's what I was seeing on the physical SPI bus, and also in
> the log when I instrumented the regmap layer.
> 
> It turned out that one of the *data* bytes which were sent over the UART
> was 0xdd, and that this *data byte* somehow ended up in the regcache's
> idea about the value within the MODE1 register. When the UART is opened
> up the next time and max310x_startup updates a single unrelated bit in
> MODE1, that code consults the regcache, notices the 0xdd data byte in
> there, and ends up sending 0xcd over SPI.
> 
> Here's what dump_stack() shows:
> 
>  max310x spi1.2: regcache_write: reg 0x9 value 0xdd
>  max310x spi1.2: PWNED
>  CPU: 1 PID: 26 Comm: kworker/1:1 Not tainted 6.5.9-7-g9e090fe75fd8 #7
>  Hardware name: Marvell Armada 380/385 (Device Tree)
>  Workqueue: events max310x_tx_proc
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x40/0x4c
>   dump_stack_lvl from regcache_write+0xc0/0xc4
>   regcache_write from _regmap_raw_write_impl+0x178/0x828
>   _regmap_raw_write_impl from _regmap_raw_write+0xb8/0x134
>   _regmap_raw_write from regmap_noinc_write+0x130/0x178
>   regmap_noinc_write from max310x_tx_proc+0xd4/0x1a4
>   max310x_tx_proc from process_one_work+0x21c/0x4e4
>   process_one_work from worker_thread+0x50/0x54c
>   worker_thread from kthread+0xe0/0xfc
>   kthread from ret_from_fork+0x14/0x28
> 
> Clearly, regmap_noinc_write of a register 0x00 (that's the TX FIFO on
> this chip) has no business updating register 0x09, but that's what was
> happening here. The regmap_config is already set up in a way that
> register 0x00 is marked precious and volatile, so it has no business
> going through the cache at all. Also, the documentation for
> regmap_noinc_write suggests that this driver was using the regmap
> infrastructure correctly, and that the real bug is somewhere in
> regmap/regcache where a call to regmap_noinc_write end up updating an
> unrelated register in regcache.

Hi Jan,
it is funny, as I am preparing to send a patch for the sc16is7xx driver
to convert FIFO R/W to use the _noinc_ versions of regmap functions,
inspired by your patch 3f42b142ea11 ("serial: max310x: fix IO data
corruption in batched operations").

I am testing on a custom board with two SC16IS752 in SPI mode.

Here is our current FIFO write code:

  regcache_cache_bypass(one->regmap, true);
  regmap_raw_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send);
  regcache_cache_bypass(one->regmap, false);

I am converting it to _noinc_ version to be able to remove the manual
(and ugly) cache control workaround to this:

  regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send);

SC16IS7XX_THR_REG is already in precious and volatile, and I also
have put it in the noinc list.

To confirm that this works ok, I have put debug traces in some regmap
functions, and escpecially a trace in regcache_write() to indicate if
regmap is caching or not the register.

Here is an example when writing 01234567890123456789 (20 bytes) to the
Tx FIFO:

sc16is7xx spi0.0: sc16is7xx_tx_proc(): entry
sc16is7xx spi0.0: sc16is7xx_handle_tx(): entry
sc16is7xx spi0.0: regcache_read() not caching volatile reg $08
spi0.0-port0: regmap_read:  [08] 40
sc16is7xx spi0.0: regcache_write() not caching volatile reg $08
sc16is7xx spi0.0: _regmap_raw_write_impl() reg     = $00
sc16is7xx spi0.0: _regmap_raw_write_impl() val_len = 20
sc16is7xx spi0.0: regcache_write() not caching volatile reg $00
spi0.0-port0: regmap_write: [00] 30 31 32 33 34 35 36 37 38 39...
spi0.0-port0: regmap_write: [00] 36 37 38 39
...

With this I have confirmed that regmap _noinc_ works as intended, with
regcache_write() indicating it is not caching the volatile register 00
(THR).

I hope this can help you with your investigation, let me know if I can
help more.

Hugo Villeneuve.


 
> Until regmap/regcache is fixed, let's just use an adapted version of the
> old code that bypasses regmap altogether, and just sends out an SPI
> transaction.
> 
> This is related to my commit 3f42b142ea1171967e40e10e4b0241c0d6d28d41
> ("serial: max310x: fix IO data corruption in batched operations") which
> introduced usage of regmap_noinc_write() to this driver. That commit is
> a fixup of commit 285e76fc049c4d32c772eea9460a7ef28a193802 ("serial:
> max310x: use regmap methods for SPI batch operations") which started
> using regmap_raw_write(), which was however also a wrong function.
> 
> Fixes: 3f42b142ea11 ("serial: max310x: fix IO data corruption in batched operations")
> Fixes: 285e76fc049c ("serial: max310x: use regmap methods for SPI batch operations")
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> To: Mark Brown <broonie@kernel.org>
> To: Cosmin Tanislav <cosmin.tanislav@analog.com>
> To: linux-serial@vger.kernel.org
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/tty/serial/max310x.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index c44237470bee..79797b573723 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -663,16 +663,34 @@ static u32 max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
>  
>  static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
>  {
> -	struct max310x_one *one = to_max310x_port(port);
> -
> -	regmap_noinc_write(one->regmap, MAX310X_THR_REG, txbuf, len);
> +	const u8 header = (port->iobase * 0x20 + MAX310X_THR_REG) | MAX310X_WRITE_BIT;
> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = &header,
> +			.len = 1,
> +		},
> +		{
> +			.tx_buf = txbuf,
> +			.len = len,
> +		},
> +	};
> +	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
>  }
>  
>  static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
>  {
> -	struct max310x_one *one = to_max310x_port(port);
> -
> -	regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
> +	const u8 header = port->iobase * 0x20 + MAX310X_RHR_REG;
> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = &header,
> +			.len = 1,
> +		},
> +		{
> +			.rx_buf = rxbuf,
> +			.len = len,
> +		},
> +	};
> +	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
>  }
>  
>  static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
> -- 
> 2.42.0
> 
> 
> 

On Fri, 1 Dec 2023 13:27:36 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Fri, 1 Dec 2023 15:51:51 +0100
> Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
> 
> > The TL;DR summary is that the regmap_noinc_write spills over the data
> > that are correctly written to the HW also to the following registers in
> > the regcache. As a result, regcache then contains user-controlled
> > garbage which will be used later for bit updates on unrelated registers.
> > 
> > This patch is a "wrong" fix; a real fix would involve fixing regmap
> > and/or regcache, but that code has too many indirections for my little
> > mind.
> > 
> > I was investigating a regression that happened somewhere between 5.12.4
> > (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
> > MAX14830 UART would work fine the first time, but when our application
> > opens the UART the second time it just wouldn't send anything over the
> > physical TX pin. With the help of a logical analyzer, I found out that
> > the kernel was sending value 0xcd to the MODE1 register, which on this
> > chip is a request to set the UART's TX pin to the Hi-Z mode and to
> > switch off RX completely. That's certainly not the intention of the
> > code, but that's what I was seeing on the physical SPI bus, and also in
> > the log when I instrumented the regmap layer.
> > 
> > It turned out that one of the *data* bytes which were sent over the UART
> > was 0xdd, and that this *data byte* somehow ended up in the regcache's
> > idea about the value within the MODE1 register. When the UART is opened
> > up the next time and max310x_startup updates a single unrelated bit in
> > MODE1, that code consults the regcache, notices the 0xdd data byte in
> > there, and ends up sending 0xcd over SPI.
> > 
> > Here's what dump_stack() shows:
> > 
> >  max310x spi1.2: regcache_write: reg 0x9 value 0xdd
> >  max310x spi1.2: PWNED
> >  CPU: 1 PID: 26 Comm: kworker/1:1 Not tainted 6.5.9-7-g9e090fe75fd8 #7
> >  Hardware name: Marvell Armada 380/385 (Device Tree)
> >  Workqueue: events max310x_tx_proc
> >   unwind_backtrace from show_stack+0x10/0x14
> >   show_stack from dump_stack_lvl+0x40/0x4c
> >   dump_stack_lvl from regcache_write+0xc0/0xc4
> >   regcache_write from _regmap_raw_write_impl+0x178/0x828
> >   _regmap_raw_write_impl from _regmap_raw_write+0xb8/0x134
> >   _regmap_raw_write from regmap_noinc_write+0x130/0x178
> >   regmap_noinc_write from max310x_tx_proc+0xd4/0x1a4
> >   max310x_tx_proc from process_one_work+0x21c/0x4e4
> >   process_one_work from worker_thread+0x50/0x54c
> >   worker_thread from kthread+0xe0/0xfc
> >   kthread from ret_from_fork+0x14/0x28
> > 
> > Clearly, regmap_noinc_write of a register 0x00 (that's the TX FIFO on
> > this chip) has no business updating register 0x09, but that's what was
> > happening here. The regmap_config is already set up in a way that
> > register 0x00 is marked precious and volatile, so it has no business
> > going through the cache at all. Also, the documentation for
> > regmap_noinc_write suggests that this driver was using the regmap
> > infrastructure correctly, and that the real bug is somewhere in
> > regmap/regcache where a call to regmap_noinc_write end up updating an
> > unrelated register in regcache.
> 
> Hi Jan,
> it is funny, as I am preparing to send a patch for the sc16is7xx driver
> to convert FIFO R/W to use the _noinc_ versions of regmap functions,
> inspired by your patch 3f42b142ea11 ("serial: max310x: fix IO data
> corruption in batched operations").
> 
> I am testing on a custom board with two SC16IS752 in SPI mode.

Hi,
I ran the tests on Greg's tty-next tree.

Hugo Villeneuve.


> Here is our current FIFO write code:
> 
>   regcache_cache_bypass(one->regmap, true);
>   regmap_raw_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send);
>   regcache_cache_bypass(one->regmap, false);
> 
> I am converting it to _noinc_ version to be able to remove the manual
> (and ugly) cache control workaround to this:
> 
>   regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send);
> 
> SC16IS7XX_THR_REG is already in precious and volatile, and I also
> have put it in the noinc list.
> 
> To confirm that this works ok, I have put debug traces in some regmap
> functions, and escpecially a trace in regcache_write() to indicate if
> regmap is caching or not the register.
> 
> Here is an example when writing 01234567890123456789 (20 bytes) to the
> Tx FIFO:
> 
> sc16is7xx spi0.0: sc16is7xx_tx_proc(): entry
> sc16is7xx spi0.0: sc16is7xx_handle_tx(): entry
> sc16is7xx spi0.0: regcache_read() not caching volatile reg $08
> spi0.0-port0: regmap_read:  [08] 40
> sc16is7xx spi0.0: regcache_write() not caching volatile reg $08
> sc16is7xx spi0.0: _regmap_raw_write_impl() reg     = $00
> sc16is7xx spi0.0: _regmap_raw_write_impl() val_len = 20
> sc16is7xx spi0.0: regcache_write() not caching volatile reg $00
> spi0.0-port0: regmap_write: [00] 30 31 32 33 34 35 36 37 38 39...
> spi0.0-port0: regmap_write: [00] 36 37 38 39
> ...
> 
> With this I have confirmed that regmap _noinc_ works as intended, with
> regcache_write() indicating it is not caching the volatile register 00
> (THR).
> 
> I hope this can help you with your investigation, let me know if I can
> help more.
> 
> Hugo Villeneuve.
> 
> 
>  
> > Until regmap/regcache is fixed, let's just use an adapted version of the
> > old code that bypasses regmap altogether, and just sends out an SPI
> > transaction.
> > 
> > This is related to my commit 3f42b142ea1171967e40e10e4b0241c0d6d28d41
> > ("serial: max310x: fix IO data corruption in batched operations") which
> > introduced usage of regmap_noinc_write() to this driver. That commit is
> > a fixup of commit 285e76fc049c4d32c772eea9460a7ef28a193802 ("serial:
> > max310x: use regmap methods for SPI batch operations") which started
> > using regmap_raw_write(), which was however also a wrong function.
> > 
> > Fixes: 3f42b142ea11 ("serial: max310x: fix IO data corruption in batched operations")
> > Fixes: 285e76fc049c ("serial: max310x: use regmap methods for SPI batch operations")
> > Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> > To: Mark Brown <broonie@kernel.org>
> > To: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > To: linux-serial@vger.kernel.org
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/tty/serial/max310x.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> > index c44237470bee..79797b573723 100644
> > --- a/drivers/tty/serial/max310x.c
> > +++ b/drivers/tty/serial/max310x.c
> > @@ -663,16 +663,34 @@ static u32 max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
> >  
> >  static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
> >  {
> > -	struct max310x_one *one = to_max310x_port(port);
> > -
> > -	regmap_noinc_write(one->regmap, MAX310X_THR_REG, txbuf, len);
> > +	const u8 header = (port->iobase * 0x20 + MAX310X_THR_REG) | MAX310X_WRITE_BIT;
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = &header,
> > +			.len = 1,
> > +		},
> > +		{
> > +			.tx_buf = txbuf,
> > +			.len = len,
> > +		},
> > +	};
> > +	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
> >  }
> >  
> >  static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
> >  {
> > -	struct max310x_one *one = to_max310x_port(port);
> > -
> > -	regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
> > +	const u8 header = port->iobase * 0x20 + MAX310X_RHR_REG;
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = &header,
> > +			.len = 1,
> > +		},
> > +		{
> > +			.rx_buf = rxbuf,
> > +			.len = len,
> > +		},
> > +	};
> > +	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
> >  }
> >  
> >  static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
> > -- 
> > 2.42.0
> > 
> > 
> > 
> 

On Fri, Dec 01, 2023 at 01:27:36PM -0500, Hugo Villeneuve wrote:

> it is funny, as I am preparing to send a patch for the sc16is7xx driver
> to convert FIFO R/W to use the _noinc_ versions of regmap functions,
> inspired by your patch 3f42b142ea11 ("serial: max310x: fix IO data
> corruption in batched operations").

If you're working on that driver it'd also be good to update the current
use of cache bypass for the enhanced features/interrupt identification
register (and anything else in there, that did seem to be the only one)
to use regmap ranges instead - that'd remove the need for the efr_lock
and be a much more sensible/idiomatic use of the regmap APIs.

On Fri, 1 Dec 2023 18:34:38 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Dec 01, 2023 at 01:27:36PM -0500, Hugo Villeneuve wrote:
> 
> > it is funny, as I am preparing to send a patch for the sc16is7xx driver
> > to convert FIFO R/W to use the _noinc_ versions of regmap functions,
> > inspired by your patch 3f42b142ea11 ("serial: max310x: fix IO data
> > corruption in batched operations").
> 
> If you're working on that driver it'd also be good to update the current
> use of cache bypass for the enhanced features/interrupt identification
> register (and anything else in there, that did seem to be the only one)
> to use regmap ranges instead - that'd remove the need for the efr_lock
> and be a much more sensible/idiomatic use of the regmap APIs.

Hi Mark,
after our discussion about regmap range, it seems that the
efr_lock will need to stay.

In fact, all of this helped me to uncover another case where an
additional lock would be needed.

Hugo Villeneuve

On Tue, Dec 05, 2023 at 10:52:46AM -0500, Hugo Villeneuve wrote:

> after our discussion about regmap range, it seems that the
> efr_lock will need to stay.

OK.

> In fact, all of this helped me to uncover another case where an
> additional lock would be needed.

Some progress at least!

On Fri, 1 Dec 2023 18:34:38 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Dec 01, 2023 at 01:27:36PM -0500, Hugo Villeneuve wrote:
> 
> > it is funny, as I am preparing to send a patch for the sc16is7xx driver
> > to convert FIFO R/W to use the _noinc_ versions of regmap functions,
> > inspired by your patch 3f42b142ea11 ("serial: max310x: fix IO data
> > corruption in batched operations").
> 
> If you're working on that driver it'd also be good to update the current
> use of cache bypass for the enhanced features/interrupt identification
> register (and anything else in there, that did seem to be the only one)
> to use regmap ranges instead - that'd remove the need for the efr_lock
> and be a much more sensible/idiomatic use of the regmap APIs.

Hi Mark,
agreed, and I have already removed all cache bypass code (after some
fix for volatile registers)...

I will also look to remove the efr_lock, altough it has more
implications since this ship has some registers that share a common
address, and selected by bits in other registers, and I think this
is why there is this efr_lock.

I need to run more tests to make sure everything is ok, but so far
so good, and I should be submitting some of these patches soon.

Hugo Villeneuve.

On Fri, Dec 01, 2023 at 04:38:46PM -0500, Hugo Villeneuve wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > If you're working on that driver it'd also be good to update the current
> > use of cache bypass for the enhanced features/interrupt identification
> > register (and anything else in there, that did seem to be the only one)
> > to use regmap ranges instead - that'd remove the need for the efr_lock
> > and be a much more sensible/idiomatic use of the regmap APIs.

> I will also look to remove the efr_lock, altough it has more
> implications since this ship has some registers that share a common
> address, and selected by bits in other registers, and I think this
> is why there is this efr_lock.

Right, the registers sharing a common address with the register selected
by bits in another register is what regmap ranges support - the less
creative use of this is banked blocks of registers with a selector
register which picks which page bank is in use, that's moderately common
especially for TI.

On Fri, 1 Dec 2023 21:41:48 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Dec 01, 2023 at 04:38:46PM -0500, Hugo Villeneuve wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > If you're working on that driver it'd also be good to update the current
> > > use of cache bypass for the enhanced features/interrupt identification
> > > register (and anything else in there, that did seem to be the only one)
> > > to use regmap ranges instead - that'd remove the need for the efr_lock
> > > and be a much more sensible/idiomatic use of the regmap APIs.
> 
> > I will also look to remove the efr_lock, altough it has more
> > implications since this ship has some registers that share a common
> > address, and selected by bits in other registers, and I think this
> > is why there is this efr_lock.
> 
> Right, the registers sharing a common address with the register selected
> by bits in another register is what regmap ranges support - the less
> creative use of this is banked blocks of registers with a selector
> register which picks which page bank is in use, that's moderately common
> especially for TI.

Hi Mark,
thanks for the info, I was not aware of that, and will look into it.

Hugo Villeneuve.

On Fri, 1 Dec 2023 17:16:44 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Fri, 1 Dec 2023 21:41:48 +0000
> Mark Brown <broonie@kernel.org> wrote:
> 
> > On Fri, Dec 01, 2023 at 04:38:46PM -0500, Hugo Villeneuve wrote:
> > > Mark Brown <broonie@kernel.org> wrote:
> > 
> > > > If you're working on that driver it'd also be good to update the current
> > > > use of cache bypass for the enhanced features/interrupt identification
> > > > register (and anything else in there, that did seem to be the only one)
> > > > to use regmap ranges instead - that'd remove the need for the efr_lock
> > > > and be a much more sensible/idiomatic use of the regmap APIs.
> > 
> > > I will also look to remove the efr_lock, altough it has more
> > > implications since this ship has some registers that share a common
> > > address, and selected by bits in other registers, and I think this
> > > is why there is this efr_lock.
> > 
> > Right, the registers sharing a common address with the register selected
> > by bits in another register is what regmap ranges support - the less
> > creative use of this is banked blocks of registers with a selector
> > register which picks which page bank is in use, that's moderately common
> > especially for TI.
> 
> Hi Mark,
> thanks for the info, I was not aware of that, and will look into it.
> 
> Hugo Villeneuve.

Hi Mark,
I am having a hard time finding documentation on how regmap ranges
work.

Do you have an example of a driver which is using regmap ranges like it
should be done in this driver, that is using the exact same address for
two or more registers? I found an example, but it doesn't seem
applicable to the sc16is7xx driver because the two registers do not
share a common address, for example they have addresses like 0x01 and
0x81, even though with the proper page selection, they finally map to
address 0x01.

Thank you,
Hugo Villeneuve

On Mon, Dec 04, 2023 at 11:29:05AM -0500, Hugo Villeneuve wrote:

> Do you have an example of a driver which is using regmap ranges like it
> should be done in this driver, that is using the exact same address for
> two or more registers? I found an example, but it doesn't seem
> applicable to the sc16is7xx driver because the two registers do not
> share a common address, for example they have addresses like 0x01 and
> 0x81, even though with the proper page selection, they finally map to
> address 0x01.

I don't understand what you mean here - you say that the addresses both
have addresses 0x1 and 0x81 but map to address 0x1.  What does the 0x81
refer to?  The comments in the driver seemed to indicate that there was
a single address which mapped to multiple underlying registers...

Searching for struct regmap_range_cfg should show a lot of users in
mainline.

On Mon, 4 Dec 2023 16:35:30 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 04, 2023 at 11:29:05AM -0500, Hugo Villeneuve wrote:
> 
> > Do you have an example of a driver which is using regmap ranges like it
> > should be done in this driver, that is using the exact same address for
> > two or more registers? I found an example, but it doesn't seem
> > applicable to the sc16is7xx driver because the two registers do not
> > share a common address, for example they have addresses like 0x01 and
> > 0x81, even though with the proper page selection, they finally map to
> > address 0x01.
> 
> I don't understand what you mean here - you say that the addresses both
> have addresses 0x1 and 0x81 but map to address 0x1.  What does the 0x81
> refer to?  The comments in the driver seemed to indicate that there was
> a single address which mapped to multiple underlying registers...

Hi,
I was referring to an example in da9063-i2c.c where they have
these two registers:

#define	DA9063_REG_STATUS_A		0x01
#define	DA9063_REG_SEQ			0x81

To access one or the other, you must select page 0 or 1 in page config
selection register at address 0x00. It makes sense to me for this case.

But for the sc16is7xx, for example you have these two
independent registers, sharing the exact same address:

#define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
#define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */

I am not sure if regmap range can be used with this configuration.
Assuming regmap range would be properly setup, when we call
regmap_read(regmap, SC16IS7XX_IIR_REG, &val), how does regmap would
know that we want to access SC16IS7XX_IIR_REG and not SC16IS7XX_FCR_REG?

> Searching for struct regmap_range_cfg should show a lot of users in
> mainline.

Yes, I am trying to find a good example but I must download and read the
datasheet for each one. If you could point to an IC/driver that uses
regmap_range similar to IC sc16is7xx, it would really help.

Thank you
Hugo Villeneuve

On Mon, Dec 04, 2023 at 12:01:51PM -0500, Hugo Villeneuve wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > I don't understand what you mean here - you say that the addresses both
> > have addresses 0x1 and 0x81 but map to address 0x1.  What does the 0x81
> > refer to?  The comments in the driver seemed to indicate that there was
> > a single address which mapped to multiple underlying registers...

> I was referring to an example in da9063-i2c.c where they have
> these two registers:

> #define	DA9063_REG_STATUS_A		0x01
> #define	DA9063_REG_SEQ			0x81

> To access one or the other, you must select page 0 or 1 in page config
> selection register at address 0x00. It makes sense to me for this case.

That appears to be a bit confused in that they've mapped the window
through which you view the paged registers on top of the physical
register map - I suppose that will work but more through luck than
design.  The window is the physical address range through which the
actual registers can be accessed, the range is the virtual register
numbers through which users access the regmap.  It'll make things
clearer if they don't overlap.

> But for the sc16is7xx, for example you have these two
> independent registers, sharing the exact same address:

> #define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
> #define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */

> I am not sure if regmap range can be used with this configuration.
> Assuming regmap range would be properly setup, when we call
> regmap_read(regmap, SC16IS7XX_IIR_REG, &val), how does regmap would
> know that we want to access SC16IS7XX_IIR_REG and not SC16IS7XX_FCR_REG?

This is the exact situation this feature is supposed to handle, your
window is address 2 and then you should pick some random non-physical
numbers to map the two registers to for access by users.  The core will
then do appropriate physical accesses transparently to manage the
window.  The whole point here is to assign new, virtual addresses to the
two registers you're trying to access.

> > Searching for struct regmap_range_cfg should show a lot of users in
> > mainline.

> Yes, I am trying to find a good example but I must download and read the
> datasheet for each one. If you could point to an IC/driver that uses
> regmap_range similar to IC sc16is7xx, it would really help.

Essentially all of them should be fine.  

On Mon, 4 Dec 2023 17:19:25 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 04, 2023 at 12:01:51PM -0500, Hugo Villeneuve wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > I don't understand what you mean here - you say that the addresses both
> > > have addresses 0x1 and 0x81 but map to address 0x1.  What does the 0x81
> > > refer to?  The comments in the driver seemed to indicate that there was
> > > a single address which mapped to multiple underlying registers...
> 
> > I was referring to an example in da9063-i2c.c where they have
> > these two registers:
> 
> > #define	DA9063_REG_STATUS_A		0x01
> > #define	DA9063_REG_SEQ			0x81
> 
> > To access one or the other, you must select page 0 or 1 in page config
> > selection register at address 0x00. It makes sense to me for this case.
> 
> That appears to be a bit confused in that they've mapped the window
> through which you view the paged registers on top of the physical
> register map - I suppose that will work but more through luck than
> design.  The window is the physical address range through which the
> actual registers can be accessed, the range is the virtual register
> numbers through which users access the regmap.  It'll make things
> clearer if they don't overlap.

Ok, that was probably not a good example, let's forget about it, for
me at least :)

I found a better example maybe with tas2770.c.

> > But for the sc16is7xx, for example you have these two
> > independent registers, sharing the exact same address:
> 
> > #define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
> > #define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */
> 
> > I am not sure if regmap range can be used with this configuration.
> > Assuming regmap range would be properly setup, when we call
> > regmap_read(regmap, SC16IS7XX_IIR_REG, &val), how does regmap would
> > know that we want to access SC16IS7XX_IIR_REG and not SC16IS7XX_FCR_REG?
> 
> This is the exact situation this feature is supposed to handle, your
> window is address 2 and then you should pick some random non-physical
> numbers to map the two registers to for access by users.  The core will
> then do appropriate physical accesses transparently to manage the
> window.  The whole point here is to assign new, virtual addresses to the
> two registers you're trying to access.

Ok, I see.

Based on tas2770.c, I am assuming I could redefine the code to look
like this:

/* SC16IS7XX register definitions */
#define SC16IS7XX_REG(page, reg)        ((page * 128) + reg)

#define SC16IS7XX_RHR_REG	SC16IS7XX_REG(0, 0x00) /* RX FIFO */
#define SC16IS7XX_THR_REG	SC16IS7XX_REG(0, 0x00) /* TX FIFO */
#define SC16IS7XX_IER_REG	SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
#define SC16IS7XX_IIR_REG	SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
#define SC16IS7XX_FCR_REG	SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
#define SC16IS7XX_MCR_REG	SC16IS7XX_REG(0, 0x04) /* Modem Control */
#define SC16IS7XX_LSR_REG	SC16IS7XX_REG(0, 0x05) /* Line Status */
#define SC16IS7XX_MSR_REG	SC16IS7XX_REG(0, 0x06) /* Modem Status */
#define SC16IS7XX_SPR_REG	SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
...
#define SC16IS7XX_EFR_REG	SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
#define SC16IS7XX_XON1_REG	SC16IS7XX_REG(1, 0x04) /* Xon1 word */
#define SC16IS7XX_XON2_REG	SC16IS7XX_REG(1, 0x05) /* Xon2 word */
#define SC16IS7XX_XOFF1_REG	SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
#define SC16IS7XX_XOFF2_REG	SC16IS7XX_REG(1, 0x07) /* Xoff2 word */
...

static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
	{
		.range_min = 0,
		.range_max = 1 * 128,
		.selector_reg = SC16IS7XX_LCR_REG,
		.selector_mask = 0xff,
		.selector_shift = 0,
		.window_start = 0,
		.window_len = 128,
	},
};

But here, selecting the proper "page" is not obvious,
because to select page 1, we need to write a fixed value of 0xBF to
the LCR register.

And when selecting page 0, we must write the previous value that was
in LCR _before_ we made the switch to page 1...

How do we do that?

Hugo.

 
> > > Searching for struct regmap_range_cfg should show a lot of users in
> > > mainline.
> 
> > Yes, I am trying to find a good example but I must download and read the
> > datasheet for each one. If you could point to an IC/driver that uses
> > regmap_range similar to IC sc16is7xx, it would really help.
> 
> Essentially all of them should be fine.

On Mon, Dec 04, 2023 at 01:59:22PM -0500, Hugo Villeneuve wrote:

> Based on tas2770.c, I am assuming I could redefine the code to look
> like this:

> /* SC16IS7XX register definitions */
> #define SC16IS7XX_REG(page, reg)        ((page * 128) + reg)
> 
> #define SC16IS7XX_RHR_REG	SC16IS7XX_REG(0, 0x00) /* RX FIFO */
> #define SC16IS7XX_THR_REG	SC16IS7XX_REG(0, 0x00) /* TX FIFO */
> #define SC16IS7XX_IER_REG	SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
> #define SC16IS7XX_IIR_REG	SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
> #define SC16IS7XX_FCR_REG	SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
> #define SC16IS7XX_MCR_REG	SC16IS7XX_REG(0, 0x04) /* Modem Control */
> #define SC16IS7XX_LSR_REG	SC16IS7XX_REG(0, 0x05) /* Line Status */
> #define SC16IS7XX_MSR_REG	SC16IS7XX_REG(0, 0x06) /* Modem Status */
> #define SC16IS7XX_SPR_REG	SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
> ...
> #define SC16IS7XX_EFR_REG	SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
> #define SC16IS7XX_XON1_REG	SC16IS7XX_REG(1, 0x04) /* Xon1 word */
> #define SC16IS7XX_XON2_REG	SC16IS7XX_REG(1, 0x05) /* Xon2 word */
> #define SC16IS7XX_XOFF1_REG	SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
> #define SC16IS7XX_XOFF2_REG	SC16IS7XX_REG(1, 0x07) /* Xoff2 word */

> static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
> 	{
> 		.range_min = 0,
> 		.range_max = 1 * 128,
> 		.selector_reg = SC16IS7XX_LCR_REG,
> 		.selector_mask = 0xff,
> 		.selector_shift = 0,
> 		.window_start = 0,
> 		.window_len = 128,
> 	},
> };

That looks plausible - I'd tend to make the range just completely
non-physical (eg, start at some unrepresentable value) so there's no
possible ambiguity with a physical address.  I'm also not clear why
you've made the window be 128 registers wide if it's just this range
from 2-7 that gets switched around, I'd do something more like

#define SC16IS7XX_RANGE_BASE 0x1000
#define SC16IS7XX_WINDOW_LEN 6
#define SC16IS7XX_PAGED_REG(page, reg) \
	(SC16IS7XX_RANGE_BASE + (SC16IS7XX_WINDOW_LEN * page) + reg)

	.range_min = SC16IS7XX_RANGE_BASE,
	.range_max = SC16IS7XX_RANGE_BASE + (2 * SC16IS7XX_WINDOW_LEN),
	.window_start = 2,
	.window_len = SC16IS7XX_WINDOW_LEN,

You could apply a - 2 offset to reg as well to make the defines for the
registers clearer.  The above means that any untranslated register you
see in I/O traces should be immediately obvious (and more likely to trip
up error handling and tell you about it if it goes wrong) and hopefully
also makes it easier to reason about what's going on.

> But here, selecting the proper "page" is not obvious,
> because to select page 1, we need to write a fixed value of 0xBF to
> the LCR register.

> And when selecting page 0, we must write the previous value that was
> in LCR _before_ we made the switch to page 1...

> How do we do that?

That's one reason why having the range cover all the registers gets
confusing.  That said the code does have special casing for a selector
register in the middle of the range since there were some devices that
just put the paging register in the middle of the range it controls for
some innovative region, the core will notice that this is a selector
register and not do any paging for that register.

On Mon, 4 Dec 2023 19:16:57 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 04, 2023 at 01:59:22PM -0500, Hugo Villeneuve wrote:
> 
> > Based on tas2770.c, I am assuming I could redefine the code to look
> > like this:
> 
> > /* SC16IS7XX register definitions */
> > #define SC16IS7XX_REG(page, reg)        ((page * 128) + reg)
> > 
> > #define SC16IS7XX_RHR_REG	SC16IS7XX_REG(0, 0x00) /* RX FIFO */
> > #define SC16IS7XX_THR_REG	SC16IS7XX_REG(0, 0x00) /* TX FIFO */
> > #define SC16IS7XX_IER_REG	SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
> > #define SC16IS7XX_IIR_REG	SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
> > #define SC16IS7XX_FCR_REG	SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
> > #define SC16IS7XX_MCR_REG	SC16IS7XX_REG(0, 0x04) /* Modem Control */
> > #define SC16IS7XX_LSR_REG	SC16IS7XX_REG(0, 0x05) /* Line Status */
> > #define SC16IS7XX_MSR_REG	SC16IS7XX_REG(0, 0x06) /* Modem Status */
> > #define SC16IS7XX_SPR_REG	SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
> > ...
> > #define SC16IS7XX_EFR_REG	SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
> > #define SC16IS7XX_XON1_REG	SC16IS7XX_REG(1, 0x04) /* Xon1 word */
> > #define SC16IS7XX_XON2_REG	SC16IS7XX_REG(1, 0x05) /* Xon2 word */
> > #define SC16IS7XX_XOFF1_REG	SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
> > #define SC16IS7XX_XOFF2_REG	SC16IS7XX_REG(1, 0x07) /* Xoff2 word */
> 
> > static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
> > 	{
> > 		.range_min = 0,
> > 		.range_max = 1 * 128,
> > 		.selector_reg = SC16IS7XX_LCR_REG,
> > 		.selector_mask = 0xff,
> > 		.selector_shift = 0,
> > 		.window_start = 0,
> > 		.window_len = 128,
> > 	},
> > };
> 
> That looks plausible - I'd tend to make the range just completely
> non-physical (eg, start at some unrepresentable value) so there's no
> possible ambiguity with a physical address.  I'm also not clear why
> you've made the window be 128 registers wide if it's just this range

I simply took what was in tas2770.c as a starting point.

> from 2-7 that gets switched around, I'd do something more like
> 
> #define SC16IS7XX_RANGE_BASE 0x1000
> #define SC16IS7XX_WINDOW_LEN 6
> #define SC16IS7XX_PAGED_REG(page, reg) \
> 	(SC16IS7XX_RANGE_BASE + (SC16IS7XX_WINDOW_LEN * page) + reg)
> 
> 	.range_min = SC16IS7XX_RANGE_BASE,
> 	.range_max = SC16IS7XX_RANGE_BASE + (2 * SC16IS7XX_WINDOW_LEN),
> 	.window_start = 2,
> 	.window_len = SC16IS7XX_WINDOW_LEN,
> 
> You could apply a - 2 offset to reg as well to make the defines for the
> registers clearer.  The above means that any untranslated register you
> see in I/O traces should be immediately obvious (and more likely to trip
> up error handling and tell you about it if it goes wrong) and hopefully
> also makes it easier to reason about what's going on.

Ok.

> > But here, selecting the proper "page" is not obvious,
> > because to select page 1, we need to write a fixed value of 0xBF to
> > the LCR register.
> 
> > And when selecting page 0, we must write the previous value that was
> > in LCR _before_ we made the switch to page 1...
> 
> > How do we do that?
> 
> That's one reason why having the range cover all the registers gets
> confusing.  That said the code does have special casing for a selector
> register in the middle of the range since there were some devices that
> just put the paging register in the middle of the range it controls for
> some innovative region, the core will notice that this is a selector
> register and not do any paging for that register.

You are talking about the selector being inside the range, and I
understand that because I have looked at _regmap_select_page()
comments and logic.

But that is not was my question was about. Here a pseudo code
example to select "page" 1:

1. save original value of LCR register.
2. write 0xBF to LCR register
3. access desired register in page 1
4. restore original LCR value saved in step 1

How do you do that with regmap range?

Hugo.

On Mon, Dec 04, 2023 at 02:41:36PM -0500, Hugo Villeneuve wrote:

> But that is not was my question was about. Here a pseudo code
> example to select "page" 1:

> 1. save original value of LCR register.
> 2. write 0xBF to LCR register
> 3. access desired register in page 1
> 4. restore original LCR value saved in step 1

> How do you do that with regmap range?

Are you saying that the selector has other, non-selector functions?
This is truly innovative hardware, generally the selector is just a
bitfield that you write paging values to.  You'd need to extend the core
so that it knows about this quirk, right now that's not possible and
we'll just leave the window pointing at whatever was last accessed.

On Mon, 4 Dec 2023 19:48:05 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 04, 2023 at 02:41:36PM -0500, Hugo Villeneuve wrote:
> 
> > But that is not was my question was about. Here a pseudo code
> > example to select "page" 1:
> 
> > 1. save original value of LCR register.
> > 2. write 0xBF to LCR register
> > 3. access desired register in page 1
> > 4. restore original LCR value saved in step 1
> 
> > How do you do that with regmap range?
> 
> Are you saying that the selector has other, non-selector functions?

Yes! There is no bit or bit range in that register that is used to
select a praticular set of registers or "page". It is only when
writing the special magic value of $BF that the IC switches to "page"
1. And if the value is NOT $BF, then it switches back to "page" 0.

When I told you if you could point to other IC/drivers that had the
same configuration, I tough you were aware of this. That explains some
of the confusion.

> This is truly innovative hardware,...

Well, I would not say innovative, but more "crappy" hardware design :)

> ...generally the selector is just a 
> bitfield that you write paging values to.

This is also what I am accustomed to normally.

>  You'd need to extend the core
> so that it knows about this quirk, right now that's not possible and
> we'll just leave the window pointing at whatever was last accessed.

Ok. I am not sure that adding support for it would make sense, since I
do not know of other ICs that could reuse this very specific and
particular method for switching "paged" registers.

Thank you,
Hugo Villeneuve

On Mon, Dec 04, 2023 at 03:02:24PM -0500, Hugo Villeneuve wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > This is truly innovative hardware,...

> Well, I would not say innovative, but more "crappy" hardware design :)

I didn't say it was *good* innovation.

> >  You'd need to extend the core
> > so that it knows about this quirk, right now that's not possible and
> > we'll just leave the window pointing at whatever was last accessed.

> Ok. I am not sure that adding support for it would make sense, since I
> do not know of other ICs that could reuse this very specific and
> particular method for switching "paged" registers.

Yeah, I'm drawing a blank there.  The thing that springs to mind is
optimisation with wanting to always be on a particular page for fast
interrupt handling or something but that feels rather thin.

On Fri, Dec 01, 2023 at 03:51:51PM +0100, Jan Kundrát wrote:

> The TL;DR summary is that the regmap_noinc_write spills over the data
> that are correctly written to the HW also to the following registers in
> the regcache. As a result, regcache then contains user-controlled
> garbage which will be used later for bit updates on unrelated registers.

> I was investigating a regression that happened somewhere between 5.12.4
> (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our

Can you reproduce this with current kernels?  That's not even an up to
date v6.5 - we're up to v6.5.13 now from the looks of things including
one upstream fix that looks potentially relevant.  The most direct thing
would be to write a kunit test demonstrating the issue with current
mainline.

If things are already fine with mainline then you'd need to talk to the
stable maintainers about what they've chosen to backport.  

On Fri, Dec 1, 2023 at 6:21 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Dec 01, 2023 at 03:51:51PM +0100, Jan Kundrát wrote:
>
> > The TL;DR summary is that the regmap_noinc_write spills over the data
> > that are correctly written to the HW also to the following registers in
> > the regcache. As a result, regcache then contains user-controlled
> > garbage which will be used later for bit updates on unrelated registers.
>
> > I was investigating a regression that happened somewhere between 5.12.4
> > (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
>
> Can you reproduce this with current kernels?  That's not even an up to
> date v6.5 - we're up to v6.5.13 now from the looks of things including
> one upstream fix that looks potentially relevant.

Indeed, the 984a4afdc87a ("regmap: prevent noinc writes from
clobbering cache") seems quite relevant.

-- 
With Best Regards,
Andy Shevchenko

On pátek 1. prosince 2023 18:02:32 CET, Andy Shevchenko wrote:
> On Fri, Dec 1, 2023 at 6:21 PM Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Dec 01, 2023 at 03:51:51PM +0100, Jan Kundrát wrote:
>> 
>>> The TL;DR summary is that the regmap_noinc_write spills over the data
>>> that are correctly written to the HW also to the following registers in
>>> the regcache. As a result, regcache then contains user-controlled
>>> garbage which will be used later for bit updates on unrelated registers.
>> 
>>> I was investigating a regression that happened somewhere between 5.12.4
>>> (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
>> 
>> Can you reproduce this with current kernels?  That's not even an up to
>> date v6.5 - we're up to v6.5.13 now from the looks of things including
>> one upstream fix that looks potentially relevant.
>
> Indeed, the 984a4afdc87a ("regmap: prevent noinc writes from
> clobbering cache") seems quite relevant.

Thank you, Andy, this is indeed the real fix.

Sorry that I missed it, Mark. I spent many days on this one over a longer 
period of time, so I haven't noticed that a fix was merged in the 
meanwhile. I was just very happy that I debugged something which looked 
like a real puzzle at the beginning :).

Cheers,
Jan

(I like your analysis and a workaround, however there are quite a few
style minors, see below)

On Fri, Dec 1, 2023 at 5:56 PM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
>
> The TL;DR summary is that the regmap_noinc_write spills over the data

regmap_noinc_write()
(we refer to function as func() in the text and code comments)

> that are correctly written to the HW also to the following registers in
> the regcache. As a result, regcache then contains user-controlled
> garbage which will be used later for bit updates on unrelated registers.
>
> This patch is a "wrong" fix; a real fix would involve fixing regmap
> and/or regcache, but that code has too many indirections for my little
> mind.
>
> I was investigating a regression that happened somewhere between 5.12.4

v5.12.4
(you even used v later)

> (plus 14 of our patches) and v6.5.9 (plus 7 of our patches). Our
> MAX14830 UART would work fine the first time, but when our application
> opens the UART the second time it just wouldn't send anything over the
> physical TX pin. With the help of a logical analyzer, I found out that
> the kernel was sending value 0xcd to the MODE1 register, which on this
> chip is a request to set the UART's TX pin to the Hi-Z mode and to
> switch off RX completely. That's certainly not the intention of the
> code, but that's what I was seeing on the physical SPI bus, and also in
> the log when I instrumented the regmap layer.
>
> It turned out that one of the *data* bytes which were sent over the UART
> was 0xdd, and that this *data byte* somehow ended up in the regcache's
> idea about the value within the MODE1 register. When the UART is opened
> up the next time and max310x_startup updates a single unrelated bit in
> MODE1, that code consults the regcache, notices the 0xdd data byte in
> there, and ends up sending 0xcd over SPI.
>
> Here's what dump_stack() shows:

According to the Submitting Patches, this can be reduced to show only
important lines.

>  max310x spi1.2: regcache_write: reg 0x9 value 0xdd
>  max310x spi1.2: PWNED

>  CPU: 1 PID: 26 Comm: kworker/1:1 Not tainted 6.5.9-7-g9e090fe75fd8 #7

This is not important...

>  Hardware name: Marvell Armada 380/385 (Device Tree)
>  Workqueue: events max310x_tx_proc

>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x40/0x4c
>   dump_stack_lvl from regcache_write+0xc0/0xc4

...nor these...

>   regcache_write from _regmap_raw_write_impl+0x178/0x828
>   _regmap_raw_write_impl from _regmap_raw_write+0xb8/0x134
>   _regmap_raw_write from regmap_noinc_write+0x130/0x178
>   regmap_noinc_write from max310x_tx_proc+0xd4/0x1a4
>   max310x_tx_proc from process_one_work+0x21c/0x4e4

>   process_one_work from worker_thread+0x50/0x54c
>   worker_thread from kthread+0xe0/0xfc
>   kthread from ret_from_fork+0x14/0x28

...neither of these.

> Clearly, regmap_noinc_write of a register 0x00 (that's the TX FIFO on

()

> this chip) has no business updating register 0x09, but that's what was
> happening here. The regmap_config is already set up in a way that
> register 0x00 is marked precious and volatile, so it has no business
> going through the cache at all. Also, the documentation for
> regmap_noinc_write suggests that this driver was using the regmap

()

> infrastructure correctly, and that the real bug is somewhere in
> regmap/regcache where a call to regmap_noinc_write end up updating an

()
ends

> unrelated register in regcache.
>
> Until regmap/regcache is fixed, let's just use an adapted version of the
> old code that bypasses regmap altogether, and just sends out an SPI
> transaction.
>
> This is related to my commit 3f42b142ea1171967e40e10e4b0241c0d6d28d41
> ("serial: max310x: fix IO data corruption in batched operations") which

12 characters of the SHA-1 is enough (for now).

> introduced usage of regmap_noinc_write() to this driver. That commit is
> a fixup of commit 285e76fc049c4d32c772eea9460a7ef28a193802 ("serial:
> max310x: use regmap methods for SPI batch operations") which started

Ditto.

> using regmap_raw_write(), which was however also a wrong function.

Oh, cool! Here you used () :-)

> Fixes: 3f42b142ea11 ("serial: max310x: fix IO data corruption in batched operations")
> Fixes: 285e76fc049c ("serial: max310x: use regmap methods for SPI batch operations")
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>

> To: Mark Brown <broonie@kernel.org>
> To: Cosmin Tanislav <cosmin.tanislav@analog.com>
> To: linux-serial@vger.kernel.org
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: linux-kernel@vger.kernel.org

"To:" lines are so unusual.

I have kinda "smart" script [1] that has a heuristics that works in
99.9+% cases well. Consider using it or taking an idea from it.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

...

>  static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
>  {
> -       struct max310x_one *one = to_max310x_port(port);
> -
> -       regmap_noinc_write(one->regmap, MAX310X_THR_REG, txbuf, len);
> +       const u8 header = (port->iobase * 0x20 + MAX310X_THR_REG) | MAX310X_WRITE_BIT;
> +       struct spi_transfer xfer[] = {
> +               {
> +                       .tx_buf = &header,
> +                       .len = 1,
> +               },
> +               {
> +                       .tx_buf = txbuf,
> +                       .len = len,
> +               },
> +       };
> +       spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));

Can you add a comment like FIXME explaining why regmap is not used
here, otherwise I will expect some clever guy to "fix" this again.

>  }
>
>  static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
>  {
> -       struct max310x_one *one = to_max310x_port(port);
> -
> -       regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
> +       const u8 header = port->iobase * 0x20 + MAX310X_RHR_REG;
> +       struct spi_transfer xfer[] = {
> +               {
> +                       .tx_buf = &header,
> +                       .len = 1,
> +               },
> +               {
> +                       .rx_buf = rxbuf,
> +                       .len = len,
> +               },
> +       };
> +       spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));

Ditto.

>  }

-- 
With Best Regards,
Andy Shevchenko

On Fri, Dec 01, 2023 at 06:14:02PM +0200, Andy Shevchenko wrote:

> > +       };
> > +       spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));

> Can you add a comment like FIXME explaining why regmap is not used
> here, otherwise I will expect some clever guy to "fix" this again.

Or just do the fix in the right place rather than throwing a bodge
about...