netdev - Re: [PATCH net-next 0/4] mlx5: Create build configuration options

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALzJLG8dh0TZPotrk-Xp6SZ=2UnjY2-C3TEQNxXDER5H8iwiQw@mail.gmail.com>
Date:   Sat, 28 Jan 2017 13:38:21 +0200
From:   Saeed Mahameed <saeedm@....mellanox.co.il>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Saeed Mahameed <saeedm@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH net-next 0/4] mlx5: Create build configuration options

On Fri, Jan 27, 2017 at 8:13 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Fri, Jan 27, 2017 at 9:58 AM, Saeed Mahameed
> <saeedm@....mellanox.co.il> wrote:
>> On Fri, Jan 27, 2017 at 1:32 AM, Tom Herbert <tom@...bertland.com> wrote:
>>> This patchset creates configuration options for sriov, vxlan, eswitch,
>>> and tc features in the mlx5 driver. The purpose of this is to allow not
>>> building these features. These features are optional advanced features
>>> that are not required for a core Ethernet driver. A user can disable
>>> these features which resuces the amount of code in the driver. Disabling
>>> these features (and DCB) reduces the size of mlx5_core.o by about 16%.
>>> This is also can reduce the complexity of backport and rebases since
>>> user would no longer need to worry about dependencies with the rest of
>>> the kernel that features which might not be of any interest to a user
>>> may bring in.
>>>
>>> Tested: Build and ran the driver with all features enabled (the default)
>>> and with none enabled (including DCB). Did not see any issues. I did
>>> not explicity test operation of ayy of features in the list.
>>>
>>
>> Basically I am not against this kind of change, infact i am with it,
>> although I would have done some restructuring in the driver before i
>> did such change ;), filling the code with ifdefs is not a neat thing.
>>
> If you wish, please take this as an RFC and feel free to structure the
> code the right way. I think the intent is clear enough and looks like
> davem isn't going to allow the directory restructuring so something
> like this seems to be the best course of action now.
>

Right.

>> I agree this will simplify backporting and provide some kind of
>> feature separation inside the driver.
>> But this will also increase the testing matrix we need to cover and
>> increase the likelihood of kbuild breaks by an order of magnitude.
>>
> The testing matrix already exploded with the proliferation of
> supported features. If anything this reduces the test matrix problem.
> For instance, if we make a change to the core driver and functionality
> properly isolated there is a much better chance that this won't affect
> peripheral functionality and vice versa. It is just not feasible for
> us to test every combination of NIC features for every change being
> made.
>

Yes for isolated features, but for base functionality, we need to test
it with all new device specific kconfig combinations on every patch!
since a misplaced code inside or outside the correct ifdef
can easily go unnoticed and break functionality.

>> One more thing, do we really need a device specific flag per feature
>> per vendor per device?  can't we just use the same kconfig flag for
>> all drivers and if there is a more generic system wide flag that
>> covers the same feature
>> can't we just use it, for instance instead of
>> CONFIG_<DRIVER_NAME>_SRIOV why not use already existing CONFIG_PCI_IOV
>> for all drivers ?
>>
> That sounds good to me. We already have CONFIG_RFS_ACCEL and others
> that do that.
>
> Tom
>
>> Saeed.
>>
>>>
>>>
>>> Tom Herbert (4):
>>>   mlx5: Make building eswitch configurable
>>>   mlx5: Make building SR-IOV configurable
>>>   mlx5: Make building tc hardware offload configurable
>>>   mlx5: Make building vxlan hardware offload configurable
>>>
>>>  drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  35 ++++++
>>>  drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  16 ++-
>>>  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 129 ++++++++++++++++------
>>>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c   |  39 +++++--
>>>  drivers/net/ethernet/mellanox/mlx5/core/eq.c      |   4 +-
>>>  drivers/net/ethernet/mellanox/mlx5/core/lag.c     |   2 +
>>>  drivers/net/ethernet/mellanox/mlx5/core/main.c    |  32 ++++--
>>>  drivers/net/ethernet/mellanox/mlx5/core/sriov.c   |   6 +-
>>>  8 files changed, 205 insertions(+), 58 deletions(-)
>>>
>>> --
>>> 2.9.3
>>>