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: <CAMpxmJWRMQ_2cyav1V4HfZf7vuAG8ngQWHrQM4Sap4v_kLoRpw@mail.gmail.com>
Date:   Mon, 12 Apr 2021 20:29:09 +0200
From:   Bartosz Golaszewski <bgolaszewski@...libre.com>
To:     Claudiu Beznea <Claudiu.Beznea@...rochip.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

On Mon, Apr 12, 2021 at 9:42 AM <Claudiu.Beznea@...rochip.com> wrote:
>
> On 07.04.2021 21:37, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
> > <claudiu.beznea@...rochip.com> wrote:
> >>
> >> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
> >> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
> >> the driver to not do offset adjustments.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
> >> ---
> >>
> >> Hi Bartosz,
> >>
> >> For the previously available compatibles the offset adjustment is done
> >> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
> >> is used in setup with macb driver which is calling of_get_mac_address()
> >> to get the proper NVMEM cell in EEPROM where the MAC resides and read
> >> directly from there. We modeled the EEPROM and NVMEM cell in DT as
> >> follows:
> >>
> >> &i2cnode {
> >>         // ...
> >>         eeprom0: eeprom0@52 {
> >>                 compatible = "atmel,24mac02e4";

Can you point me to the datasheet for this model, google only directs
me to this very email.

>From the device tree it looks as if it was just a regular 24c02 EEPROM
with MAC hard-coded at 250-255 bytes, is that right?

Bartosz

> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>                 reg = <0x52>;
> >>                 pagesize = <16>;
> >>                 size = <256>;
> >>                 status = "okay";
> >>
> >>                 eeprom0_eui48: eui48@fa {
> >>                         reg = <0xfa 0x6>;
> >>                 };
> >>         };
> >> };
> >>
> >> &gmac {
> >>         // ...
> >>
> >>         nvmem-cells = <&eeprom0_eui48>;
> >>         nvmem-cell-names = "mac-address";
> >>
> >>         // ...
> >> };
> >>
> >>
> >> Let me know if some other approach needs to be taken into account in
> >> at24 driver for this to work.
> >>
> >> Thank you,
> >> Claudiu Beznea
> >>
> >
> > Hi Claudiu,
> >
> > First of all: any new compatibles need to go into the DT bindings document.
>
> Agree! I missed this.
>
> >
> >
> >>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
> >>  1 file changed, 40 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index 926408b41270..ae2fbcb5e83d 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
> >>  struct at24_chip_data {
> >>         u32 byte_len;
> >>         u8 flags;
> >> +       u8 adjoff;
> >>         void (*read_post)(unsigned int off, char *buf, size_t count);
> >>  };
> >>
> >> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> >> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
> >>         static const struct at24_chip_data _name = {                    \
> >> -               .byte_len = _len, .flags = _flags,                      \
> >> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
> >>         }
> >>
> >> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> >> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
> >>         static const struct at24_chip_data _name = {                    \
> >>                 .byte_len = _len, .flags = _flags,                      \
> >> +               .adjoff = _adjoff,                                      \
> >>                 .read_post = _read_post,                                \
> >>         }
> >>
> >> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
> >>  }
> >>
> >>  /* needs 8 addresses as A0-A2 are ignored */
> >> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
> >> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
> >>  /* old variants can't be handled with this generic entry! */
> >> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs01, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs02, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> >> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> >> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >>  /* spd is a 24c02 in memory DIMMs */
> >>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> >> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
> >> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
> >>  /* 24c02_vaio is a 24c02 on some Sony laptops */
> >>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
> >> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
> >> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
> >>         at24_read_post_vaio);
> >> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs04, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>  /* 24rf08 quirk is handled at i2c-core */
> >> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs08, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs16, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs32, 16,
> >> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs64, 16,
> >> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
> >>  /* identical to 24c08 ? */
> >> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
> >>
> >>  static const struct i2c_device_id at24_ids[] = {
> >>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
> >> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
> >>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
> >>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
> >>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
> >> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
> >>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> >> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
> >>         { "spd",        (kernel_ulong_t)&at24_data_spd },
> >>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
> >>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> >> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
> >>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
> >>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
> >>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
> >> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
> >>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
> >> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
> >>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
> >>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
> >>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
> >> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
> >>         at24->flags = flags;
> >>         at24->read_post = cdata->read_post;
> >>         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[0].client = client;
> >>         at24->client[0].regmap = regmap;
> >>
> >> --
> >> 2.25.1
> >>
> >
> > What is the problem you're trying to solve?
>
> I wanted to instantiate a NVMEM cell with proper offset and size via DT and
> make whatever Ethernet driver aware of this NVMEM cell via DT bindings, as
> bellow:
>
> &i2cnode {
>         // ...
>         eeprom0: eeprom0@52 {
>                 compatible = "atmel,24mac02e4";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0x52>;
>                 pagesize = <16>;
>                 size = <256>;
>                 status = "okay";
>
>                 eeprom0_eui48: eui48@fa {
>                         reg = <0xfa 0x6>;
>                 };
>         };
> };
>
> &gmac {
>         // ...
>
>         nvmem-cells = <&eeprom0_eui48>;
>         nvmem-cell-names = "mac-address";
>
>         // ...
> };
>
> By adding this new compatible and changing the driver to not adjust the
> NVMEM cell offset for it but make it use the one provided in DT I was
> thinking this may scale for any future EEPROM storing MAC addresses at any
> offsets (because these would be provided via DT and not adjusted by driver).
>
> I the current at24 driver I haven't managed to find a compatible in the
> driver that mach the offset 0xfa (used to store MAC address on case of
> 24AA025E48).
>
> > The MAC area is accessible
> > on a different device address. The variants with serial and MAC areas
> > are meant to be instantiated as devices separate from the main
> > writeable area and you can then create an nvmem cell that will take up
> > the whole MAC.
> >
> > Or does your model keep the MAC in the same block that's used for
> > writing? In which case just using a regular compatible that matches
> > the size and creating the nvmem cell at the right offset should be
> > enough, right?
>
> Meaning describing the NVMEM cell in DT with an offset such that the driver
> adjustment +/- that offset to lead to the offset described in EEPROM datasheet?
>
> >
> > Am I missing something?
>
>
>
> >
> > Bartosz
> >
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