[PATCH] ethernet: aquantia: Try MAC address from device tree

[PATCH] ethernet: aquantia: Try MAC address from device tree

[Tianhao Chai]

Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
card, but instead need to obtain MAC addresses from the device tree. In
this case the hardware will report an invalid MAC.

Currently atlantic driver does not query the DT for MAC address and will
randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
This patch causes the driver to perfer a valid MAC address from OF (if
present) over HW self-reported MAC and only fall back to a random MAC
address when neither of them is valid.

Signed-off-by: Tianhao Chai <cth451@gmail.com>
---
 .../net/ethernet/aquantia/atlantic/aq_nic.c   | 28 ++++++++++++-------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 1acf544afeb4..ae6c4a044390 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -316,18 +316,26 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
 	aq_macsec_init(self);
 #endif
 
-	mutex_lock(&self->fwreq_mutex);
-	err = self->aq_fw_ops->get_mac_permanent(self->aq_hw, addr);
-	mutex_unlock(&self->fwreq_mutex);
-	if (err)
-		goto err_exit;
+	if (eth_platform_get_mac_address(&self->pdev->dev, addr) == 0 &&
+	    is_valid_ether_addr(addr)) {
+		// DT supplied a valid MAC address
+		eth_hw_addr_set(self->ndev, addr);
+	} else {
+		// If DT has none or an invalid one, ask device for MAC address
+		mutex_lock(&self->fwreq_mutex);
+		err = self->aq_fw_ops->get_mac_permanent(self->aq_hw, addr);
+		mutex_unlock(&self->fwreq_mutex);
 
-	eth_hw_addr_set(self->ndev, addr);
+		if (err)
+			goto err_exit;
 
-	if (!is_valid_ether_addr(self->ndev->dev_addr) ||
-	    !aq_nic_is_valid_ether_addr(self->ndev->dev_addr)) {
-		netdev_warn(self->ndev, "MAC is invalid, will use random.");
-		eth_hw_addr_random(self->ndev);
+		if (is_valid_ether_addr(addr) &&
+		    aq_nic_is_valid_ether_addr(addr)) {
+			eth_hw_addr_set(self->ndev, addr);
+		} else {
+			netdev_warn(self->ndev, "MAC is invalid, will use random.");
+			eth_hw_addr_random(self->ndev);
+		}
 	}
 
 #if defined(AQ_CFG_MAC_ADDR_PERMANENT)
-- 
2.30.2

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Andrew Lunn]

On Sat, Nov 27, 2021 at 08:37:33PM -0600, Tianhao Chai wrote:
> Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
> card, but instead need to obtain MAC addresses from the device tree. In
> this case the hardware will report an invalid MAC.
> 
> Currently atlantic driver does not query the DT for MAC address and will
> randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
> This patch causes the driver to perfer a valid MAC address from OF (if
> present) over HW self-reported MAC and only fall back to a random MAC
> address when neither of them is valid.

This is a change in behaviour, and could cause regressions. It would
be better to keep with the current flow. Call
aq_fw_ops->get_mac_permanent() first. If that does not give a valid
MAC address, then try DT, and lastly use a random MAC address.

    Andrew

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Hector Martin]

On 29/11/2021 01.33, Andrew Lunn wrote:
> On Sat, Nov 27, 2021 at 08:37:33PM -0600, Tianhao Chai wrote:
>> Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
>> card, but instead need to obtain MAC addresses from the device tree. In
>> this case the hardware will report an invalid MAC.
>>
>> Currently atlantic driver does not query the DT for MAC address and will
>> randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
>> This patch causes the driver to perfer a valid MAC address from OF (if
>> present) over HW self-reported MAC and only fall back to a random MAC
>> address when neither of them is valid.
> 
> This is a change in behaviour, and could cause regressions. It would
> be better to keep with the current flow. Call
> aq_fw_ops->get_mac_permanent() first. If that does not give a valid
> MAC address, then try DT, and lastly use a random MAC address.

On DT platforms, it is expected that the device tree MAC will override 
whatever the device thinks is its MAC address. See tg3, igb, igc, r8169, 
for examples where eth_platform_get_mac_address takes precedence over 
everything else.

