[PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[Ivan Delalande]

Some of the tg3 NICs we use (BCM57762) reset the SRAM MAC address to the
placeholder address on link flaps, tg3_chip_reset, etc. We've typically
fixed it from userspace, but since e4c00ba7274b ("tg3: replace
placeholder MAC address with device property") was merged, tg3 just
fails probe as we don't have a way to get it through the generic
device_get_mac_address infrastructure on our systems.

Since that commit effectively made the placeholder address invalid,
include this check in the earlier `addr_ok` expression, so we proceed
with the NVRAM and control register fallback to find a valid MAC.

Fixes: e4c00ba7274b ("tg3: replace placeholder MAC address with device property")
Signed-off-by: Ivan Delalande <colona@arista.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 73a4b569b03e..0b47908f0935 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -17081,7 +17081,8 @@ static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
 		addr[5] = (lo >>  0) & 0xff;
 
 		/* Some old bootcode may report a 0 MAC address in SRAM */
-		addr_ok = is_valid_ether_addr(addr);
+		addr_ok = is_valid_ether_addr(addr) &&
+			  !tg3_is_default_mac_address(addr);
 	}
 	if (!addr_ok) {
 		__be32 be_hi, be_lo;
-- 
2.43.0

Re: [PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] In tg3_get_device_address()
  (drivers/net/ethernet/broadcom/tg3.c), clearing addr_ok when the SRAM…

--

tg3: check NVRAM and control register if SRAM has the placeholder MAC address

This adds the placeholder address check to the addr_ok expression in
tg3_get_device_address(), so a BCM57762 reporting 00:10:18:00:00:00 in the
SRAM mailbox falls through to the NVRAM and MAC control register sources
instead of failing probe.

> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 73a4b569b03e3..0b47908f09355 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -17081,7 +17081,8 @@ static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
>  		addr[5] = (lo >>  0) & 0xff;
>  
>  		/* Some old bootcode may report a 0 MAC address in SRAM */
> -		addr_ok = is_valid_ether_addr(addr);
> +		addr_ok = is_valid_ether_addr(addr) &&
> +			  !tg3_is_default_mac_address(addr);
>  	}
>  	if (!addr_ok) {
>  		__be32 be_hi, be_lo;

[Severity: High]
Does clearing addr_ok for the placeholder address remove the only way to
reach the device_get_mac_address() fallback added by e4c00ba7274b?

Once addr_ok is 0, the block that follows rewrites all six bytes of addr
from NVRAM or from MAC_ADDR_0_{HIGH,LOW}, so the placeholder value no
longer survives to the tail of tg3_get_device_address():

drivers/net/ethernet/broadcom/tg3.c:tg3_get_device_address() {
	...
	if (!is_valid_ether_addr(addr))
		return -EINVAL;

	if (tg3_is_default_mac_address(addr))
		return device_get_mac_address(&tp->pdev->dev, addr);
	...
}

If the NVRAM words at mac_offset or the MAC control registers read as
0xffffffff (erased flash, ff:ff:ff:ff:ff:ff is multicast) or as zeros, the
is_valid_ether_addr() test returns -EINVAL before the
device_get_mac_address() call is reached.  Would that not move the probe
failure from the placeholder path to the NVRAM/register path, on the same
systems (iMac 20,1 / BCM57766) where the fwnode property is currently the
working source?

And when the NVRAM or register value is valid but stale, is the firmware
property still consulted at all?  It looks like the hardware source now
wins over the property, which inverts the priority e4c00ba7274b set up for
the placeholder-in-SRAM case.

Note eth_platform_get_mac_address() earlier in the same function does not
cover that platform class either, since net/ethernet/eth.c only consults
dev->of_node via of_get_mac_address() and arch_get_platform_mac_address(),
not dev_fwnode()/ACPI.

Would it work to read each fallback source into a scratch buffer, reject
content that is invalid or equal to the placeholder, and keep
device_get_mac_address() as the last resort, for example by testing

	if (!is_valid_ether_addr(addr) || tg3_is_default_mac_address(addr))
		return device_get_mac_address(&tp->pdev->dev, addr);

instead of returning -EINVAL first?

A related question about the fallback block itself: tg3_nvram_read_be32()
returns 0 for any successfully executed read command regardless of the
content read, so unprogrammed NVRAM (0x00.. or 0xff..) is copied into addr
and also suppresses the MAC_ADDR_0_{HIGH,LOW} branch that may still hold
the bootcode-programmed address.  Should the NVRAM content be validated
before that branch is skipped?

Lastly, could the commit message mention that the device property fallback
is being narrowed by this change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903233255.GA4109366%40visor

Re: [PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[Jakub Kicinski]

On Thu, 3 Sep 2026 16:32:55 -0700 Ivan Delalande wrote:
> Some of the tg3 NICs we use (BCM57762) reset the SRAM MAC address to the
> placeholder address on link flaps, tg3_chip_reset, etc. We've typically
> fixed it from userspace, but since e4c00ba7274b ("tg3: replace
> placeholder MAC address with device property") was merged, tg3 just
> fails probe as we don't have a way to get it through the generic
> device_get_mac_address infrastructure on our systems.
> 
> Since that commit effectively made the placeholder address invalid,
> include this check in the earlier `addr_ok` expression, so we proceed
> with the NVRAM and control register fallback to find a valid MAC.

If there are so many cases with known-broken MAC addresses with tg3
shouldnt we do something like:

--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -17914,12 +17914,13 @@ static int tg3_init_one(struct pci_dev *pdev,
        }
 
        err = tg3_get_device_address(tp, addr);
-       if (err) {
-               dev_err(&pdev->dev,
-                       "Could not obtain valid ethernet address, aborting\n");
-               goto err_out_apeunmap;
+       if (!err) {
+               eth_hw_addr_set(dev, addr);
+       } else {
+               dev_warn(&pdev->dev,
+                        "Could not obtain valid ethernet address, using a random address\n");
+               eth_hw_addr_random(dev);
        }
-       eth_hw_addr_set(dev, addr);
 
        intmbx = MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW;
        rcvmbx = MAILBOX_RCVRET_CON_IDX_0 + TG3_64BIT_REG_LOW;

?

Re: [PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[Ivan Delalande]

On Wed, Sep 09, 2026 at 07:17:51PM -0700, Jakub Kicinski wrote:
> On Thu, 3 Sep 2026 16:32:55 -0700 Ivan Delalande wrote:
> > Some of the tg3 NICs we use (BCM57762) reset the SRAM MAC address to the
> > placeholder address on link flaps, tg3_chip_reset, etc. We've typically
> > fixed it from userspace, but since e4c00ba7274b ("tg3: replace
> > placeholder MAC address with device property") was merged, tg3 just
> > fails probe as we don't have a way to get it through the generic
> > device_get_mac_address infrastructure on our systems.
> > 
> > Since that commit effectively made the placeholder address invalid,
> > include this check in the earlier `addr_ok` expression, so we proceed
> > with the NVRAM and control register fallback to find a valid MAC.
> 
> If there are so many cases with known-broken MAC addresses with tg3
> shouldnt we do something like:

I think we could work with this on our systems, though it would be best
if the driver also checked the control register when SRAM is invalid or
the default MAC.

I was testing a slightly more extensive v2 based on Sashiko's comments
as I agree tg3_get_device_address() is pretty awkard in its current
version. But I don't have access to enough different tg3 NICs or
Broadcom internal docs for a confident rewrite, so I was hoping Pavan
and Michael could chime in on all of this (should we just check SRAM,
NVRAM, control register, and device_get_mac_address in sequence and take
the first valid non-placeholder address? should device_get_mac_address
take priority over any of the other sources? should this be controlled
or overridable through module parameters?).

Thanks,

> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -17914,12 +17914,13 @@ static int tg3_init_one(struct pci_dev *pdev,
>         }
>  
>         err = tg3_get_device_address(tp, addr);
> -       if (err) {
> -               dev_err(&pdev->dev,
> -                       "Could not obtain valid ethernet address, aborting\n");
> -               goto err_out_apeunmap;
> +       if (!err) {
> +               eth_hw_addr_set(dev, addr);
> +       } else {
> +               dev_warn(&pdev->dev,
> +                        "Could not obtain valid ethernet address, using a random address\n");
> +               eth_hw_addr_random(dev);
>         }
> -       eth_hw_addr_set(dev, addr);
>  
>         intmbx = MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW;
>         rcvmbx = MAILBOX_RCVRET_CON_IDX_0 + TG3_64BIT_REG_LOW;
> 
> ?

-- 
Ivan Delalande - Arista Networks

Re: [PATCH net] tg3: check NVRAM and control register if SRAM has the placeholder MAC address

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 2928 bytes --]

On Thu, Sep 10, 2026 at 11:01 AM Ivan Delalande <colona@arista.com> wrote:
>
> On Wed, Sep 09, 2026 at 07:17:51PM -0700, Jakub Kicinski wrote:
> > On Thu, 3 Sep 2026 16:32:55 -0700 Ivan Delalande wrote:
> > > Some of the tg3 NICs we use (BCM57762) reset the SRAM MAC address to the
> > > placeholder address on link flaps, tg3_chip_reset, etc. We've typically
> > > fixed it from userspace, but since e4c00ba7274b ("tg3: replace
> > > placeholder MAC address with device property") was merged, tg3 just
> > > fails probe as we don't have a way to get it through the generic
> > > device_get_mac_address infrastructure on our systems.
> > >
> > > Since that commit effectively made the placeholder address invalid,
> > > include this check in the earlier `addr_ok` expression, so we proceed
> > > with the NVRAM and control register fallback to find a valid MAC.
> >
> > If there are so many cases with known-broken MAC addresses with tg3
> > shouldnt we do something like:
>
> I think we could work with this on our systems, though it would be best
> if the driver also checked the control register when SRAM is invalid or
> the default MAC.
>
> I was testing a slightly more extensive v2 based on Sashiko's comments
> as I agree tg3_get_device_address() is pretty awkard in its current
> version. But I don't have access to enough different tg3 NICs or
> Broadcom internal docs for a confident rewrite, so I was hoping Pavan
> and Michael could chime in on all of this (should we just check SRAM,
> NVRAM, control register, and device_get_mac_address in sequence and take
> the first valid non-placeholder address? should device_get_mac_address
> take priority over any of the other sources? should this be controlled
> or overridable through module parameters?).

IMO Jakub's suggestion fixes your issue with 0 side effects. Changing
the priority or the order is an unnecessary change in behaviour..

>
> Thanks,
>
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -17914,12 +17914,13 @@ static int tg3_init_one(struct pci_dev *pdev,
> >         }
> >
> >         err = tg3_get_device_address(tp, addr);
> > -       if (err) {
> > -               dev_err(&pdev->dev,
> > -                       "Could not obtain valid ethernet address, aborting\n");
> > -               goto err_out_apeunmap;
> > +       if (!err) {
> > +               eth_hw_addr_set(dev, addr);
> > +       } else {
> > +               dev_warn(&pdev->dev,
> > +                        "Could not obtain valid ethernet address, using a random address\n");
> > +               eth_hw_addr_random(dev);
> >         }
> > -       eth_hw_addr_set(dev, addr);
> >
> >         intmbx = MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW;
> >         rcvmbx = MAILBOX_RCVRET_CON_IDX_0 + TG3_64BIT_REG_LOW;
> >
> > ?
>
> --
> Ivan Delalande - Arista Networks

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]