Skip to contentSkip to conversation

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

PATCH In review Ivan Delalande 4 participants 5 messages Sep 3 – Sep 10

Subsystems (3)
Networking Network drivers Broadcom TG3 gigabit Ethernet driver
lore
Share & tools
lore

Ivan Delalande

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/net/ethernet/broadcom/tg3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/tg3.c +2 -1
index 73a4b569b03e..0b47908f0935 100644
@@ -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;
Signature · 1 line
2.43.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

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.

Quoted 5 lines
drivers/net/ethernet/broadcom/tg3.c +0 -0
index 73a4b569b03e3..0b47908f09355 100644

drivers/net/ethernet/broadcom/tg3.c static int tg3_get_device_address(struct tg3 *tp, u8 *addr) · line 17088

Quoted 9 lines
drivers/net/ethernet/broadcom/tg3.c +2 -1
@@ -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?

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903233255.GA4109366%40visor
maintainer
#2 lore raw
In reply to Ivan Delalande
Quoted 10 lines · 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:

drivers/net/ethernet/broadcom/tg3.c +6 -5
@@ -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;

?

author
#3 lore raw
Quoted 8 lines · Jakub Kicinski
Quoted 7 lines · 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.

Quoted 6 lines
Quoted 3 lines

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,

Quoted 23 lines
drivers/net/ethernet/broadcom/tg3.c +6 -5
@@ -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;

?

Signature · 1 line
Ivan Delalande - Arista Networks
maintainer
#4 lore raw
Quoted 21 lines · Ivan Delalande
Quoted 8 lines · Jakub Kicinski
Quoted 7 lines · 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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 9 lines

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..

Quoted 29 lines

Thanks,

Quoted 22 lines
drivers/net/ethernet/broadcom/tg3.c +6 -5
@@ -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;
Quoted 1 line

?

--
Ivan Delalande - Arista Networks