linux-kernel - Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

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: <1934f597-9133-4784-9a41-8808a1253376@roeck-us.net>
Date: Mon, 1 Jul 2024 10:30:19 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

On 7/1/24 09:54, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 3:47 PM, Guenter Roeck wrote:
>> On 7/1/24 06:01, Quentin Schulz wrote:
>>
>>>> -#define AMC6821_CONF1_FDRC1        BIT(7)
>>>> +#define AMC6821_CONF1_FDRC1        BIT(6)
>>>
>>> This should have been squashed with a previous commit.
>>>
>>
>> Yes. I had found the bug but then fixed it in the wrong patch of the series.
>>
>> ...
>>>>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>>>>                char *buf)
>>>>   {
>>>> -    struct amc6821_data *data = amc6821_update_device(dev);
>>>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>>>       int ix = to_sensor_dev_attr(devattr)->index;
>>>> +    u32 regval;
>>>
>>> Why not a u8 directly here? We know single reads are going to return a u8 so no need to store more?
>>>
>>
>> The parameter of regmap_read is a pointer to unsigned int.
>>
> 
> Eventually through the many indirections, because our regmap_config sets the size of values in registers to 8b, it's a u8. But it's not worth our time to optimize this.
> 
> EDIT: coming back to this after reading the rest... Wouldn't we have a possible endianness issue here? Especially with the int8_t cast or the sign_extend32 (wouldn't the sign bit be at a different index on LE/BE ?). Sorry for the question, endianness really isn't my cup of tea.
> 

The underlying code does an implicit conversion. For example,  see
regmap_smbus_byte_reg_read(). There is no endianness issue because
the returned data is always in host endianness order. Sure, that
could be big endian, but the sign bit is still in bit 7.

Thanks,
Guenter