tg3: replace placeholder MAC address with device property

[PATCH] tg3: replace placeholder MAC address with device property

Posted by Paul SAGE 3 months, 2 weeks ago

On some systems (e.g. iMac 20,1 with BCM57766), the tg3 driver reads a default placeholder mac address (00:10:18:00:00:00) from the mailbox.
The correct value on those systems are stored in the 'local-mac-address' property.

This patch, detect the default value and tries to retrieve the correct address from the device_get_mac_address function instead.

The patch has been tested on two different systems:
- iMac 20,1 (BCM57766) model which use the local-mac-address property
- iMac 13,2 (BCM57766) model which can use the mailbox, NVRAM or MAC control registers

Co-developed-by: Vincent MORVAN <vinc@42.fr>
Signed-off-by: Vincent MORVAN <vinc@42.fr>
Signed-off-by: Paul SAGE <paul.sage@42.fr>
---
 drivers/net/ethernet/broadcom/tg3.c | 12 ++++++++++++
 drivers/net/ethernet/broadcom/tg3.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index d78cafdb2094..55c2f2804df5 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -17042,6 +17042,14 @@ static int tg3_get_invariants(struct tg3 *tp, const struct pci_device_id *ent)
 	return err;
 }
 
+static int tg3_is_default_mac_address(u8 *addr)
+{
+	u32 addr_high = (addr[0] << 16) | (addr[1] << 8) | addr[2];
+	u32 addr_low = (addr[3] << 16) | (addr[4] <<  8) | addr[5];
+
+	return addr_high == BROADCOM_OUI && addr_low == 0;
+}
+
 static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
 {
 	u32 hi, lo, mac_offset;
@@ -17115,6 +17123,10 @@ static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
 
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
+
+	if (tg3_is_default_mac_address(addr))
+		device_get_mac_address(&tp->pdev->dev, addr);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index a9e7f88fa26d..9fb226772e79 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -14,6 +14,8 @@
 #ifndef _T3_H
 #define _T3_H
 
+#define BROADCOM_OUI			0x00001018
+
 #define TG3_64BIT_REG_HIGH		0x00UL
 #define TG3_64BIT_REG_LOW		0x04UL
 
-- 
2.51.0

Re: [PATCH] tg3: replace placeholder MAC address with device property

Posted by Andrew Lunn 3 months, 2 weeks ago

On Thu, Oct 23, 2025 at 06:08:42PM +0200, Paul SAGE wrote:
> On some systems (e.g. iMac 20,1 with BCM57766), the tg3 driver reads a default placeholder mac address (00:10:18:00:00:00) from the mailbox.
> The correct value on those systems are stored in the 'local-mac-address' property.

Is this the DT property? I don't see any compatible in tg.c, so it is
not clear to me how this driver gets probed. 

Also, please wrap the commit message to around 70 characters.

    Andrew

---
pw-bot: cr

RE: Andrew Lunn

Posted by Paul SAGE 3 months, 1 week ago

The tg3 currently call eth_platform_get_mac_address to retrieve the
MAC address from the device tree before trying the mailbox,
NVRAM and MAC registers.
However, this function only retrieves the MAC address from the device
tree using of_get_mac_address.

We are using device_get_mac_address, which use fwnode to obtain a MAC
address using the ACPI, so as we understand fwnode is an
abstraction layer for both the device tree (on ARM) and ACPI (on x86)

If true, it could be appropriate to replace the call to replace
eth_platform_get_mac_address with device_get_mac_address. This would
avoid running the entire function only to later check for a dummy
address.

Do you see any regression possible with this change ?

Re: Andrew Lunn

Posted by Andrew Lunn 3 months, 1 week ago

On Mon, Oct 27, 2025 at 10:51:39AM +0100, Paul SAGE wrote:
> The tg3 currently call eth_platform_get_mac_address to retrieve the
> MAC address from the device tree before trying the mailbox,
> NVRAM and MAC registers.
> However, this function only retrieves the MAC address from the device
> tree using of_get_mac_address.
> 
> We are using device_get_mac_address, which use fwnode to obtain a MAC
> address using the ACPI, so as we understand fwnode is an
> abstraction layer for both the device tree (on ARM) and ACPI (on x86)
> 
> If true, it could be appropriate to replace the call to replace
> eth_platform_get_mac_address with device_get_mac_address. This would
> avoid running the entire function only to later check for a dummy
> address.
> 
> Do you see any regression possible with this change ?

I don't know ACPI too well, DT is more advanced, and i tend to keep to
ARM platforms.

Does ACPI actually have a standardised mechanism for storing MAC
address? Is there anything in the standard?

If you do make use of device_get_mac_address(), how is the MAC address
stored in ACPI?

Documentation/firmware-guide/acpi/dsd/phy.rst says:

  These properties are defined in accordance with the "Device
  Properties UUID For _DSD" [dsd-guide] document and the
  daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device
  Data Descriptors containing them.

Is that what you are doing?

Have you looked through other MAC drivers? Are there any others
getting the MAC address from ACPI? Is it all proprietary, or is there
some standardisation?

If you do decide ACPI is missing this, and you want to move forward,
please also add a document under Documentation/firmware-guide/acpi/dsd
describing it.

	Andrew

Re: [PATCH] tg3: replace placeholder MAC address with device property

Posted by Florian Fainelli 3 months, 2 weeks ago

On 10/23/25 09:08, Paul SAGE wrote:
> On some systems (e.g. iMac 20,1 with BCM57766), the tg3 driver reads a default placeholder mac address (00:10:18:00:00:00) from the mailbox.
> The correct value on those systems are stored in the 'local-mac-address' property.
> 
> This patch, detect the default value and tries to retrieve the correct address from the device_get_mac_address function instead.
> 
> The patch has been tested on two different systems:
> - iMac 20,1 (BCM57766) model which use the local-mac-address property
> - iMac 13,2 (BCM57766) model which can use the mailbox, NVRAM or MAC control registers
> 
> Co-developed-by: Vincent MORVAN <vinc@42.fr>
> Signed-off-by: Vincent MORVAN <vinc@42.fr>
> Signed-off-by: Paul SAGE <paul.sage@42.fr>
> ---
>   drivers/net/ethernet/broadcom/tg3.c | 12 ++++++++++++
>   drivers/net/ethernet/broadcom/tg3.h |  2 ++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index d78cafdb2094..55c2f2804df5 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -17042,6 +17042,14 @@ static int tg3_get_invariants(struct tg3 *tp, const struct pci_device_id *ent)
>   	return err;
>   }
>   
> +static int tg3_is_default_mac_address(u8 *addr)
> +{
> +	u32 addr_high = (addr[0] << 16) | (addr[1] << 8) | addr[2];
> +	u32 addr_low = (addr[3] << 16) | (addr[4] <<  8) | addr[5];
> +
> +	return addr_high == BROADCOM_OUI && addr_low == 0;
> +}
> +
>   static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
>   {
>   	u32 hi, lo, mac_offset;
> @@ -17115,6 +17123,10 @@ static int tg3_get_device_address(struct tg3 *tp, u8 *addr)
>   
>   	if (!is_valid_ether_addr(addr))
>   		return -EINVAL;
> +
> +	if (tg3_is_default_mac_address(addr))
> +		device_get_mac_address(&tp->pdev->dev, addr);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
> index a9e7f88fa26d..9fb226772e79 100644
> --- a/drivers/net/ethernet/broadcom/tg3.h
> +++ b/drivers/net/ethernet/broadcom/tg3.h
> @@ -14,6 +14,8 @@
>   #ifndef _T3_H
>   #define _T3_H
>   
> +#define BROADCOM_OUI			0x00001018

There are multiple OUIs that Broadcom has used throughout the years, 
they are documented under include/linux/brcmphy.h, to avoid any 
confusion for people looking only at that driver, I would rather we find 
a different constant name, or we just don't use a constant.
-- 
Florian