| 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
| ||
|
Date: Fri, 28 Jun 2024 16:46:33 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andrei.Simion@...rochip.com
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 Fri, Jun 28, 2024 at 4:17 PM <Andrei.Simion@...rochip.com> wrote:
>
> On 28.06.2024 11:30, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
> > <andrei.simion@...rochip.com> wrote:
> >>
> >> From: Claudiu Beznea <claudiu.beznea@...rochip.com>
> >>
> >> The EEPROMs could be used only for MAC storage. In this case the
> >> EEPROM areas where MACs resides could be modeled as NVMEM cells
> >> (directly via DT bindings) such that the already available networking
> >> infrastructure to read properly the MAC addresses (via
> >> of_get_mac_address()). The previously available compatibles needs the
> >> offset adjustment probably for compatibility w/ old DT bindings.
> >> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
> >> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
> >> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
> >> [andrei.simion@...rochip.com: Add extended macros to initialize the structure
> >> with explicit value to adjusting offset. Add extra description for the commit
> >> message.]
> >> Signed-off-by: Andrei Simion <andrei.simion@...rochip.com>
> >> ---
> >> v2 -> v3:
> >> - add specific compatible names according with
> >> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
> >> - add extended macros to initialize the structure with explicit value for adjoff
> >> - drop co-developed-by to maintain the commit history
> >> (chronological order of modifications)
> >>
> >> v1 -> v2:
> >> - no change
> >> ---
> >> drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
> >> 1 file changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index 4bd4f32bcdab..e2ac08f656cf 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -121,20 +121,29 @@ struct at24_chip_data {
> >> u32 byte_len;
> >> u8 flags;
> >> u8 bank_addr_shift;
> >> + u8 adjoff;
> >> void (*read_post)(unsigned int off, char *buf, size_t count);
> >> };
> >>
> >> -#define AT24_CHIP_DATA(_name, _len, _flags) \
> >> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao) \
> >
> > Please, don't try to save space on a few letters, call it
> > AT24_CHIP_DATA_ADJOFF() for better readability.
> >
>
> I will change in next the version.
>
> >> static const struct at24_chip_data _name = { \
> >> .byte_len = _len, .flags = _flags, \
> >> + .adjoff = _ao \
> >> }
> >>
> >> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post) \
> >> +#define AT24_CHIP_DATA(_name, _len, _flags) \
> >> + AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
> >> +
> >> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post) \
> >> static const struct at24_chip_data _name = { \
> >> .byte_len = _len, .flags = _flags, \
> >> + .adjoff = _ao, \
> >> .read_post = _read_post, \
> >> }
> >>
> >> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post) \
> >> + AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
> >> +
> >> #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift) \
> >> static const struct at24_chip_data _name = { \
> >> .byte_len = _len, .flags = _flags, \
> >> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
> >> AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> >> AT24_CHIP_DATA(at24_data_24cs02, 16,
> >> AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> >> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
> >> + AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >
> > And this will not break existing users? I guess you refer to these
> > changes in your commit message but it's not very clear what you're
> > doing and why.
> >
>
> 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?
Bart
> Best Regards,
> Andrei Simion
>
> >> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
> >> + AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
> >> AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> >> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
> >> AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> /* spd is a 24c02 in memory DIMMs */
> >> AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> >> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
> >> { "24cs02", (kernel_ulong_t)&at24_data_24cs02 },
> >> { "24mac402", (kernel_ulong_t)&at24_data_24mac402 },
> >> { "24mac602", (kernel_ulong_t)&at24_data_24mac602 },
> >> + { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
> >> + { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
> >> { "spd", (kernel_ulong_t)&at24_data_spd },
> >> { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
> >> { "24c04", (kernel_ulong_t)&at24_data_24c04 },
> >> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
> >> { .compatible = "atmel,24c1024", .data = &at24_data_24c1024 },
> >> { .compatible = "atmel,24c1025", .data = &at24_data_24c1025 },
> >> { .compatible = "atmel,24c2048", .data = &at24_data_24c2048 },
> >> + { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
> >> + { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
> >> { /* END OF LIST */ },
> >> };
> >> MODULE_DEVICE_TABLE(of, at24_of_match);
> >> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
> >> at24->read_post = cdata->read_post;
> >> at24->bank_addr_shift = cdata->bank_addr_shift;
> >> at24->num_addresses = num_addresses;
> >> - at24->offset_adj = at24_get_offset_adj(flags, byte_len);
> >> + at24->offset_adj = cdata->adjoff ?
> >> + at24_get_offset_adj(flags, byte_len) : 0;
> >> at24->client_regmaps[0] = regmap;
> >>
> >> at24->vcc_reg = devm_regulator_get(dev, "vcc");
> >> --
> >> 2.34.1
> >>
> >
> > Bart
Powered by blists - more mailing lists