tg3: use random MAC address when tg3_get_device_address fails
Subsystems (3)
PATCH v2 1/1 tg3: use random MAC address when tg3_get_device_address fails
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 as fallback on our systems.
Make the driver assign a random address in this condition instead of being fatal for probe.
v2: assign random address when tg3_get_device_address fails instead of
changing its logic.
--- drivers/net/ethernet/broadcom/tg3.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
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
2.43.0
Quoted 14 lines · 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 sincee4c00ba7274b("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 genericdevice_get_mac_addressinfrastructure as fallback on our systems.Make the driver assign a random address in this condition instead of being fatal for probe.
v2: assign random address when
tg3_get_device_addressfails instead of
changing its logic.
Quoted 8 lines
Fixes e4c00ba7274b ("tg3: replace placeholder MAC address with device property")Suggested-by Jakub Kicinski <[email protected]>Signed-off-by Ivan Delalande <[email protected]>--- drivers/net/ethernet/broadcom/tg3.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Thank you.
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] tg3_init_one() (drivers/net/ethernet/broadcom/tg3.c:17916-17923) now converts *every*…
- [Low] The recorded commit message of eb360ca620ab contains the inter- revision changelog line "v2: assign random address when…
--
tg3: use random MAC address when tg3_get_device_address fails
Instead of failing probe when no valid ethernet address can be obtained, tg3_init_one() now warns and assigns a random MAC address so the device still probes on systems where the SRAM placeholder address cannot be replaced through device_get_mac_address().
Quoted 2 lines
v2: assign random address when
tg3_get_device_addressfails instead of
changing its logic.
[Severity: Low]
This isn't a bug, but should the "v2:" changelog line live below the --- separator instead of in the commit body? The worked example in Documentation/process/maintainer-netdev.rst places the per-version changelog after the --- marker that follows the Signed-off-by tags, so it does not end up in git history.
Quoted 10 lines
drivers/net/ethernet/broadcom/tg3.c +0
-2@@ -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,
drivers/net/ethernet/broadcom/tg3.c static int tg3_init_one(struct pci_dev *pdev, · line 17922
Quoted 10 lines
drivers/net/ethernet/broadcom/tg3.c +6
-3- "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);
[Severity: Medium]
Should -EPROBE_DEFER be handled separately here rather than being turned into a random address?
In the exact case this patch targets, the SRAM holds the placeholder address, so tg3_get_device_address() returns whatever device_get_mac_address() gives back:
drivers/net/ethernet/broadcom/tg3.c:tg3_get_device_address() {
...
if (tg3_is_default_mac_address(addr))
return device_get_mac_address(&tp->pdev->dev, addr);
return 0;
}That value can be -EPROBE_DEFER:
net/ethernet/eth.c:device_get_mac_address() {
if (!fwnode_get_mac_address(dev_fwnode(dev), addr))
return 0;
return nvmem_get_mac_address(dev, addr);
}net/ethernet/eth.c:nvmem_get_mac_address() {
...
cell = nvmem_cell_get(dev, "mac-address");
if (IS_ERR(cell))
return PTR_ERR(cell);
...
}drivers/nvmem/core.c:nvmem_cell_get() {
...
if (dev->of_node) { /* try dt first */
cell = of_nvmem_cell_get(dev->of_node, id);
if (!IS_ERR(cell) || PTR_ERR(cell) == -EPROBE_DEFER)
return cell;
}
...
}Before this change the error was propagated out of the PCI probe via 'goto err_out_apeunmap', so the driver core retried once the NVMEM provider registered and the firmware MAC was picked up. With the new else branch, does the deferral get swallowed, leaving the interface registered with a locally-administered random address that changes on every boot or module reload, with no later retry?
Would the usual idiom help here, i.e. returning early on -EPROBE_DEFER before falling back to eth_hw_addr_random()?
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914230521.GA1400582%40visor