linux-kernel - Re: [PATCH] arch: arm: kirkwood: support nvmem mac address

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: <CAKxU2N9sBOnTGDhtxGVvyrSw=_WuJH29tgZXfmK-RD2RRY=ZSQ@mail.gmail.com>
Date: Mon, 30 Sep 2024 19:50:56 -0700
From: Rosen Penev <rosenp@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: linux-arm-kernel@...ts.infradead.org, gregory.clement@...tlin.com, 
	sebastian.hesselbarth@...il.com, linux@...linux.org.uk, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arch: arm: kirkwood: support nvmem mac address

On Mon, Sep 30, 2024 at 6:53 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Mon, Sep 30, 2024 at 02:59:34PM -0700, Rosen Penev wrote:
> > of_get_ethdev_address gets called too early for nvmem. If EPROBE_DEFER
> > gets called, skip so that the ethernet driver can adjust the MAC address
> > through nvmem.
>
> Is this from code analysis or do you have a board with real issues? Do
> we want to add a Fixed: so it gets back ported in stable?
Working with a guy that does. This device has not been upstreamed, so
not quite valid.
>
> >
> > Signed-off-by: Rosen Penev <rosenp@...il.com>
> > ---
> >  arch/arm/mach-mvebu/kirkwood.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
> > index 73b2a86d6489..da347f66900b 100644
> > --- a/arch/arm/mach-mvebu/kirkwood.c
> > +++ b/arch/arm/mach-mvebu/kirkwood.c
> > @@ -86,13 +86,18 @@ static void __init kirkwood_dt_eth_fixup(void)
> >               void __iomem *io;
> >               u8 *macaddr;
> >               u32 reg;
> > +             int err;
> >
> >               if (!pnp)
> >                       continue;
> >
> > -             /* skip disabled nodes or nodes with valid MAC address*/
> > -             if (!of_device_is_available(pnp) ||
> > -                 !of_get_mac_address(np, tmpmac))
> > +             /* skip disabled nodes */
> > +             if (!of_device_is_available(pnp))
> > +                     goto eth_fixup_skip;
> > +
> > +             /* skip nodes with valid MAC address*/
> > +             err = of_get_mac_address(np, tmpmac);
> > +             if (err == -EPROBE_DEFER || !err)
> >                       goto eth_fixup_skip;
>
> I'm wondering about ordering here. What exactly does EPROBE_DEFER
> mean? Does it mean we know there is a MAC address in nvmem, but the
> nvmem has not probed yet? Or can it mean, the nvmem has not probed
> yet, and maybe there is a MAC address in it, maybe not?
It only means NVMEM has not loaded. NVMEM loads after MTD, meaning quite late.
>
> In the maybe not case, we should still be trying to read the MAC from
> the hardware and storing it way safe for later use.
Not sure how to go about that. OTOH it's not very common to have
CONFIG_NVMEM where not needed.

Actually I have no idea what this whole function even does. For these
devices, uboot usually reads the ethaddr variable and passes it to the
kernel. Something like that can be handled entirely by nvmem.
>
>         Andrew