linux-kernel - Re: [PATCH] tty: max310x: work around regmap->regcache data corruption

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: <20231204135922.0355f030945920086d21b8b6@hugovil.com>
Date:   Mon, 4 Dec 2023 13:59:22 -0500
From:   Hugo Villeneuve <hugo@...ovil.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Jan Kundrát <jan.kundrat@...net.cz>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>,
        linux-serial@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: max310x: work around regmap->regcache data
 corruption

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

> On Mon, Dec 04, 2023 at 12:01:51PM -0500, Hugo Villeneuve wrote:
> > Mark Brown <broonie@...nel.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.