On 6/21/2023 12:09 PM, Ivan Malov wrote:
> 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.
>
Ack, thanks.
Let me continue with this set as it is taking into account that -rc2 is
so close, we can sync more for future patches.
> Thank you.
>
> On Mon, 19 Jun 2023, Ferruh Yigit wrote:
>
>> On 6/7/2023 2:02 PM, Ivan Malov wrote:
>>> From: Denis Pryazhennikov <denis.pryazhenni...@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.pryazhenni...@arknetworks.am>
>>> Reviewed-by: Andy Moreton <amore...@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
>>
