linux-kernel - RE: [PATCH 1/3] hwmon: max31827: Add PEC support

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: 
 <PH0PR03MB677186C968350C7ABD0B3DA58996A@PH0PR03MB6771.namprd03.prod.outlook.com>
Date: Wed, 20 Dec 2023 01:49:21 +0000
From: "Matyas, Daniel" <Daniel.Matyas@...log.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH 1/3] hwmon: max31827: Add PEC support



> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Monday, December 18, 2023 9:01 PM
> To: Matyas, Daniel <Daniel.Matyas@...log.com>
> Cc: Jean Delvare <jdelvare@...e.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>; linux-
> hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-doc@...r.kernel.org
> Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support
> 
> [External]
> 
> On 12/18/23 09:59, Matyas, Daniel wrote:
> >
> >
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------------------------------------------------------------------
> > ----------
> > *Von:* Guenter Roeck <groeck7@...il.com> im Auftrag von Guenter
> Roeck
> > <linux@...ck-us.net>
> > *Gesendet:* Montag, Dezember 18, 2023 6:26:57 nachm.
> > *An:* Matyas, Daniel <Daniel.Matyas@...log.com>
> > *Cc:* Jean Delvare <jdelvare@...e.com>; Rob Herring
> > <robh+dt@...nel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> > <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>;
> > linux-hwmon@...r.kernel.org <linux-hwmon@...r.kernel.org>;
> > devicetree@...r.kernel.org <devicetree@...r.kernel.org>;
> > linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>;
> > linux-doc@...r.kernel.org <linux-doc@...r.kernel.org>
> > *Betreff:* Re: [PATCH 1/3] hwmon: max31827: Add PEC support
> >
> > [External]
> >
> > On 12/18/23 06:55, Matyas, Daniel wrote:
> > [ ... ]
> >>> On top of that, it is not clear why regmap can't be used in the first
> place.
> >>> It seems that the major change is that one needs to read the
> >>> configuration register after a write to see if there was a PEC
> >>> error. It is not immediately obvious why that additional read (if
> >>> indeed necessary) would require regmap support to be dropped.
> >>>
> >>
> >> I tried out writing and and reading with regmap, but it is not working
> properly. Even if I modify the client flag, I still receive only 2 bytes of data
> (a word). I should be receiving 2+1 bytes = data + CRC-8.
> >>
> >> With i2c_smbus reads and writes, when I set the flag, I receive the 2+1
> bytes, as expected.
> >>
> >
> > The SMBus code in drivers/i2c/i2c-core-smbus.c is supposed to check if
> > the received PEC is correct for SMBus transfers. Are you saying that
> > this doesn't work, or that regmap doesn't use SMBus functions to
> > communicate with the chip ?
> >
> > Thanks,
> > Guenter
> >
> >
> > I am 70% sure, that the regmap does not use SMBus functions.
> >
> 
> It should.
> 
> $ git grep smbus drivers/base/regmap/regmap-i2c.c
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_byte_reg_read(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       ret =
> i2c_smbus_read_byte_data(i2c, reg);
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_byte_reg_write(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       return
> i2c_smbus_write_byte_data(i2c, reg, val);
> drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus
> regmap_smbus_byte = {
> drivers/base/regmap/regmap-i2c.c:       .reg_write =
> regmap_smbus_byte_reg_write,
> drivers/base/regmap/regmap-i2c.c:       .reg_read =
> regmap_smbus_byte_reg_read,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_word_reg_read(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       ret =
> i2c_smbus_read_word_data(i2c, reg);
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_word_reg_write(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       return
> i2c_smbus_write_word_data(i2c, reg, val);
> drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus
> regmap_smbus_word = {
> drivers/base/regmap/regmap-i2c.c:       .reg_write =
> regmap_smbus_word_reg_write,
> drivers/base/regmap/regmap-i2c.c:       .reg_read =
> regmap_smbus_word_reg_read,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_word_read_swapped(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       ret =
> i2c_smbus_read_word_swapped(i2c, reg);
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_smbus_word_write_swapped(void *context, unsigned int reg,
> drivers/base/regmap/regmap-i2c.c:       return
> i2c_smbus_write_word_swapped(i2c, reg, val);
> drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus
> regmap_smbus_word_swapped = {
> drivers/base/regmap/regmap-i2c.c:       .reg_write =
> regmap_smbus_word_write_swapped,
> drivers/base/regmap/regmap-i2c.c:       .reg_read =
> regmap_smbus_word_read_swapped,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_i2c_smbus_i2c_write(void *context, const void *data,
> drivers/base/regmap/regmap-i2c.c:       return
> i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_i2c_smbus_i2c_read(void *context, const void *reg,
> drivers/base/regmap/regmap-i2c.c:       ret =
> i2c_smbus_read_i2c_block_data(i2c, ((u8 *)reg)[0], val_size, val);
> drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus
> regmap_i2c_smbus_i2c_block = {
> drivers/base/regmap/regmap-i2c.c:       .write =
> regmap_i2c_smbus_i2c_write,
> drivers/base/regmap/regmap-i2c.c:       .read =
> regmap_i2c_smbus_i2c_read,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_i2c_smbus_i2c_write_reg16(void *context, const void *data,
> drivers/base/regmap/regmap-i2c.c:       return
> i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count,
> drivers/base/regmap/regmap-i2c.c:static int
> regmap_i2c_smbus_i2c_read_reg16(void *context, const void *reg,
> drivers/base/regmap/regmap-i2c.c:       ret =
> i2c_smbus_write_byte_data(i2c, ((u16 *)reg)[0] & 0xff,
> drivers/base/regmap/regmap-i2c.c:               ret =
> i2c_smbus_read_byte(i2c);
> drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus
> regmap_i2c_smbus_i2c_block_reg16 = {
> drivers/base/regmap/regmap-i2c.c:       .write =
> regmap_i2c_smbus_i2c_write_reg16,
> drivers/base/regmap/regmap-i2c.c:       .read =
> regmap_i2c_smbus_i2c_read_reg16,
> drivers/base/regmap/regmap-i2c.c:               bus =
> &regmap_i2c_smbus_i2c_block;
> drivers/base/regmap/regmap-i2c.c:               bus =
> &regmap_i2c_smbus_i2c_block_reg16;
> drivers/base/regmap/regmap-i2c.c:                       bus =
> &regmap_smbus_word;
> drivers/base/regmap/regmap-i2c.c:                       bus =
> &regmap_smbus_word_swapped;
> drivers/base/regmap/regmap-i2c.c:               bus = &regmap_smbus_byte;
> 
> If that doesn't work for some reason, I'd rather figure out why instead of
> starting to drop regmap support.
> 
> Guenter

I tried to figure it out and this is what I came up with. The code snippet below is from drivers/base/regmap/regmap-i2c.c:

static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
					const struct regmap_config *config)
{
	const struct i2c_adapter_quirks *quirks;
	const struct regmap_bus *bus = NULL;
	struct regmap_bus *ret_bus;
	u16 max_read = 0, max_write = 0;

	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
		bus = &regmap_i2c;
	else if (config->val_bits == 8 && config->reg_bits == 8 &&
		 i2c_check_functionality(i2c->adapter,
					 I2C_FUNC_SMBUS_I2C_BLOCK))
		bus = &regmap_i2c_smbus_i2c_block;
	else if (config->val_bits == 8 && config->reg_bits == 16 &&
		i2c_check_functionality(i2c->adapter,
					I2C_FUNC_SMBUS_I2C_BLOCK))
		bus = &regmap_i2c_smbus_i2c_block_reg16;
	else if (config->val_bits == 16 && config->reg_bits == 8 &&
		 i2c_check_functionality(i2c->adapter,
					 I2C_FUNC_SMBUS_WORD_DATA))
		switch (regmap_get_val_endian(&i2c->dev, NULL, config)) {
		case REGMAP_ENDIAN_LITTLE:
			bus = &regmap_smbus_word;
			break;
		case REGMAP_ENDIAN_BIG:
			bus = &regmap_smbus_word_swapped;
			break;
		default:		/* everything else is not supported */
			break;
		}

This is executed when regmap is initialized. My adapter has the I2C_FUNC_I2C functionality (I use a raspberry pi 4), so it seems to me like regmap_i2c is loaded as the bus. This uses i2c_transfer internally to read and write.

For PEC I need regmap_smbus_word. This uses i2c_smbus_xfer internally. Unlike i2c_transfer, i2c_smbus_xfer can be used to send and receive PEC byte.

What should I do?

Kind regards,
Daniel