net: atlantic: fix check for invalid ethernet addresses

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

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