diff mbox series

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

Message ID 20240908213554.11979-1-rosenp@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: gianfar: fix NVMEM mac address | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-09-09--00-00 (tests: 722)

Commit Message

Rosen Penev Sept. 8, 2024, 9:35 p.m. UTC
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(+)

Comments

Pavan Chebbi Sept. 9, 2024, 4:22 a.m. UTC | #1
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
>
>
Simon Horman Sept. 9, 2024, 8:55 a.m. UTC | #2
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);
Rosen Penev Sept. 9, 2024, 5:52 p.m. UTC | #3
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
> >
> >
Rosen Penev Sept. 9, 2024, 6:11 p.m. UTC | #4
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
Rosen Penev Sept. 9, 2024, 6:20 p.m. UTC | #5
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
Simon Horman Sept. 9, 2024, 6:48 p.m. UTC | #6
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
>
Rosen Penev Sept. 9, 2024, 7:14 p.m. UTC | #7
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
> >
Simon Horman Sept. 10, 2024, 6:44 a.m. UTC | #8
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
> > >
>
diff mbox series

Patch

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);