I would not expect any other existing platform to have a MAC assigned to 
the device in this way using these cards; if any platforms do, chances 
are they intended it for it to be used and this patch will fix a current 
bug. If some platforms out there really have bogus MACs assigned in this 
way, that's a firmware bug, and we'd have to find out and add explicit, 
targeted workaround code. Are you aware of any such platforms? :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Heiner Kallweit]

On 28.11.2021 03:37, Tianhao Chai wrote:
> Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
> card, but instead need to obtain MAC addresses from the device tree. In
> this case the hardware will report an invalid MAC.
> 
> Currently atlantic driver does not query the DT for MAC address and will
> randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
> This patch causes the driver to perfer a valid MAC address from OF (if
> present) over HW self-reported MAC and only fall back to a random MAC
> address when neither of them is valid.
> 
> Signed-off-by: Tianhao Chai <cth451@gmail.com>
> ---
>  .../net/ethernet/aquantia/atlantic/aq_nic.c   | 28 ++++++++++++-------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 1acf544afeb4..ae6c4a044390 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -316,18 +316,26 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
>  	aq_macsec_init(self);
>  #endif
>  
> -	mutex_lock(&self->fwreq_mutex);
> -	err = self->aq_fw_ops->get_mac_permanent(self->aq_hw, addr);
> -	mutex_unlock(&self->fwreq_mutex);
> -	if (err)
> -		goto err_exit;
> +	if (eth_platform_get_mac_address(&self->pdev->dev, addr) == 0 &&
> +	    is_valid_ether_addr(addr)) {

Calling is_valid_ether_addr() shouldn't be needed here. of_get_mac_addr()
does this check already. If you should decide to keep this check:
Kernel doc of is_valid_ether_addr() states that argument must be
word-aligned. So you may need to add a __align(2) to the address char
array definition.

> +		// DT supplied a valid MAC address
> +		eth_hw_addr_set(self->ndev, addr);
> +	} else {
> +		// If DT has none or an invalid one, ask device for MAC address
> +		mutex_lock(&self->fwreq_mutex);
> +		err = self->aq_fw_ops->get_mac_permanent(self->aq_hw, addr);
> +		mutex_unlock(&self->fwreq_mutex);
>  
> -	eth_hw_addr_set(self->ndev, addr);
> +		if (err)
> +			goto err_exit;
>  
> -	if (!is_valid_ether_addr(self->ndev->dev_addr) ||
> -	    !aq_nic_is_valid_ether_addr(self->ndev->dev_addr)) {
> -		netdev_warn(self->ndev, "MAC is invalid, will use random.");
> -		eth_hw_addr_random(self->ndev);
> +		if (is_valid_ether_addr(addr) &&
> +		    aq_nic_is_valid_ether_addr(addr)) {
> +			eth_hw_addr_set(self->ndev, addr);
> +		} else {
> +			netdev_warn(self->ndev, "MAC is invalid, will use random.");
> +			eth_hw_addr_random(self->ndev);
> +		}
>  	}
>  
>  #if defined(AQ_CFG_MAC_ADDR_PERMANENT)
> 

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Andrew Lunn]

On Mon, Nov 29, 2021 at 02:08:28AM +0900, Hector Martin wrote:
> On 29/11/2021 01.33, Andrew Lunn wrote:
> > On Sat, Nov 27, 2021 at 08:37:33PM -0600, Tianhao Chai wrote:
> > > Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
> > > card, but instead need to obtain MAC addresses from the device tree. In
> > > this case the hardware will report an invalid MAC.
> > > 
> > > Currently atlantic driver does not query the DT for MAC address and will
> > > randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
> > > This patch causes the driver to perfer a valid MAC address from OF (if
> > > present) over HW self-reported MAC and only fall back to a random MAC
> > > address when neither of them is valid.
> > 
> > This is a change in behaviour, and could cause regressions. It would
> > be better to keep with the current flow. Call
> > aq_fw_ops->get_mac_permanent() first. If that does not give a valid
> > MAC address, then try DT, and lastly use a random MAC address.
> 
> On DT platforms, it is expected that the device tree MAC will override
> whatever the device thinks is its MAC address.

Can you point to any documentation of that expectation?

