Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221213074726.51756-1-lianglixuehao@126.com> <Y5l5pUKBW9DvHJAW@unreal>
 <20221214085106.42a88df1@kernel.org> <Y5obql8TVeYEsRw8@unreal>
 <20221214125016.5a23c32a@kernel.org> <4576ee79-1040-1998-6d91-7ef836ae123b@gmail.com>
In-Reply-To: <4576ee79-1040-1998-6d91-7ef836ae123b@gmail.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Wed, 14 Dec 2022 15:17:34 -0800
Message-ID: <CAKgT0UdZUmFD8qYdF6k47ZmRgB0jS-graOYgB5Se+y+4fKqW8w@mail.gmail.com>
Subject: Re: [PATCH v7] igb: Assign random MAC address instead of fail in case
 of invalid one
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        Leon Romanovsky <leon@kernel.org>,
        Lixue Liang <lianglixuehao@126.com>,
        anthony.l.nguyen@intel.com, linux-kernel@vger.kernel.org,
        jesse.brandeburg@intel.com, davem@davemloft.net,
        edumazet@google.com, pabeni@redhat.com, netdev@vger.kernel.org,
        lianglixue@greatwall.com.cn
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 14, 2022 at 1:43 PM Heiner Kallweit <hkallweit1@gmail.com> 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.