| Message ID | 20221130174259.1591567-1-bmasney@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: atlantic: fix check for invalid ethernet addresses | expand |
On Wed, Nov 30, 2022 at 12:42:59PM -0500, Brian Masney wrote: > The Qualcomm sa8540p automotive development board (QDrive3) has an > Aquantia NIC wired over PCIe. The ethernet MAC address assigned to > all of the boards in our lab is 00:17:b6:00:00:00. The existing > check in aq_nic_is_valid_ether_addr() only checks for leading zeros > in the MAC address. Let's update the check to also check for trailing > zeros in the MAC address so that a random MAC address is assigned > in this case. > > Signed-off-by: Brian Masney <bmasney@redhat.com> I have a question about the original commit that introduced this check: 553217c24426 ("ethernet: aquantia: Try MAC address from device tree"). The commit message talks about getting the MAC address from device tree, however I don't see any compatible lines in this driver, nor a of_match_table. As far as I can tell, this driver is only setup to be accessed over PCIe. The random MAC address is not ideal for our lab since we'd like to have stable addresses. I'd like to have the bootloader be able to inject a MAC address that's generated based on the board's serial number. I assume that it would go in the chosen node in device tree. One of the issues is that there are multiple NICs on this board, so I'm not sure how that would go in the chosen node and identify this particular NIC. Does anyone know of a place in the kernel where this is already done? Brian
On Wed, Nov 30, 2022 at 12:57:23PM -0500, Brian Masney wrote: > I have a question about the original commit that introduced this check: > 553217c24426 ("ethernet: aquantia: Try MAC address from device tree"). > The commit message talks about getting the MAC address from device tree, > however I don't see any compatible lines in this driver, nor a > of_match_table. As far as I can tell, this driver is only setup to be > accessed over PCIe. In aq_nic_ndev_register(), the code calls platform_get_ethdev_address(), which in turn access the device tree via OF interface. > The random MAC address is not ideal for our lab since we'd like to have > stable addresses. I'd like to have the bootloader be able to inject a > MAC address that's generated based on the board's serial number. I > assume that it would go in the chosen node in device tree. One of the > issues is that there are multiple NICs on this board, so I'm not sure > how that would go in the chosen node and identify this particular NIC. > Does anyone know of a place in the kernel where this is already done? I'm not familar with this particular board, but this probably shouldn't be done in kernel. AFAIK uboot allows overriding MAC with env 'ethaddr'. uboot then either writes this MAC into DT or calls NIC specific code to set the MAC into NIC memory before booting the kernel. The other way around I can think of is to use systemd-networkd or some other network management daemon to override the mac address as it tries to establish a network connection. This might be less hassle if you don't want to mess with the boot loader, but for embedded devices you'd need a different root fs image for every board. Acked-by: Tianhao Chai <cth451@gmail.com>
On Wed, Nov 30, 2022 at 01:26:40PM -0500, Tianhao Chai wrote: > I'm not familar with this particular board, but this probably shouldn't > be done in kernel. AFAIK uboot allows overriding MAC with env 'ethaddr'. > uboot then either writes this MAC into DT or calls NIC specific code to > set the MAC into NIC memory before booting the kernel. Our Boot Loader is ABL on the Qualcomm platform. > The other way around I can think of is to use systemd-networkd or some > other network management daemon to override the mac address as it tries > to establish a network connection. This might be less hassle if you > don't want to mess with the boot loader, but for embedded devices you'd > need a different root fs image for every board. We'll look into the systemd approach. I see that our board serial number is available in /sys/devices/soc0/serial_number and we can have a script generate a MAC address based on that. > Acked-by: Tianhao Chai <cth451@gmail.com> Thanks! Brian
On Wed, Nov 30, 2022 at 12:42:59PM -0500, Brian Masney wrote: > The Qualcomm sa8540p automotive development board (QDrive3) has an > Aquantia NIC wired over PCIe. The ethernet MAC address assigned to > all of the boards in our lab is 00:17:b6:00:00:00. The existing > check in aq_nic_is_valid_ether_addr() only checks for leading zeros > in the MAC address. Let's update the check to also check for trailing > zeros in the MAC address so that a random MAC address is assigned > in this case. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > index 06508eebb585..c9c850bbc805 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > @@ -293,7 +293,8 @@ static bool aq_nic_is_valid_ether_addr(const u8 *addr) > /* Some engineering samples of Aquantia NICs are provisioned with a > * partially populated MAC, which is still invalid. > */ > - return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0); > + return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0) && > + !(addr[3] == 0 && addr[4] == 0 && addr[5] == 0); Hi Brian is_valid_ether_addr() Andrew
On Wed, Nov 30, 2022 at 08:41:29PM +0100, Andrew Lunn wrote: > On Wed, Nov 30, 2022 at 12:42:59PM -0500, Brian Masney wrote: > > The Qualcomm sa8540p automotive development board (QDrive3) has an > > Aquantia NIC wired over PCIe. The ethernet MAC address assigned to > > all of the boards in our lab is 00:17:b6:00:00:00. The existing > > check in aq_nic_is_valid_ether_addr() only checks for leading zeros > > in the MAC address. Let's update the check to also check for trailing > > zeros in the MAC address so that a random MAC address is assigned > > in this case. > > > > Signed-off-by: Brian Masney <bmasney@redhat.com> > > --- > > drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > > index 06508eebb585..c9c850bbc805 100644 > > --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > > +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c > > @@ -293,7 +293,8 @@ static bool aq_nic_is_valid_ether_addr(const u8 *addr) > > /* Some engineering samples of Aquantia NICs are provisioned with a > > * partially populated MAC, which is still invalid. > > */ > > - return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0); > > + return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0) && > > + !(addr[3] == 0 && addr[4] == 0 && addr[5] == 0); > > Hi Brian > > is_valid_ether_addr() aq_nic_ndev_register() already calls is_valid_ether_addr(): if (is_valid_ether_addr(addr) && aq_nic_is_valid_ether_addr(addr)) { (self->ndev, addr); } else { ... } That won't work for this board since that function only checks that the MAC "is not 00:00:00:00:00:00, is not a multicast address, and is not FF:FF:FF:FF:FF:FF." The MAC address that we get on all of our boards is 00:17:b6:00:00:00. Brian
> > > - return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0); > > > + return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0) && > > > + !(addr[3] == 0 && addr[4] == 0 && addr[5] == 0); > > > > Hi Brian > > > > is_valid_ether_addr() > > aq_nic_ndev_register() already calls is_valid_ether_addr(): > > if (is_valid_ether_addr(addr) && > aq_nic_is_valid_ether_addr(addr)) { > (self->ndev, addr); > } else { > ... > } > > That won't work for this board since that function only checks that the > MAC "is not 00:00:00:00:00:00, is not a multicast address, and is not > FF:FF:FF:FF:FF:FF." The MAC address that we get on all of our boards is > 00:17:b6:00:00:00. Which is a valid MAC address. So i don't see why the kernel should reject it and use a random one. Maybe you should talk to Marvell about how you can program the e-fuses. You can then use MAC addresses from A8-97-DC etc. Andrew
From: Andrew Lunn > Sent: 30 November 2022 21:33 ... > > That won't work for this board since that function only checks that the > > MAC "is not 00:00:00:00:00:00, is not a multicast address, and is not > > FF:FF:FF:FF:FF:FF." The MAC address that we get on all of our boards is > > 00:17:b6:00:00:00. > > Which is a valid MAC address. So i don't see why the kernel should > reject it and use a random one. It isn't very valid... The first three bytes are the mulicast, local and company bits. So the last three bytes being zero indicate you have the very first address the company allocated. Pretty much zero chance of that board ever working well enough to be in a system. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
> Pretty much zero chance of that board ever working well > enough to be in a system. IBM, for example, has at least three ranges. Maybe they could of reached the end of one range, and simply continued shipping products from the beginning of the next range... aQuantia only has one range, so i would however agree for them, the first valid MAC address is probably assigned to some internal development device. Andrew
>> That won't work for this board since that function only checks that the >> MAC "is not 00:00:00:00:00:00, is not a multicast address, and is not >> FF:FF:FF:FF:FF:FF." The MAC address that we get on all of our boards is >> 00:17:b6:00:00:00. > > Which is a valid MAC address. So i don't see why the kernel should > reject it and use a random one. > > Maybe you should talk to Marvell about how you can program the > e-fuses. You can then use MAC addresses from A8-97-DC etc. Hi Brian, I do completely agree with Andrew. Thats not a fix to be made in linux kernel. The boards you get have zero efuse. You should work with Qualcomm on how to update mac addresses of your boards. Igor
On Thu, Dec 01, 2022 at 09:07:49AM +0100, Igor Russkikh wrote: > > >> That won't work for this board since that function only checks that the > >> MAC "is not 00:00:00:00:00:00, is not a multicast address, and is not > >> FF:FF:FF:FF:FF:FF." The MAC address that we get on all of our boards is > >> 00:17:b6:00:00:00. > > > > Which is a valid MAC address. So i don't see why the kernel should > > reject it and use a random one. > > > > Maybe you should talk to Marvell about how you can program the > > e-fuses. You can then use MAC addresses from A8-97-DC etc. > > Hi Brian, > > I do completely agree with Andrew. Thats not a fix to be made in > linux kernel. > > The boards you get have zero efuse. You should work with Qualcomm on > how to update mac addresses of your boards. OK, I'll work to track down someone within Qualcomm that can help us program the MAC address into the boards that we have. In the mean time, we'll go with the systemd unit approach to set a MAC address. Thank you all! Brian
Hi Igor
> You should work with Qualcomm on how to update mac addresses of your boards.
Why Qualcomm? I assume the fuses are part of the MAC chip? So Marvell
should have a tool to program them? Ideally, it should be part of
ethtool -E|--change-eeprom
but when i took a quick look, i could not see anything.
Andrew
Hi Andrew, >> You should work with Qualcomm on how to update mac addresses of your > boards. > > Why Qualcomm? I assume the fuses are part of the MAC chip? So Marvell > should have a tool to program them? Ideally, it should be part of > > ethtool -E|--change-eeprom > > but when i took a quick look, i could not see anything. Normal production chips should have efuses (and macs) programmed on factory. Here I assume we have preproduction/development samples which made no way through the normal process. Marvell provides normally all the tools to the customers, but these are internal, we do not make these available to the end users. Igor
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c index 06508eebb585..c9c850bbc805 100644 --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c @@ -293,7 +293,8 @@ static bool aq_nic_is_valid_ether_addr(const u8 *addr) /* Some engineering samples of Aquantia NICs are provisioned with a * partially populated MAC, which is still invalid. */ - return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0); + return !(addr[0] == 0 && addr[1] == 0 && addr[2] == 0) && + !(addr[3] == 0 && addr[4] == 0 && addr[5] == 0); } int aq_nic_ndev_register(struct aq_nic_s *self)
The Qualcomm sa8540p automotive development board (QDrive3) has an Aquantia NIC wired over PCIe. The ethernet MAC address assigned to all of the boards in our lab is 00:17:b6:00:00:00. The existing check in aq_nic_is_valid_ether_addr() only checks for leading zeros in the MAC address. Let's update the check to also check for trailing zeros in the MAC address so that a random MAC address is assigned in this case. Signed-off-by: Brian Masney <bmasney@redhat.com> --- drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)