[v1] net: gianfar: fix NVMEM mac address

[PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Rosen Penev 2 months, 3 weeks ago

If nvmem loads after the ethernet driver, mac address assignments will
not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
case so we need to handle that to avoid eth_hw_addr_random.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 634049c83ebe..9755ec947029 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 		priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
 
 	err = of_get_ethdev_address(np, dev);
+	if (err == -EPROBE_DEFER)
+		return err;
 	if (err) {
 		eth_hw_addr_random(dev);
 		dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
-- 
2.46.0

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Simon Horman 2 months, 3 weeks ago

On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> If nvmem loads after the ethernet driver, mac address assignments will
> not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> case so we need to handle that to avoid eth_hw_addr_random.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 634049c83ebe..9755ec947029 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>  		priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
>  
>  	err = of_get_ethdev_address(np, dev);
> +	if (err == -EPROBE_DEFER)
> +		return err;

To avoid leaking resources, I think this should be:

		goto err_grp_init;

Flagged by Smatch.

>  	if (err) {
>  		eth_hw_addr_random(dev);
>  		dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);

-- 
pw-bot: cr

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Rosen Penev 2 months, 3 weeks ago

On Mon, Sep 9, 2024 at 1:55 AM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> > If nvmem loads after the ethernet driver, mac address assignments will
> > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > case so we need to handle that to avoid eth_hw_addr_random.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 634049c83ebe..9755ec947029 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >               priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> >
> >       err = of_get_ethdev_address(np, dev);
> > +     if (err == -EPROBE_DEFER)
> > +             return err;
>
> To avoid leaking resources, I think this should be:
>
>                 goto err_grp_init;
will do in v2. Unfortunately net-next closes today AFAIK.
>
> Flagged by Smatch.
>
> >       if (err) {
> >               eth_hw_addr_random(dev);
> >               dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
>
> --
> pw-bot: cr

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Rosen Penev 2 months, 3 weeks ago

On Mon, Sep 9, 2024 at 11:11 AM Rosen Penev <rosenp@gmail.com> wrote:
>
> On Mon, Sep 9, 2024 at 1:55 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> > > If nvmem loads after the ethernet driver, mac address assignments will
> > > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > > case so we need to handle that to avoid eth_hw_addr_random.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > > index 634049c83ebe..9755ec947029 100644
> > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > >               priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> > >
> > >       err = of_get_ethdev_address(np, dev);
> > > +     if (err == -EPROBE_DEFER)
> > > +             return err;
> >
> > To avoid leaking resources, I think this should be:
> >
> >                 goto err_grp_init;
> will do in v2. Unfortunately net-next closes today AFAIK.
On second thought, where did you find this?

git grep err_grp_init

returns nothing.

Not only that, this function has no goto.
> >
> > Flagged by Smatch.
> >
> > >       if (err) {
> > >               eth_hw_addr_random(dev);
> > >               dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> >
> > --
> > pw-bot: cr

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Simon Horman 2 months, 3 weeks ago

On Mon, Sep 09, 2024 at 11:20:20AM -0700, Rosen Penev wrote:
> On Mon, Sep 9, 2024 at 11:11 AM Rosen Penev <rosenp@gmail.com> wrote:
> >
> > On Mon, Sep 9, 2024 at 1:55 AM Simon Horman <horms@kernel.org> wrote:
> > >
> > > On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> > > > If nvmem loads after the ethernet driver, mac address assignments will
> > > > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > > > case so we need to handle that to avoid eth_hw_addr_random.
> > > >
> > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > ---
> > > >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > > > index 634049c83ebe..9755ec947029 100644
> > > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > > >               priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> > > >
> > > >       err = of_get_ethdev_address(np, dev);
> > > > +     if (err == -EPROBE_DEFER)
> > > > +             return err;
> > >
> > > To avoid leaking resources, I think this should be:
> > >
> > >                 goto err_grp_init;
> > will do in v2. Unfortunately net-next closes today AFAIK.
> On second thought, where did you find this?
> 
> git grep err_grp_init
> 
> returns nothing.
> 
> Not only that, this function has no goto.

Maybe we are looking at different things for some reason.

I'm looking at this:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/freescale/gianfar.c?id=bfba7bc8b7c2c100b76edb3a646fdce256392129#n814

> > >
> > > Flagged by Smatch.
> > >
> > > >       if (err) {
> > > >               eth_hw_addr_random(dev);
> > > >               dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> > >
> > > --
> > > pw-bot: cr
>

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Rosen Penev 2 months, 3 weeks ago

On Mon, Sep 9, 2024 at 11:48 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Sep 09, 2024 at 11:20:20AM -0700, Rosen Penev wrote:
> > On Mon, Sep 9, 2024 at 11:11 AM Rosen Penev <rosenp@gmail.com> wrote:
> > >
> > > On Mon, Sep 9, 2024 at 1:55 AM Simon Horman <horms@kernel.org> wrote:
> > > >
> > > > On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> > > > > If nvmem loads after the ethernet driver, mac address assignments will
> > > > > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > > > > case so we need to handle that to avoid eth_hw_addr_random.
> > > > >
> > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > > > > index 634049c83ebe..9755ec947029 100644
> > > > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > > > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > > > >               priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> > > > >
> > > > >       err = of_get_ethdev_address(np, dev);
> > > > > +     if (err == -EPROBE_DEFER)
> > > > > +             return err;
> > > >
> > > > To avoid leaking resources, I think this should be:
> > > >
> > > >                 goto err_grp_init;
> > > will do in v2. Unfortunately net-next closes today AFAIK.
> > On second thought, where did you find this?
> >
> > git grep err_grp_init
> >
> > returns nothing.
> >
> > Not only that, this function has no goto.
>
> Maybe we are looking at different things for some reason.
Well that's embarrassing. Locally I seem to have a commit that adds a
bunch of devm and as a result these gotos. Unfortunately I don't have
the hardware to test those changes. I'll be doing a v2 for when
net-next opens.
>
> I'm looking at this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/freescale/gianfar.c?id=bfba7bc8b7c2c100b76edb3a646fdce256392129#n814
>
> > > >
> > > > Flagged by Smatch.
> > > >
> > > > >       if (err) {
> > > > >               eth_hw_addr_random(dev);
> > > > >               dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> > > >
> > > > --
> > > > pw-bot: cr
> >

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Simon Horman 2 months, 3 weeks ago

On Mon, Sep 09, 2024 at 12:14:38PM -0700, Rosen Penev wrote:
> On Mon, Sep 9, 2024 at 11:48 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Sep 09, 2024 at 11:20:20AM -0700, Rosen Penev wrote:
> > > On Mon, Sep 9, 2024 at 11:11 AM Rosen Penev <rosenp@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 9, 2024 at 1:55 AM Simon Horman <horms@kernel.org> wrote:
> > > > >
> > > > > On Sun, Sep 08, 2024 at 02:35:54PM -0700, Rosen Penev wrote:
> > > > > > If nvmem loads after the ethernet driver, mac address assignments will
> > > > > > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > > > > > case so we need to handle that to avoid eth_hw_addr_random.
> > > > > >
> > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > > > ---
> > > > > >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > > > > > index 634049c83ebe..9755ec947029 100644
> > > > > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > > > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > > > > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > > > > >               priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> > > > > >
> > > > > >       err = of_get_ethdev_address(np, dev);
> > > > > > +     if (err == -EPROBE_DEFER)
> > > > > > +             return err;
> > > > >
> > > > > To avoid leaking resources, I think this should be:
> > > > >
> > > > >                 goto err_grp_init;
> > > > will do in v2. Unfortunately net-next closes today AFAIK.
> > > On second thought, where did you find this?
> > >
> > > git grep err_grp_init
> > >
> > > returns nothing.
> > >
> > > Not only that, this function has no goto.
> >
> > Maybe we are looking at different things for some reason.
> Well that's embarrassing. Locally I seem to have a commit that adds a
> bunch of devm and as a result these gotos. Unfortunately I don't have
> the hardware to test those changes. I'll be doing a v2 for when
> net-next opens.

No problem. TBH it is a relief, as I was beginning to doubt my own sanity.

> >
> > I'm looking at this:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/freescale/gianfar.c?id=bfba7bc8b7c2c100b76edb3a646fdce256392129#n814
> >
> > > > >
> > > > > Flagged by Smatch.
> > > > >
> > > > > >       if (err) {
> > > > > >               eth_hw_addr_random(dev);
> > > > > >               dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> > > > >
> > > > > --
> > > > > pw-bot: cr
> > >
>

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Pavan Chebbi 2 months, 3 weeks ago

On Mon, Sep 9, 2024 at 3:06 AM Rosen Penev <rosenp@gmail.com> wrote:
>
> If nvmem loads after the ethernet driver, mac address assignments will
> not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> case so we need to handle that to avoid eth_hw_addr_random.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

What is the issue you are facing with a random MAC address?
If there is a real problem, this patch should go to the net, with a
proper description and fixes tag.

>  drivers/net/ethernet/freescale/gianfar.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 634049c83ebe..9755ec947029 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>                 priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
>
>         err = of_get_ethdev_address(np, dev);
> +       if (err == -EPROBE_DEFER)
> +               return err;
>         if (err) {
>                 eth_hw_addr_random(dev);
>                 dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> --
> 2.46.0
>
>

Re: [PATCH net-next] net: gianfar: fix NVMEM mac address

Posted by Rosen Penev 2 months, 3 weeks ago

On Sun, Sep 8, 2024 at 9:22 PM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>
> On Mon, Sep 9, 2024 at 3:06 AM Rosen Penev <rosenp@gmail.com> wrote:
> >
> > If nvmem loads after the ethernet driver, mac address assignments will
> > not take effect. of_get_ethdev_address returns EPROBE_DEFER in such a
> > case so we need to handle that to avoid eth_hw_addr_random.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
>
> What is the issue you are facing with a random MAC address?
> If there is a real problem, this patch should go to the net, with a
> proper description and fixes tag.
Embedded devices usually store the mac address in some fixed offset on
flash. NVMEM deals with this.

I don't think net is appropriate given that there's not really a
commit to point to. nvmem is a fairly recent addition.

Similar commits in this vain are:
42404d8f1c01861b22ccfa1d70f950242720ae57
f7650d82e7dc501dfc5920c698bcc0591791a57c
f4693b81ea3802d2c28c868e1639e580d0da2d1f
be04024a24a93f761a7b2c5f2de46db0f3acdc74

>
> >  drivers/net/ethernet/freescale/gianfar.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 634049c83ebe..9755ec947029 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -716,6 +716,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >                 priv->device_flags |= FSL_GIANFAR_DEV_HAS_BUF_STASHING;
> >
> >         err = of_get_ethdev_address(np, dev);
> > +       if (err == -EPROBE_DEFER)
> > +               return err;
> >         if (err) {
> >                 eth_hw_addr_random(dev);
> >                 dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
> > --
> > 2.46.0
> >
> >