linux-kernel - RE: [PATCH v7 2/2] iio: imu: smi240: add driver

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:
 <AM8PR10MB4721D14E9A13F1DE04FA8B68CD612@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM>
Date: Tue, 17 Sep 2024 13:13:57 +0000
From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "lars@...afoo.de" <lars@...afoo.de>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "dima.fedrau@...il.com" <dima.fedrau@...il.com>,
	"marcelo.schmitt1@...il.com" <marcelo.schmitt1@...il.com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Lorenz
 Christian (ME-SE/EAD2)" <Christian.Lorenz3@...bosch.com>, "Frauendorf Ulrike
 (ME/PJ-SW3)" <Ulrike.Frauendorf@...bosch.com>, "Dolde Kai (ME-SE/PAE-A3)"
	<Kai.Dolde@...bosch.com>
Subject: RE: [PATCH v7 2/2] iio: imu: smi240: add driver

>> >Hi Shen,
>> >
>> >I suspect I led you astray.  regmap core seems unlikely to feed us
>> >little endian buffers on writes (they should be CPU endian I think)
>> >so there should be memcpy() for that not a get_unaligned_le16()
>> >
>> >> +
>> >> +static int smi240_regmap_spi_write(void *context, const void *data,
>> >> +				   size_t count)
>> >> +{
>> >> +	u8 reg_addr;
>> >> +	u16 reg_data;
>> >> +	u32 request;
>> >> +	struct spi_device *spi = context;
>> >> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
>> >> +	struct smi240_data *iio_priv_data = iio_priv(indio_dev);
>> >> +
>> >> +	if (count < 2)
>> >> +		return -EINVAL;
>> >> +
>> >> +	reg_addr = ((u8 *)data)[0];
>> >> +	reg_data = get_unaligned_le16(&((u8 *)data)[1]);
>> >
>> >Why is the regmap core giving us an le16?
>> >I probably sent you wrong way with this earlier :( memcpy probably
>> >the correct choice here.
>>
>> Yes, you are right. We shall use memcpy to keep the be CPU endian.  Just using
>memcpy may be not enough.
>>
>> Shall we also change regmap_config.val_format_endian  from
>REGMAP_ENDIAN_LITTLE to  REGMAP_ENDIAN_NATIVE ?
>>
>> This is to make sure that regmap_write passes the reg-value to
>smi240_regmap_spi_write without changing the CPU endian.
>>
>Hmm. I'd missed that control.  If the register data needs to be little endian then it is
>correct to leave that set as REGMAP_ENDIAN_LITTLE as then the regmap core will
>do the endian swap for you on Big endian systems.
>
>If I follow that bit of regmap correctly it will then have the data in the right order so
>the above still wants to just be a memcpy.
>
>As it stands, on a Big endian host, regmap will use the val_format_endian to decide
>to flip the bytes.  This code then flips them back again and the value written is big
>endian which is not what you intend!
>
>Easy way to check this will be to set it, on your little endian host, to
>REGMAP_BIG_ENDIAN and see what you get in the value.
>Then consider if you'd had get_unaligned_be16 then it would end up as little endian
>again.  This should mirror the current situation if this driver runs on a big endian host.
>
>Hope that confusing set of comments helps!
>
>Jonathan
>
Hi  Jonathan,

we check the regmap behavior with the following tests

1. host : little endian   val_format_endian ==REGMAP_BIG_ENDIAN     regmap_write(data->regmap, REG_ADDR, 0x12AB)  
    then  in smi240_regmap_spi_write(void *context, const void *data, size_t count)    we have data[0]== REG_ADDR, data[1]==0xAB , data[2]==0x12 

2. host : little endian   val_format_endian ==REGMAP_BIG_LITTLE     regmap_write(data->regmap, REG_ADDR, 0x12AB)  
    then  in smi240_regmap_spi_write(void *context, const void *data, size_t count)    we have data[0]== REG_ADDR, data[1]==0x12 , data[2]==0xAB

3. host : little endian   val_format_endian ==REGMAP_BIG_NATIVE     regmap_write(data->regmap, REG_ADDR, 0x12AB)  
    then  in smi240_regmap_spi_write(void *context, const void *data, size_t count)    we have data[0]== REG_ADDR, data[1]==0x12 , data[2]==0xAB

when regmap_write passes the reg-value to the underlaying spi-write function, it flips the bytes if  val_format_endian != cpu_endian

Since we prepare the request and the reg-value (as part of the request) with cpu_endian, we need to make sure that the cpu_endian keeps untouched until we pass the whole request to spi buffer using  "iio_priv_data->spi_buf = cpu_to_be32(request)"
Therefore we need to remove the change of cpu_endian during the request preparation. 

1. Instead get_unaligned_le16 now we use memcpy to take the reg-value without changing the cpu_endian.
2, we use REGMAP_BIG_NATIVE on val_format_endian to make sure when regmap_write passes the reg-value to the underlaying spi-write function the cpu_endian kept untouched.

This makes our driver be able to work properly on both little endian and big endian host. We tested the new changes on little endian host it works properly. Big endian host case is not tested yet, since today the big endian processors are almost dead.

The next version will looks like that.

static const struct regmap_config smi240_regmap_config = {
	.reg_bits = 8,
	.val_bits = 16,
	.val_format_endian = REGMAP_ENDIAN_NATIVE,

};

static int smi240_regmap_spi_write(void *context, const void *data,size_t count)
{
	u8 reg_addr;
	u16 reg_data;
	u32 request;
	struct spi_device *spi = context;
	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
	struct smi240_data *iio_priv_data = iio_priv(indio_dev);
	if (count < 2)
		return -EINVAL;

	reg_addr = ((u8 *)data)[0];
	memcpy(&reg_data, &((u8 *)data)[1], 2);

	request = FIELD_PREP(SMI240_BUS_ID_MASK, SMI240_BUS_ID);
	request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
	request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);

	iio_priv_data->spi_buf = cpu_to_be32(request);
	return spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
}

Is the new version now correct ?

Best regards

Jianping Shen