Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Or Gerlitz <gerlitz.or@gmail.com>]

On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>
>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>> wrote:
>>>
>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>
>>>>
>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>>> compile without offload support enabled.
>>
>>
>>>> I am pretty sure we can do that exercise you're up to without any
>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>> (en_rep.c), I'll take that with Saeed.
>>
>>
>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>> keep
>>> it readable. I did it gradually to make sure I didn't break anything and
>>> to
>>> allow for it to be bisected in case something did break. If we can move
>>> out
>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>> disabled that way that would be great, but I like to limit the number of
>>> #ifdefs we add to the actual code.
>>
>>
>> FWIW (see below), squashing your seven patches to one resulted in a
>> fairly simple/clear
>> patch, so if we go that way, no need to have seven commits just for this
>> piece.
>
>
> Squashing patches into jumbo patches is inherently broken and bad coding
> practice! It makes it way more complicated to debug and bisect in case a
> minor detail broke in the process.

Not that pure LOC ##-s is the only/deep measurement, but your overall
changes in the the seven patch series account to:

 5 files changed, 94 insertions(+), 3 deletions(-)

and by no mean this is jumbo or inherently broken and bad coded, so
please slow down please, I looked with care on the resulted patch and
said it's basically ok.


>> Re SRIOV, I don't think it would be correct to break the support info few
>> CONFIG directives. If we want to allow someone to build the driver w.o
>> SRIOV that's fine, but I don't think we should further go down and disable
>> some of the SRIOV sub-modes.

>> Re TC offload support, that's make sense.

> OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.

I think Saeed wants us to conduct that exercise, let me check with him
and get back to you