[Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[Lixue Liang]

From: Lixue Liang <lianglixue@greatwall.com.cn>

In some cases, when the user uses igb_set_eeprom to modify the MAC
address to be invalid, the igb driver will fail to load. If there is no
network card device, the user must modify it to a valid MAC address by
other means.

Since the MAC address can be modified, then add a random valid MAC address
to replace the invalid MAC address in the driver can be workable, it can
continue to finish the loading, and output the relevant log reminder.

Signed-off-by: Lixue Liang <lianglixue@greatwall.com.cn>
---
Changelog:
* v4:
  - Change the igb_mian in the title to igb
  - Fix dev_err message: replace "already assigned random MAC address" 
    with "Invalid MAC address. Assigned random MAC address" 
  Suggested-by Tony <anthony.l.nguyen@intel.com>

* v3:
  - Add space after comma in commit message 
  - Correct spelling of MAC address
  Suggested-by Paul <pmenzel@molgen.mpg.de>

* v2:
  - Change memcpy to ether_addr_copy
  - Change dev_info to dev_err
  - Fix the description of the commit message
  - Change eth_random_addr to eth_hw_addr_random
  Reported-by: kernel test robot <lkp@intel.com>

 drivers/net/ethernet/intel/igb/igb_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 34b33b21e0dc..5e3b162e50ac 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3359,9 +3359,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	eth_hw_addr_set(netdev, hw->mac.addr);
 
 	if (!is_valid_ether_addr(netdev->dev_addr)) {
-		dev_err(&pdev->dev, "Invalid MAC Address\n");
-		err = -EIO;
-		goto err_eeprom;
+		eth_hw_addr_random(netdev);
+		ether_addr_copy(hw->mac.addr, netdev->dev_addr);
+		dev_err(&pdev->dev,
+			"Invalid MAC address. Assigned random MAC address\n");
 	}
 
 	igb_set_default_mac_filter(adapter);
-- 
2.27.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[Alexander H Duyck]

On Wed, 2022-06-01 at 15:04 +0000, Lixue Liang wrote:
> From: Lixue Liang <lianglixue@greatwall.com.cn>
> 
> In some cases, when the user uses igb_set_eeprom to modify the MAC
> address to be invalid, the igb driver will fail to load. If there is no
> network card device, the user must modify it to a valid MAC address by
> other means.
> 
> Since the MAC address can be modified, then add a random valid MAC address
> to replace the invalid MAC address in the driver can be workable, it can
> continue to finish the loading, and output the relevant log reminder.
> 
> Signed-off-by: Lixue Liang <lianglixue@greatwall.com.cn>
> ---
> Changelog:
> * v4:
>   - Change the igb_mian in the title to igb
>   - Fix dev_err message: replace "already assigned random MAC address" 
>     with "Invalid MAC address. Assigned random MAC address" 
>   Suggested-by Tony <anthony.l.nguyen@intel.com>
> 
> * v3:
>   - Add space after comma in commit message 
>   - Correct spelling of MAC address
>   Suggested-by Paul <pmenzel@molgen.mpg.de>
> 
> * v2:
>   - Change memcpy to ether_addr_copy
>   - Change dev_info to dev_err
>   - Fix the description of the commit message
>   - Change eth_random_addr to eth_hw_addr_random
>   Reported-by: kernel test robot <lkp@intel.com>
> 
>  drivers/net/ethernet/intel/igb/igb_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 34b33b21e0dc..5e3b162e50ac 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3359,9 +3359,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	eth_hw_addr_set(netdev, hw->mac.addr);
>  
>  	if (!is_valid_ether_addr(netdev->dev_addr)) {
> -		dev_err(&pdev->dev, "Invalid MAC Address\n");
> -		err = -EIO;
> -		goto err_eeprom;
> +		eth_hw_addr_random(netdev);
> +		ether_addr_copy(hw->mac.addr, netdev->dev_addr);
> +		dev_err(&pdev->dev,
> +			"Invalid MAC address. Assigned random MAC address\n");
>  	}
>  
>  	igb_set_default_mac_filter(adapter);

Losing the MAC address is one of the least destructive things you can
do by poking the EEPROM manually. There are settings in there for other
parts of the EEPROM for the NIC that can just as easily prevent the
driver from loading, or worse yet even prevent it from appearing on the
PCIe bus in some cases. So I don't see the user induced EEPROM
corruption as a good justification for this patch as the user shouldn't
be poking the EEPROM if they cannot do so without breaking things.

With that said I would be okay with adding this with the provision that
there is a module parameter to turn on this funcitonality. The
justification would be that the user is expecting to have a corrupted
EEPROM because they are working with some pre-production board or
uninitialized sample. This way if somebody is wanting to update the
EEPROM on a bad board they can use the kernel to do it, but they have
to explicitly enable this mode and not just have the fact that their
EEPROM is corrupted hidden as error messages don't necessarily get
peoples attention unless they are seeing some other issue.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[梁礼学]

Hi,
thank you very much for your suggestion.

As you said, the way to cause ‘Invalid MAC address’ is not only through igb_set_eeprom,
but also some pre-production or uninitialized boards.

But if set by module parameters, especially in the case of CONFIG_IGB=y,
The situation may be more troublesome, because for most users, if the system does not properly load and generate 
the network card device, they can only ask the host supplier for help.But,

(1) If the invalid mac address is caused by igb_set_eeprom, it is relatively more convenient for most operations engineers 
to change the invalid mac address to the mac address they think should be valid by ethtool, which may still be Invalid.
At this time,assigned random MAC address which is valid by the driver enables the network card driver to continue to complete the loading.
As for what you mentioned, in this case if the user does not notice that the driver had used a random mac address,
it may lead to other problems.but the fact is that if the user deliberately sets a customized mac address, 
the user should pay attention to whether the mac address is successfully changed, and also pay attention to the 
expected result after changing the mac address.When users find that the custom mac address cannot 
be successfully changed to the customized one, they can continue debugging, which is easier than looking for 
the host supplier’s support from the very first time of “Invalid MAC address”.

(2) If the invalid mac address is caused during pre-production or initialization of the board, it is even more necessary 
to use a random mac address to complete the loading of the network card, because the user only cares about whether 
the network card is loaded, not what the valid MAC address is.

And I also noticed that ixgbvef_sw_init also uses a random valid mac address to continue loading the driver when 
the address is invalid. In addition, network card drivers such as marvell, broadcom, realtek, etc., when an invalid 
MAC address is detected, it also does not directly exit the driver loading, but uses a random valid MAC address.

> 2022年6月2日 23:57，Alexander H Duyck <alexander.duyck@gmail.com> 写道：
> 
> On Wed, 2022-06-01 at 15:04 +0000, Lixue Liang wrote:
>> From: Lixue Liang <lianglixue@greatwall.com.cn>
>> 
>> In some cases, when the user uses igb_set_eeprom to modify the MAC
>> address to be invalid, the igb driver will fail to load. If there is no
>> network card device, the user must modify it to a valid MAC address by
>> other means.
>> 
>> Since the MAC address can be modified, then add a random valid MAC address
>> to replace the invalid MAC address in the driver can be workable, it can
>> continue to finish the loading, and output the relevant log reminder.
>> 
>> Signed-off-by: Lixue Liang <lianglixue@greatwall.com.cn>
>> ---
>> Changelog:
>> * v4:
>> - Change the igb_mian in the title to igb
>> - Fix dev_err message: replace "already assigned random MAC address" 
>>  with "Invalid MAC address. Assigned random MAC address" 
>> Suggested-by Tony <anthony.l.nguyen@intel.com>
>> 
>> * v3:
>> - Add space after comma in commit message 
>> - Correct spelling of MAC address
>> Suggested-by Paul <pmenzel@molgen.mpg.de>
>> 
>> * v2:
>> - Change memcpy to ether_addr_copy
>> - Change dev_info to dev_err
>> - Fix the description of the commit message
>> - Change eth_random_addr to eth_hw_addr_random
>> Reported-by: kernel test robot <lkp@intel.com>
>> 
>> drivers/net/ethernet/intel/igb/igb_main.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 34b33b21e0dc..5e3b162e50ac 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -3359,9 +3359,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> 	eth_hw_addr_set(netdev, hw->mac.addr);
>> 
>> 	if (!is_valid_ether_addr(netdev->dev_addr)) {
>> -		dev_err(&pdev->dev, "Invalid MAC Address\n");
>> -		err = -EIO;
>> -		goto err_eeprom;
>> +		eth_hw_addr_random(netdev);
>> +		ether_addr_copy(hw->mac.addr, netdev->dev_addr);
>> +		dev_err(&pdev->dev,
>> +			"Invalid MAC address. Assigned random MAC address\n");
>> 	}
>> 
>> 	igb_set_default_mac_filter(adapter);
> 
> Losing the MAC address is one of the least destructive things you can
> do by poking the EEPROM manually. There are settings in there for other
> parts of the EEPROM for the NIC that can just as easily prevent the
> driver from loading, or worse yet even prevent it from appearing on the
> PCIe bus in some cases. So I don't see the user induced EEPROM
> corruption as a good justification for this patch as the user shouldn't
> be poking the EEPROM if they cannot do so without breaking things.
> 
> With that said I would be okay with adding this with the provision that
> there is a module parameter to turn on this funcitonality. The
> justification would be that the user is expecting to have a corrupted
> EEPROM because they are working with some pre-production board or
> uninitialized sample. This way if somebody is wanting to update the
> EEPROM on a bad board they can use the kernel to do it, but they have
> to explicitly enable this mode and not just have the fact that their
> EEPROM is corrupted hidden as error messages don't necessarily get
> peoples attention unless they are seeing some other issue.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[Alexander H Duyck]

On Mon, 2022-06-06 at 22:35 +0800, 梁礼学 wrote:
> Hi,
> thank you very much for your suggestion.
> 
> As you said, the way to cause ‘Invalid MAC address’ is not only through igb_set_eeprom,
> but also some pre-production or uninitialized boards.
> 
> But if set by module parameters, especially in the case of CONFIG_IGB=y,
> The situation may be more troublesome, because for most users, if the system does not properly load and generate 
> the network card device, they can only ask the host supplier for help.But,

A module parameter can be passed as a part of the kernel command line
in the case of CONFIG_IGB=y. So it is still something that can be dealt
with via module parameters.

> (1) If the invalid mac address is caused by igb_set_eeprom, it is relatively more convenient for most operations engineers 
> to change the invalid mac address to the mac address they think should be valid by ethtool, which may still be Invalid.
> At this time,assigned random MAC address which is valid by the driver enables the network card driver to continue to complete the loading.
> As for what you mentioned, in this case if the user does not notice that the driver had used a random mac address,
> it may lead to other problems.but the fact is that if the user deliberately sets a customized mac address, 
> the user should pay attention to whether the mac address is successfully changed, and also pay attention to the 
> expected result after changing the mac address.When users find that the custom mac address cannot 
> be successfully changed to the customized one, they can continue debugging, which is easier than looking for 
> the host supplier’s support from the very first time of “Invalid MAC address”.

The problem is, having a random MAC address automatically assigned
makes it less likely to detect issues caused by (1). What I have seen
in the past is people program EEPROMs and overwrite things like a MAC
address to all 0s. This causes an obvious problem with the current
driver. If it is changed to just default to using a random MAC address
when this occurs the issue can be easily overlooked and will likely
lead to more difficulty in trying to maintain the device as it becomes
harder to identify if there may be EEPROM issues.

> (2) If the invalid mac address is caused during pre-production or initialization of the board, it is even more necessary 
> to use a random mac address to complete the loading of the network card, because the user only cares about whether 
> the network card is loaded, not what the valid MAC address is.

This isn't necessarily true. What I was getting at is that in the pre-
production case there may not be an EEPROM even loaded and as one of
the initial steps it will be necessary to put one together for the
device.

The user could either make the module parameter permenant and have it
used for every boot, or they might just have to set it once in order to
load a valid EEPROM image on the system.

> And I also noticed that ixgbvef_sw_init also uses a random valid mac address to continue loading the driver when 
> the address is invalid. In addition, network card drivers such as marvell, broadcom, realtek, etc., when an invalid 
> MAC address is detected, it also does not directly exit the driver loading, but uses a random valid MAC address.

The VF drivers assign a random MAC address due to more historic reasons
than anything else. In addition generally the use of the random MAC
address is more-or-less frowned upon. There is logic in ixgbevf that
will cause the PF to reject the VF MAC address and overwrite the MAC
address from the PF side.

As far as the other drivers they have their reasons. In many cases I
suspect the driver is intended for an embedded environment where the
user might not be able to reach the device if the NIC doesn't come up.

The igb driver is meant to typically be used in a desktop environment.
Catching a malformed MAC address is important as a part of that as it
is one of the health checks for the device. That is why I am open to
supporting it by default, but only if it is via a module parameter to
specify the behavior. Otherwise we are changing a key piece of driver
behavior and will be potentially masking EEPROM issues.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

[梁礼学]

Hi,
   Thank you very much for your analysis and explanation, What you said is indeed very reasonable, 
and lead me to further understanding of these scenarios. 

I will provide a new patch based on your suggestion.

Kind regards,

Lixue

> 2022年6月7日 01:04，Alexander H Duyck <alexander.duyck@gmail.com> 写道：
> 
> On Mon, 2022-06-06 at 22:35 +0800, 梁礼学 wrote:
>> Hi,
>> thank you very much for your suggestion.
>> 
>> As you said, the way to cause ‘Invalid MAC address’ is not only through igb_set_eeprom,
>> but also some pre-production or uninitialized boards.
>> 
>> But if set by module parameters, especially in the case of CONFIG_IGB=y,
>> The situation may be more troublesome, because for most users, if the system does not properly load and generate 
>> the network card device, they can only ask the host supplier for help.But,
> 
> A module parameter can be passed as a part of the kernel command line
> in the case of CONFIG_IGB=y. So it is still something that can be dealt
> with via module parameters.
> 
>> (1) If the invalid mac address is caused by igb_set_eeprom, it is relatively more convenient for most operations engineers 
>> to change the invalid mac address to the mac address they think should be valid by ethtool, which may still be Invalid.
>> At this time,assigned random MAC address which is valid by the driver enables the network card driver to continue to complete the loading.
>> As for what you mentioned, in this case if the user does not notice that the driver had used a random mac address,
>> it may lead to other problems.but the fact is that if the user deliberately sets a customized mac address, 
>> the user should pay attention to whether the mac address is successfully changed, and also pay attention to the 
>> expected result after changing the mac address.When users find that the custom mac address cannot 
>> be successfully changed to the customized one, they can continue debugging, which is easier than looking for 
>> the host supplier’s support from the very first time of “Invalid MAC address”.
> 
> The problem is, having a random MAC address automatically assigned
> makes it less likely to detect issues caused by (1). What I have seen
> in the past is people program EEPROMs and overwrite things like a MAC
> address to all 0s. This causes an obvious problem with the current
> driver. If it is changed to just default to using a random MAC address
> when this occurs the issue can be easily overlooked and will likely
> lead to more difficulty in trying to maintain the device as it becomes
> harder to identify if there may be EEPROM issues.
> 
>> (2) If the invalid mac address is caused during pre-production or initialization of the board, it is even more necessary 
>> to use a random mac address to complete the loading of the network card, because the user only cares about whether 
>> the network card is loaded, not what the valid MAC address is.
> 
> This isn't necessarily true. What I was getting at is that in the pre-
> production case there may not be an EEPROM even loaded and as one of
> the initial steps it will be necessary to put one together for the
> device.
> 
> The user could either make the module parameter permenant and have it
> used for every boot, or they might just have to set it once in order to
> load a valid EEPROM image on the system.
> 
>> And I also noticed that ixgbvef_sw_init also uses a random valid mac address to continue loading the driver when 
>> the address is invalid. In addition, network card drivers such as marvell, broadcom, realtek, etc., when an invalid 
>> MAC address is detected, it also does not directly exit the driver loading, but uses a random valid MAC address.
> 
> The VF drivers assign a random MAC address due to more historic reasons
> than anything else. In addition generally the use of the random MAC
> address is more-or-less frowned upon. There is logic in ixgbevf that
> will cause the PF to reject the VF MAC address and overwrite the MAC
> address from the PF side.
> 
> As far as the other drivers they have their reasons. In many cases I
> suspect the driver is intended for an embedded environment where the
> user might not be able to reach the device if the NIC doesn't come up.
> 
> The igb driver is meant to typically be used in a desktop environment.
> Catching a malformed MAC address is important as a part of that as it
> is one of the health checks for the device. That is why I am open to
> supporting it by default, but only if it is via a module parameter to
> specify the behavior. Otherwise we are changing a key piece of driver
> behavior and will be potentially masking EEPROM issues.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan