[PATCH v4 03/34] common/sfc_efx/base: add API to list HW tables

[Ivan Malov]

Hi Ferruh,

Thank you so much for your review notes. You suggested to squash
some pairs of "common/sfc_base/efx" and "net/sfc" patches, so
that logical changes would be unified. I see your point.

On the other hand, however, doing so would be rather unusual
from our internal process standpoint. Typically, we stick
with the idea that splitting patches into smaller ones is
better, as it is much easier to reason about and debug
smaller changesets rather than bigger ones.

The thing is, the DPDK driver is not the only one based on
the common code ("libefx"), that is, we got used to
keeping things separate, even despite some of them
beging logically connected. I apologise in case
my explanation is still vague.

If the reader comes across a libefx patch in one of the
other libefx-based projects and wants to search for it
in the other projects (DPDK), it is much easier for
them to find what they need provided that the patch
exists in DPDK in the same format, as a separate
change set with (almost) the same commit summary.

In other words, there are pros and cons to squashing
things, well, at least in this particular series,
which is rather big and complicated.

How about we retain the series as it is, in its current state
this time? We hope to adopt the suggested ("bigger logical
patches") next time, in our future work. What do you think?

We would discuss this internally, with our team, and come
up with the new approach for us all to structure future
patch sets, for some other features yet to be supported.

Thank you.

On Mon, 19 Jun 2023, Ferruh Yigit wrote:

> On 6/7/2023 2:02 PM, Ivan Malov wrote:
>> From: Denis Pryazhennikov <denis.pryazhennikov at arknetworks.am>
>>
>> New MCDI Table Access API allows management of
>> the HW tables' content.
>> This part of API helps to list all supported tables.
>> In the near future, only the CT table is planned
>> to be used, so only one identifier for this table
>> was added to the efx.
>> New table IDs will be added as needed.
>>
>> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov at arknetworks.am>
>> Reviewed-by: Andy Moreton <amoreton at xilinx.com>
>>
>
> This patch adds a function to the base code, but it is disconnected from
> the context.
> In the future if someone looks this function in git log, there is no
> easy way to see why this function added and where/how it is used at time
> it is added etc..
>
> So, instead of making commit per function, can you please split commits
> based on functionality/logic?
>
> Please combine the commit that new function and commit where new
> function is used to single commit, making a commit per feature?
>
>
> If you are concerned about checkpatch warnings related to the component
> (like common/sfc_efx/base), please ignore it for the case when a feature
> is distributed into multiple components, and feel free to use most
> appropriate component name, I assume it will be driver component
> (net/sfc) most of the times.
>
>
> There is apply errors on CI, which prevents CI checks, can you please
> rebase set on top of latest head?
>
>
> Btw, we call 'API' to end-user facing functions, that user directly
> call, for this context better to call it 'function', but after patches
> merged probably you won't need it at all.
>
> Thanks,
> Ferruh
>