linux-kernel - Re: Page select register restrictions in regmap core

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]

Date: Tue, 18 Jun 2024 21:46:14 +0100
From: Mark Brown <broonie@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, Armin Wolf <W_Armin@....de>
Subject: Re: Page select register restrictions in regmap core

On Tue, Jun 18, 2024 at 12:33:40PM -0700, Guenter Roeck wrote:
> On 6/18/24 10:45, Mark Brown wrote:
> > On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:

> > > It turns out that at least some i801 controllers don't work with the
> > > access mechanism used by regmap, or maybe the spd5118 chips don't support
> > > I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
> > > auto-incrementing the register address and actually reset the address on byte
> > > reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
> > > from the first register). Since regmap doesn't have a means for me to say
> > > "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
> > > I may have to drop regmap support entirely anyway. That would be annoying,
> > > but right now I have no idea how to work around that problem.

> > You can set the use_single_read and use_single_write flags in the config
> > to ensure registers are accessed one at a time, that restriction is
> > moderately common.

> That doesn't help, unfortunately. Thinking about it, that is not really
> surprising. The failing write is to the page register, and that was
> a single write anyway.

Oh, that's interesting - I'm kind of surprised the wire protocols differ
but it's been a while since I looked.  We should probably add this to
the quirking in regmap-i2c.c, have it select one the 8 bit only smbus
versions for devices that need register at a time operation.  I'd not be
surprised if other devices have issues, and anyway if it makes a
difference to the wire protocol we should try to select something as
close as possible to what we're actually doing.

Something like the below perhaps (this probably needs to be converted to
a match table type thing at this point, there's the SMBUS_WORD_DATA case
too):

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index a905e955bbfc..499fcec00f2d 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -313,6 +313,11 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
 
 	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
 		bus = &regmap_i2c;
+	else if (config->val_bits == 8 && config->reg_bits == 8 &&
+		 config->use_single_read && config->use_single_write &&
+		 i2c_check_functionality(i2c->adapter,
+					 I2C_FUNC_SMBUS_BYTE_DATA))
+		bus = &regmap_smbus_byte;
 	else if (config->val_bits == 8 && config->reg_bits == 8 &&
 		 i2c_check_functionality(i2c->adapter,
 					 I2C_FUNC_SMBUS_I2C_BLOCK))
@@ -334,10 +339,6 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
 		default:		/* everything else is not supported */
 			break;
 		}
-	else if (config->val_bits == 8 && config->reg_bits == 8 &&
-		 i2c_check_functionality(i2c->adapter,
-					 I2C_FUNC_SMBUS_BYTE_DATA))
-		bus = &regmap_smbus_byte;
 
 	if (!bus)
 		return ERR_PTR(-ENOTSUPP);

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)