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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

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;