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

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

[Lixue Liang]

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

Add the module parameter "allow_invalid_mac_address" to control the
behavior. When set to true, a random MAC address is assigned, and the
driver can be loaded, allowing the user to correct the invalid MAC address.

Signed-off-by: Lixue Liang <lianglixue@greatwall.com.cn>
---
Changelog:
* v6:
  - Modify commit messages and naming of module parameters
Suggested-by Paul <pmenzel@molgen.mpg.de>
* v5:
  - Through the setting of module parameters, it is allowed to complete
    the loading of the igb network card driver with an invalid MAC address.
Suggested-by <alexander.duyck@gmail.com>
* 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 | 16 +++++++++++++---
 1 file changed, 13 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..b61f216331da 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -238,8 +238,11 @@ MODULE_LICENSE("GPL v2");
 
 #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
 static int debug = -1;
+static bool allow_invalid_mac_address;
 module_param(debug, int, 0);
+module_param(allow_invalid_mac_address, bool, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+MODULE_PARM_DESC(allow_invalid_mac_address, "Allow NIC driver to be loaded with invalid MAC address");
 
 struct igb_reg_info {
 	u32 ofs;
@@ -3359,9 +3362,16 @@ 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;
+		if (!allow_invalid_mac_address) {
+			dev_err(&pdev->dev, "Invalid MAC address\n");
+			err = -EIO;
+			goto err_eeprom;
+		} else {
+			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 v6] igb: Assign random MAC address instead of fail in case of invalid one

[Tony Nguyen]

On 6/9/2022 7:39 PM, Lixue Liang wrote:
> From: Lixue Liang <lianglixue@greatwall.com.cn>
> 
> Add the module parameter "allow_invalid_mac_address" to control the

netdev maintainers:

I know the use of module parameters is extremely frowned upon. Is this a 
use/exception that would be allowed?

> behavior. When set to true, a random MAC address is assigned, and the
> driver can be loaded, allowing the user to correct the invalid MAC address.
> 
> Signed-off-by: Lixue Liang <lianglixue@greatwall.com.cn>
> ---
> Changelog:
> * v6:
>    - Modify commit messages and naming of module parameters
> Suggested-by Paul <pmenzel@molgen.mpg.de>
> * v5:
>    - Through the setting of module parameters, it is allowed to complete
>      the loading of the igb network card driver with an invalid MAC address.
> Suggested-by <alexander.duyck@gmail.com>
> * 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 | 16 +++++++++++++---
>   1 file changed, 13 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..b61f216331da 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -238,8 +238,11 @@ MODULE_LICENSE("GPL v2");
>   
>   #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
>   static int debug = -1;
> +static bool allow_invalid_mac_address;
>   module_param(debug, int, 0);
> +module_param(allow_invalid_mac_address, bool, 0);
>   MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> +MODULE_PARM_DESC(allow_invalid_mac_address, "Allow NIC driver to be loaded with invalid MAC address");

Lixue:
If the maintainers are willing to take it, I believe the convention is 
to group each parameter together vs mixing them.

e.g.

  static int debug = -1;
  module_param(debug, int, 0);
  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");

+static bool allow_invalid_mac_address;
+module_param(allow_invalid_mac_address, bool, 0);
+MODULE_PARM_DESC(allow_invalid_mac_address, "Allow NIC driver to be 
loaded with invalid MAC address");

Thanks,
Tony

>   struct igb_reg_info {
>   	u32 ofs;

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

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

[Jakub Kicinski]

On Tue, 14 Jun 2022 12:09:02 -0700 Tony Nguyen wrote:
> > Add the module parameter "allow_invalid_mac_address" to control the  
> 
> netdev maintainers:
> 
> I know the use of module parameters is extremely frowned upon. Is this a 
> use/exception that would be allowed?

I think so, I don't see a different way here.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[梁礼学]

As alexander.duyck@gmail.com said, "allow_unsupported_sfp" in ixgbe_main.c, 
and I also noticed "eeprom_bad_csum_allow" in e100.c, are all in the form of module parameters.

If the invalid MAC address is automatically replaced with a random MAC address, 
other problems caused by the random MAC address may be difficult to debug. 
Using module parameters can make it easy for users to correct invalid MAC addresses 
without causing the above problems

> 2022年6月15日 09:19，Jakub Kicinski <kuba@kernel.org> 写道：
> 
> On Tue, 14 Jun 2022 12:09:02 -0700 Tony Nguyen wrote:
>>> Add the module parameter "allow_invalid_mac_address" to control the  
>> 
>> netdev maintainers:
>> 
>> I know the use of module parameters is extremely frowned upon. Is this a 
>> use/exception that would be allowed?
> 
> I think so, I don't see a different way here.

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