From: Lixue Liang <[email protected]>
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 <[email protected]>
---
Changelog:
* v7:
- To group each parameter together
Suggested-by Tony Nguyen <[email protected]>
* v6:
- Modify commit messages and naming of module parameters
- [PATCH v6] link:
https://lore.kernel.org/netdev/[email protected]/
Suggested-by Paul <[email protected]>
* 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.
- [PATCH v5] link:
https://lore.kernel.org/netdev/[email protected]/
Suggested-by <[email protected]>
* 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"
- [PATCH v4] link:
https://lore.kernel.org/netdev/[email protected]/
Suggested-by Tony <[email protected]>
* v3:
- Add space after comma in commit message
- Correct spelling of MAC address
- [PATCH v3] link:
https://lore.kernel.org/netdev/[email protected]/
Suggested-by Paul <[email protected]>
* 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
- [PATCH v2] link:
https://lore.kernel.org/netdev/[email protected]/
Reported-by: kernel test robot <[email protected]>
drivers/net/ethernet/intel/igb/igb_main.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f8e32833226c..8ff0c698383c 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -241,6 +241,10 @@ 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");
+
struct igb_reg_info {
u32 ofs;
char *name;
@@ -3358,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
On 12/12/2022 11:47 PM, Lixue Liang wrote:
> From: Lixue Liang <[email protected]>
>
> 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.
Please include Intel Wired LAN, [email protected], for
Intel ethernet driver patches.
> Signed-off-by: Lixue Liang <[email protected]>
> ---
> Changelog:
> * v7:
> - To group each parameter together
> Suggested-by Tony Nguyen <[email protected]>
> * v6:
> - Modify commit messages and naming of module parameters
> - [PATCH v6] link:
> https://lore.kernel.org/netdev/[email protected]/
> Suggested-by Paul <[email protected]>
> * 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.
> - [PATCH v5] link:
> https://lore.kernel.org/netdev/[email protected]/
> Suggested-by <[email protected]>
> * 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"
> - [PATCH v4] link:
> https://lore.kernel.org/netdev/[email protected]/
> Suggested-by Tony <[email protected]>
>
> * v3:
> - Add space after comma in commit message
> - Correct spelling of MAC address
> - [PATCH v3] link:
> https://lore.kernel.org/netdev/[email protected]/
> Suggested-by Paul <[email protected]>
>
> * 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
> - [PATCH v2] link:
> https://lore.kernel.org/netdev/[email protected]/
> Reported-by: kernel test robot <[email protected]>
>
> drivers/net/ethernet/intel/igb/igb_main.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index f8e32833226c..8ff0c698383c 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -241,6 +241,10 @@ 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");
> +
> struct igb_reg_info {
> u32 ofs;
> char *name;
> @@ -3358,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);
On Tue, Dec 13, 2022 at 07:47:26AM +0000, Lixue Liang wrote:
> From: Lixue Liang <[email protected]>
>
> 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 <[email protected]>
I'm amused that we are in v7 version of module parameter.
NAK to any module driver parameter. If it is applicable to all drivers,
please find a way to configure it to more user-friendly. If it is not,
try to do the same as other drivers do.
Thanks
hi Leon,
Currently this module parameter is only used for igb.
Is the more user-friendly way not to use module parameters?
just like other network card drivers, use random MAC.
As I submitted [patch v3] before, link:
https://lore.kernel.org/netdev/[email protected]/
Or, can you suggest me how to do it more reasonable?
Thanks
> 2022年12月14日 15:22，Leon Romanovsky <[email protected]> 写道：
>
> On Tue, Dec 13, 2022 at 07:47:26AM +0000, Lixue Liang wrote:
>> From: Lixue Liang <[email protected]>
>>
>> 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 <[email protected]>
>
>
> I'm amused that we are in v7 version of module parameter.
>
> NAK to any module driver parameter. If it is applicable to all drivers,
> please find a way to configure it to more user-friendly. If it is not,
> try to do the same as other drivers do.
>
> Thanks
On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> NAK to any module driver parameter. If it is applicable to all drivers,
> please find a way to configure it to more user-friendly. If it is not,
> try to do the same as other drivers do.
I think this one may be fine. Configuration which has to be set before
device probing can't really be per-device.
On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > NAK to any module driver parameter. If it is applicable to all drivers,
> > please find a way to configure it to more user-friendly. If it is not,
> > try to do the same as other drivers do.
>
> I think this one may be fine. Configuration which has to be set before
> device probing can't really be per-device.
This configuration can be different between multiple devices
which use same igb module. Module parameters doesn't allow such
separation.
Also, as a user, I despise random module parameters which I need
to set after every HW update/replacement.
Thanks
On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> > On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > > NAK to any module driver parameter. If it is applicable to all drivers,
> > > please find a way to configure it to more user-friendly. If it is not,
> > > try to do the same as other drivers do.
> >
> > I think this one may be fine. Configuration which has to be set before
> > device probing can't really be per-device.
>
> This configuration can be different between multiple devices
> which use same igb module. Module parameters doesn't allow such
> separation.
Configuration of the device, sure, but this module param is more of
a system policy. BTW the name of the param is not great, we're allowing
the use of random address, not an invalid address.
> Also, as a user, I despise random module parameters which I need
> to set after every HW update/replacement.
Agreed, IIUC the concern was alerting users to incorrect EEPROM values.
I thought falling back to a random address was relatively common, but
I haven't done the research.
On 14.12.2022 21:50, Jakub Kicinski wrote:
> On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
>> On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
>>> On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
>>>> NAK to any module driver parameter. If it is applicable to all drivers,
>>>> please find a way to configure it to more user-friendly. If it is not,
>>>> try to do the same as other drivers do.
>>>
>>> I think this one may be fine. Configuration which has to be set before
>>> device probing can't really be per-device.
>>
>> This configuration can be different between multiple devices
>> which use same igb module. Module parameters doesn't allow such
>> separation.
>
> Configuration of the device, sure, but this module param is more of
> a system policy. BTW the name of the param is not great, we're allowing
> the use of random address, not an invalid address.
>
>> Also, as a user, I despise random module parameters which I need
>> to set after every HW update/replacement.
>
> Agreed, IIUC the concern was alerting users to incorrect EEPROM values.
> I thought falling back to a random address was relatively common, but
> I haven't done the research.
My 2ct, because I once added the fallback to a random MAC address to r8169:
Question is whether there's any scenario where you would prefer bailing out
in case of invalid MAC address over assigning a random MAC address (that the
user may manually change later) plus a warning.
I'm not aware of such a scenario. Therefore I decided to hardcode this
fallback in the driver.
On Wed, Dec 14, 2022 at 1:43 PM Heiner Kallweit <[email protected]> wrote:
>
> On 14.12.2022 21:50, Jakub Kicinski wrote:
> > On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> >> On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> >>> On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> >>>> NAK to any module driver parameter. If it is applicable to all drivers,
> >>>> please find a way to configure it to more user-friendly. If it is not,
> >>>> try to do the same as other drivers do.
> >>>
> >>> I think this one may be fine. Configuration which has to be set before
> >>> device probing can't really be per-device.
> >>
> >> This configuration can be different between multiple devices
> >> which use same igb module. Module parameters doesn't allow such
> >> separation.
> >
> > Configuration of the device, sure, but this module param is more of
> > a system policy. BTW the name of the param is not great, we're allowing
> > the use of random address, not an invalid address.
> >
> >> Also, as a user, I despise random module parameters which I need
> >> to set after every HW update/replacement.
> >
> > Agreed, IIUC the concern was alerting users to incorrect EEPROM values.
> > I thought falling back to a random address was relatively common, but
> > I haven't done the research.
>
> My 2ct, because I once added the fallback to a random MAC address to r8169:
> Question is whether there's any scenario where you would prefer bailing out
> in case of invalid MAC address over assigning a random MAC address (that the
> user may manually change later) plus a warning.
> I'm not aware of such a scenario. Therefore I decided to hardcode this
> fallback in the driver.
I've seen issues with such a solution in the past. In addition it is
very easy for the user to miss the warning and when the EEPROM is
corrupted on the Intel NICs it has other side effects. That is one of
the reasons why the MAC address is used as a requirement for us to
spawn a netdev.
As far as the discussion for module parameter vs something else. The
problem with the driver is that it is monolithic so it isn't as if
there is a devlink interface to configure a per-network parameter
before the network portion is loaded. The module parameter is a
compromise that should only be used to enable the workaround so that
the driver can be loaded so that the EEPROM can then be edited. If
anything, tying the EEPROM to ethtool is the issue. If somebody wants
to look at providing an option to edit the EEPROM via devlink that
would solve the issue as then the driver could expose the devlink
interface to edit the EEPROM without having to allocate and register a
netdev.
The module parameter method does bring some inconvenience to the user,
especially the parameter needs to be specified when the module is loaded.
But as alexander said, if the net device is not successfully registered,
the user has no chance to modify the invalid MAC address in the current EEPROM.
At present, the read/write of EEPROM is bundled with the net driver.
I am not sure if there is any other way to complete the modification of EEPROM
independently of the network driver;
Is it necessary to bind the registration of net device to the judgment of invalid MAC?
I personally think that MAC configuration is not the capability or function of the device,
this should not affect the registration of the device;
Can the invalid MAC be judged in the up stage of the network device?
In this way, the net driver can continue to be loaded successfully,
and the MAC can be changed using ethtool, and it will not increase the difficulty of debugging for users.
Thanks
> 2022年12月15日 07:17，Alexander Duyck <[email protected]> 写道：
>
> On Wed, Dec 14, 2022 at 1:43 PM Heiner Kallweit <[email protected]> wrote:
>>
>> On 14.12.2022 21:50, Jakub Kicinski wrote:
>>> On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
>>>> On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
>>>>> On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
>>>>>> NAK to any module driver parameter. If it is applicable to all drivers,
>>>>>> please find a way to configure it to more user-friendly. If it is not,
>>>>>> try to do the same as other drivers do.
>>>>>
>>>>> I think this one may be fine. Configuration which has to be set before
>>>>> device probing can't really be per-device.
>>>>
>>>> This configuration can be different between multiple devices
>>>> which use same igb module. Module parameters doesn't allow such
>>>> separation.
>>>
>>> Configuration of the device, sure, but this module param is more of
>>> a system policy. BTW the name of the param is not great, we're allowing
>>> the use of random address, not an invalid address.
>>>
>>>> Also, as a user, I despise random module parameters which I need
>>>> to set after every HW update/replacement.
>>>
>>> Agreed, IIUC the concern was alerting users to incorrect EEPROM values.
>>> I thought falling back to a random address was relatively common, but
>>> I haven't done the research.
>>
>> My 2ct, because I once added the fallback to a random MAC address to r8169:
>> Question is whether there's any scenario where you would prefer bailing out
>> in case of invalid MAC address over assigning a random MAC address (that the
>> user may manually change later) plus a warning.
>> I'm not aware of such a scenario. Therefore I decided to hardcode this
>> fallback in the driver.
>
> I've seen issues with such a solution in the past. In addition it is
> very easy for the user to miss the warning and when the EEPROM is
> corrupted on the Intel NICs it has other side effects. That is one of
> the reasons why the MAC address is used as a requirement for us to
> spawn a netdev.
>
> As far as the discussion for module parameter vs something else. The
> problem with the driver is that it is monolithic so it isn't as if
> there is a devlink interface to configure a per-network parameter
> before the network portion is loaded. The module parameter is a
> compromise that should only be used to enable the workaround so that
> the driver can be loaded so that the EEPROM can then be edited. If
> anything, tying the EEPROM to ethtool is the issue. If somebody wants
> to look at providing an option to edit the EEPROM via devlink that
> would solve the issue as then the driver could expose the devlink
> interface to edit the EEPROM without having to allocate and register a
> netdev.
On Thu, 2022-12-15 at 11:24 +0800, 梁礼学 wrote:
> The module parameter method does bring some inconvenience to the user,
> especially the parameter needs to be specified when the module is loaded.
> But as alexander said, if the net device is not successfully registered,
> the user has no chance to modify the invalid MAC address in the current EEPROM.
> At present, the read/write of EEPROM is bundled with the net driver.
> I am not sure if there is any other way to complete the modification of EEPROM
> independently of the network driver;
>
> Is it necessary to bind the registration of net device to the judgment of invalid MAC?
> I personally think that MAC configuration is not the capability or function of the device,
> this should not affect the registration of the device;
> Can the invalid MAC be judged in the up stage of the network device?
> In this way, the net driver can continue to be loaded successfully,
> and the MAC can be changed using ethtool, and it will not increase the difficulty of debugging for users.
>
> Thanks
The problem is that the decision all depends on use case. For a small
embedded device or desktop system it probably doesn't care as it will
always just default to DHCP most likely anyway so it doesn't really
care about maintaining a static MAC configuration.
However the igb device covers a range of products including workstation
and some server. The issue is that changing the MAC address on server
setups can trigger significant issues depending on the setup as things
like static IP reservations can be lost due to either a static DHCP
reservation or sysconfig potentially being lost. I know on older redhat
systems random MACs would lead to a buildup up sysconfig files as it
would generate a new one every time the MAC changed. It is one of the
reasons why Intel stopped using random MAC on VFs if I recall
correctly.
Lastly one thing that occurs to me is that there is support for
providing a MAC address via eth_platform_get_mac_address() as some of
the smaller embedded parts have an option to run without an EEPROM. I
wonder if there isn't a way to work around this by providing a
devicetree overlay on problematic systems.
On Wed, Dec 14, 2022 at 12:50:16PM -0800, Jakub Kicinski wrote:
> On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> > On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> > > On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > > > NAK to any module driver parameter. If it is applicable to all drivers,
> > > > please find a way to configure it to more user-friendly. If it is not,
> > > > try to do the same as other drivers do.
> > >
> > > I think this one may be fine. Configuration which has to be set before
> > > device probing can't really be per-device.
> >
> > This configuration can be different between multiple devices
> > which use same igb module. Module parameters doesn't allow such
> > separation.
>
> Configuration of the device, sure, but this module param is more of
> a system policy.
And system policy should be controlled by userspace and applicable to as
much as possible NICs, without custom module parameters.
I would imagine global (at the beginning, till someone comes forward and
requests this parameter be per-device) to whole stack parameter with policies:
* Be strict - fail if mac is not valid
* Fallback to random
* Random only ???
Thanks
On Sun, Dec 18, 2022 at 12:41 AM Leon Romanovsky <[email protected]> wrote:
>
> On Wed, Dec 14, 2022 at 12:50:16PM -0800, Jakub Kicinski wrote:
> > On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> > > On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> > > > On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > > > > NAK to any module driver parameter. If it is applicable to all drivers,
> > > > > please find a way to configure it to more user-friendly. If it is not,
> > > > > try to do the same as other drivers do.
> > > >
> > > > I think this one may be fine. Configuration which has to be set before
> > > > device probing can't really be per-device.
> > >
> > > This configuration can be different between multiple devices
> > > which use same igb module. Module parameters doesn't allow such
> > > separation.
> >
> > Configuration of the device, sure, but this module param is more of
> > a system policy.
>
> And system policy should be controlled by userspace and applicable to as
> much as possible NICs, without custom module parameters.
>
> I would imagine global (at the beginning, till someone comes forward and
> requests this parameter be per-device) to whole stack parameter with policies:
> * Be strict - fail if mac is not valid
> * Fallback to random
> * Random only ???
>
> Thanks
So are you suggesting you would rather see something like this as a
sysctl then? Maybe something like net.core.netdev_mac_behavior where
we have some enum with a predetermined set of behaviors available? I
would be fine with us making this a global policy if that is the route
we want to go. It would just be a matter of adding the sysctl and an
accessor so that drivers can determine if it is set or not.
On 12/14/2022 3:17 PM, Alexander Duyck wrote:
> On Wed, Dec 14, 2022 at 1:43 PM Heiner Kallweit <[email protected]> wrote:
> As far as the discussion for module parameter vs something else. The
> problem with the driver is that it is monolithic so it isn't as if
> there is a devlink interface to configure a per-network parameter
> before the network portion is loaded. The module parameter is a
> compromise that should only be used to enable the workaround so that
> the driver can be loaded so that the EEPROM can then be edited. If
> anything, tying the EEPROM to ethtool is the issue. If somebody wants
> to look at providing an option to edit the EEPROM via devlink that
> would solve the issue as then the driver could expose the devlink
> interface to edit the EEPROM without having to allocate and register a
> netdev.
Right. Since igb only has netdev as the way to change the MAC or edit
the EEPROM, there is no way to fix this if the driver doesn't load.
Ideally the driver would instead still be able to register and expose an
interface (devlink?) that does not depend on the MAC, and when the MAC
is invalid it just would not register a netdevice. Another option might
me some method of a netdev to say "I don't have a valid MAC, so don't
allow me to do traffic"?
A global sysctl policy as discussed elsewhere in the thread also seems
reasonable to me, given that modifying the driver to expose devlink is a
lot more work. Of course we'd also need to check and get other drivers
to use the same global policy as well.
Thanks,
Jake
On Mon, Dec 19, 2022 at 07:30:45AM -0800, Alexander Duyck wrote:
> On Sun, Dec 18, 2022 at 12:41 AM Leon Romanovsky <[email protected]> wrote:
> >
> > On Wed, Dec 14, 2022 at 12:50:16PM -0800, Jakub Kicinski wrote:
> > > On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> > > > On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> > > > > On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > > > > > NAK to any module driver parameter. If it is applicable to all drivers,
> > > > > > please find a way to configure it to more user-friendly. If it is not,
> > > > > > try to do the same as other drivers do.
> > > > >
> > > > > I think this one may be fine. Configuration which has to be set before
> > > > > device probing can't really be per-device.
> > > >
> > > > This configuration can be different between multiple devices
> > > > which use same igb module. Module parameters doesn't allow such
> > > > separation.
> > >
> > > Configuration of the device, sure, but this module param is more of
> > > a system policy.
> >
> > And system policy should be controlled by userspace and applicable to as
> > much as possible NICs, without custom module parameters.
> >
> > I would imagine global (at the beginning, till someone comes forward and
> > requests this parameter be per-device) to whole stack parameter with policies:
> > * Be strict - fail if mac is not valid
> > * Fallback to random
> > * Random only ???
> >
> > Thanks
>
> So are you suggesting you would rather see something like this as a
> sysctl then? Maybe something like net.core.netdev_mac_behavior where
> we have some enum with a predetermined set of behaviors available? I
> would be fine with us making this a global policy if that is the route
> we want to go. It would just be a matter of adding the sysctl and an
> accessor so that drivers can determine if it is set or not.
Something like that and maybe convert drivers and/or to honor this policy.
Thanks
On Wed, Dec 28, 2022 at 11:15:01AM +0200, Leon Romanovsky wrote:
> On Mon, Dec 19, 2022 at 07:30:45AM -0800, Alexander Duyck wrote:
> > On Sun, Dec 18, 2022 at 12:41 AM Leon Romanovsky <[email protected]> wrote:
> > >
> > > On Wed, Dec 14, 2022 at 12:50:16PM -0800, Jakub Kicinski wrote:
> > > > On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
> > > > > On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
> > > > > > On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
> > > > > > > NAK to any module driver parameter. If it is applicable to all drivers,
> > > > > > > please find a way to configure it to more user-friendly. If it is not,
> > > > > > > try to do the same as other drivers do.
> > > > > >
> > > > > > I think this one may be fine. Configuration which has to be set before
> > > > > > device probing can't really be per-device.
> > > > >
> > > > > This configuration can be different between multiple devices
> > > > > which use same igb module. Module parameters doesn't allow such
> > > > > separation.
> > > >
> > > > Configuration of the device, sure, but this module param is more of
> > > > a system policy.
> > >
> > > And system policy should be controlled by userspace and applicable to as
> > > much as possible NICs, without custom module parameters.
> > >
> > > I would imagine global (at the beginning, till someone comes forward and
> > > requests this parameter be per-device) to whole stack parameter with policies:
> > > * Be strict - fail if mac is not valid
> > > * Fallback to random
> > > * Random only ???
> > >
> > > Thanks
> >
> > So are you suggesting you would rather see something like this as a
> > sysctl then? Maybe something like net.core.netdev_mac_behavior where
> > we have some enum with a predetermined set of behaviors available? I
> > would be fine with us making this a global policy if that is the route
> > we want to go. It would just be a matter of adding the sysctl and an
> > accessor so that drivers can determine if it is set or not.
>
> Something like that and maybe convert drivers and/or to honor this policy.
Converting drivers is very unlikely to happen. There are over 240
calls to register_netdev() under drivers/net/ethernet. Who has the
time to add such code to so many drivers?
What many drivers do is called one of platform_get_ethdev_addr(),
of_get_mac_address(), or device_get_ethdev_address() etc, which will
look around DT, ACPI and maybe in NVMEM, etc. It is not user space
controllable policy, but most drivers fall back to a random MAC
address, and a warning, if no fixed MAC addresses can be found.
So i would recommend doing what most drivers do, if everything else
fails, us a random address.
Andrew