linux-kernel - Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

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: <e9878950-5751-475c-aa39-cb96ddec3911@microchip.com>
Date: Mon, 1 Jul 2024 12:17:13 +0000
From: <Andrei.Simion@...rochip.com>
To: <brgl@...ev.pl>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
	<Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
	<claudiu.beznea@...on.dev>, <arnd@...db.de>, <gregkh@...uxfoundation.org>,
	<linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<claudiu.beznea@...rochip.com>
Subject: Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for
 24AA025E{48, 64}

On 01.07.2024 14:16, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@...rochip.com> wrote:
>>
>> On 01.07.2024 11:46, Bartosz Golaszewski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@...rochip.com> wrote:
>>>>
>>>>>>
>>>>>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
>>>>>> So, indeed, it is an entanglement in logic.
>>>>>> To keep the implementation as it is:
>>>>>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>>>>>>
>>>>>> I think that is enough not to break the existing users. What are your thoughts?
>>>>>>
>>>>>
>>>>> Wait... is the adjoff field effectively a boolean? Why u8?
>>>>>
>>>>
>>>> struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
>>>> Yes, adjoff needs to be treated as a boolean. I will change it in the next version.
>>>>
>>>
>>> No, wait. Why can't you just do:
>>>
>>> AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY);
>>>
>>> and avoid this whole new macro variant entirely?
>>>
>>
>> just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY):
>> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
>> 00000000  ff ff ff ff ff ff                                 |......|
>> 00000006
>> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
>> 00000000  ff ff ff ff ff ff                                 |......|
>> 00000006
>>
>> with this patch (adjoff false and new macro)
>> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
>> 00000000  04 91 62 [the rest bytes]                                 |..b...|
>> 00000006
>> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
>> 00000000  04 91 62 [the rest bytes]                                 |..b..m|
>> 00000006
>> #
>>
> 
> Ok, but your goal is for at24_get_offset_adj() to return 0, isn't it?
> This is what line
> 
> at24->offset_adj = cdata->adjoff ? at24_get_offset_adj(flags, byte_len) : 0;
> 
> is effectively achieving. What's the difference between this patch and
> the solution I'm proposing? Isn't the offset_adj field 0 in both
> cases? Is there any other difference I'm not seeing?
> 
> Because I still think we can avoid all this churn.
> 

I've rechecked what you said and see the function implementation at24_get_offset_adj(flags, byte_len) and made a mistake.
I didn't see that you said only AT24_FLAG_READONLY. (Sorry for the wrong output)
Ok, if I put only AT24_FLAG_READONLY then at24_get_offset_adj(flags,  byte_len)  returns 0 -> I've got in both 'cells/eui48@fa\,0' the MAC address.

Best Regards,
Andrei

> Bart