Re: [dpdk-dev] [PATCH 1/2] net/ena: upgrade HAL for new HW features

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/13/2019 8:01 PM, Michał Krawczyk wrote:
> pt., 13 gru 2019 o 17:34 Stephen Hemminger <stephen@networkplumber.org>
> napisał(a):
>>
>> On Fri, 13 Dec 2019 14:32:15 +0100
>> Michal Krawczyk <mk@semihalf.com> wrote:
>>
>>> This version of the HAL allows to use the latest HW features, like
>>> rx offsets.
>>>
>>> Driver was adjusted to the new version to fix the build.
>>>
>>> Signed-off-by: Michal Krawczyk <mk@semihalf.com>
>>> Signed-off-by: Maciej Bielski <mba@semihalf.com>
>>
>> You are mixing multiple changes into one patch.
>> This makes it harder to review (find the real bits) and also
>> harder for bisection.
>>
>> It makes sense to fix whitespace and related stuff in one
>> patch if you are fixing one function and the nearby code
>> already needed work. But please avoid larger scale change
>> put together.
>>
>> In your patch I see:
>>         - remove unnecessary whitespace
>>         - drop unnecessary inline
>>         - add missing newline in log messages
>>         - check for NULL pointer
>>
>> If possible could you preserve the per-commit updates for base
>> code, rather than one lump diff.
> 
> Hi Stephen,
> 
> We are not developing HAL (ena_com) on our own. We are getting it from the
> Amazon team and we don't have history of it's development. Tha's why we are
> upstreaming a diff between two versions of the ena_com.
> 
> Moreover this HAL is common for Linux kernel driver, FreeBSD kernel driver
> and Windows kernel driver. Because of that, we are trying to avoid to
> modify it on our side unless it's really necessary (for example, if
> compilation fails on for the DPDK).

Hi Michał,

Overall this is the common situation for most of the base code, that is why we
are a little more flexible on them and not enforcing the coding convention etc..

But as a high level rule, still please try to split the base code drop to
logical pieces as much as possible, and I am aware that this requires additional
effort.
I will proceed with this patchset, but please bare in mind this for next time.

Regards,
ferruh


> 
> In fact, we are doing changes to the PMD and our platform file
> (ena_plat_dpdk.h which is a glue) in this patch, but that is required in
> order to do not have patch which is not compiling.
> Both the PMD code and the glue code can be changed without affecting other
> platforms.
>