Re: [PATCH net v2] tg3: use random MAC address when tg3_get_device_address fails

[netdev-bot+sashiko@kernel.org]

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

Link: https://lore.kernel.org/netdev/20260903233255.GA4109366@visor/t/#u

>     v2: assign random address when tg3_get_device_address fails 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.

> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 73a4b569b03e3..75cb0b0fab2fa 100644
> --- 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);

[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()?

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