> I would not expect any other existing platform to have a MAC assigned to the
> device in this way using these cards; if any platforms do, chances are they
> intended it for it to be used and this patch will fix a current bug. If some
> platforms out there really have bogus MACs assigned in this way, that's a
> firmware bug, and we'd have to find out and add explicit, targeted
> workaround code. Are you aware of any such platforms? :)

I'm not aware of any, because i try to avoid making behaviour changes.

Anyway, lets go with this, and if stuff breaks we can always change
the order to what i suggested in order to unbreak stuff. I'm assuming
for Apple M1 Mac minis the order does not actually matter?

    Andrew

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Tianhao Chai]

> Calling is_valid_ether_addr() shouldn't be needed here. of_get_mac_addr()
> does this check already.

You are right. I'll remove the check.

~cth451

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Tianhao Chai]

On Tue, Nov 30, 2021 at 03:32:10AM +0100, Andrew Lunn wrote:
> On Mon, Nov 29, 2021 at 02:08:28AM +0900, Hector Martin wrote:
> > On DT platforms, it is expected that the device tree MAC will override
> > whatever the device thinks is its MAC address.
> 
> Can you point to any documentation of that expectation?

Since other drivers will prefer DT provided MAC as well, I'd assume
this to be the case, though I'm not sure where this behavior is documented.
I'm new to embedded systems and maybe Hector knows better than I do.

I don't think this will cause regression on platforms that don't even use
DT, say amd64, but could be a change of behavior where DT and NIC both
report valid MACs on OF platforms.

> I'm assuming for Apple M1 Mac minis the order does not actually matter?

The order does not matter in this case. On my M1 mini the hardware
reports an all-zero MAC address. The MAC from DT matches the one printed
on the box, and we should use this one instead.

~cth451

Re: [PATCH] ethernet: aquantia: Try MAC address from device tree

[Hector Martin]

On 30/11/2021 11.32, Andrew Lunn wrote:
> On Mon, Nov 29, 2021 at 02:08:28AM +0900, Hector Martin wrote:
>> On 29/11/2021 01.33, Andrew Lunn wrote:
>>> On Sat, Nov 27, 2021 at 08:37:33PM -0600, Tianhao Chai wrote:
>>>> Apple M1 Mac minis (2020) with 10GE NICs do not have MAC address in the
>>>> card, but instead need to obtain MAC addresses from the device tree. In
>>>> this case the hardware will report an invalid MAC.
>>>>
>>>> Currently atlantic driver does not query the DT for MAC address and will
>>>> randomly assign a MAC if the NIC doesn't have a permanent MAC burnt in.
>>>> This patch causes the driver to perfer a valid MAC address from OF (if
>>>> present) over HW self-reported MAC and only fall back to a random MAC
>>>> address when neither of them is valid.
>>>
>>> This is a change in behaviour, and could cause regressions. It would
>>> be better to keep with the current flow. Call
>>> aq_fw_ops->get_mac_permanent() first. If that does not give a valid
>>> MAC address, then try DT, and lastly use a random MAC address.
>>
>> On DT platforms, it is expected that the device tree MAC will override
>> whatever the device thinks is its MAC address.
> 
> Can you point to any documentation of that expectation?

I don't think this is explicitly clarified anywhere, but the DT binding 
says:

 > Specifies the MAC address that was assigned to the network device.

It certainly doesn't say this should be a fallback only to be used if 
the device doesn't have some idea of its MAC. Usually you'd expect 
firmware information to override whatever the device's built-in defaults 
are.

>> I would not expect any other existing platform to have a MAC assigned to the
>> device in this way using these cards; if any platforms do, chances are they
>> intended it for it to be used and this patch will fix a current bug. If some
>> platforms out there really have bogus MACs assigned in this way, that's a
>> firmware bug, and we'd have to find out and add explicit, targeted
>> workaround code. Are you aware of any such platforms? :)
> 
> I'm not aware of any, because i try to avoid making behaviour changes.
> 
> Anyway, lets go with this, and if stuff breaks we can always change
> the order to what i suggested in order to unbreak stuff. I'm assuming
> for Apple M1 Mac minis the order does not actually matter?

Correct, on these machines the burned-in MAC is invalid so it doesn't 
matter.


-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub