| Message ID | 20211128023733.GA466664@cth-desktop-dorm.mad.wi.cth451.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: aquantia: Try MAC address from device tree | expand |
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
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? :)
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) >
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
> 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
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
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.
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)
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(-)