From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2C53442D13; Wed, 21 Jun 2023 13:09:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 074A34068E; Wed, 21 Jun 2023 13:09:20 +0200 (CEST) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 7B3E94003C for ; Wed, 21 Jun 2023 13:09:18 +0200 (CEST) Received: from debian (unknown [78.109.72.170]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by agw.arknetworks.am (Postfix) with ESMTPSA id 7FC88E0924; Wed, 21 Jun 2023 15:09:17 +0400 (+04) Date: Wed, 21 Jun 2023 15:09:09 +0400 (+04) From: Ivan Malov To: Ferruh Yigit cc: dev@dpdk.org, Andrew Rybchenko , Denis Pryazhennikov , Andy Moreton Subject: Re: [PATCH v4 03/34] common/sfc_efx/base: add API to list HW tables In-Reply-To: <1170a8ae-f3f6-88b1-c48c-ce4bb74710bc@amd.com> Message-ID: <7dc198b0-1b9d-3eb5-4a69-933ded9af0d9@arknetworks.am> References: <20230601195538.8265-1-ivan.malov@arknetworks.am> <20230607130245.8048-1-ivan.malov@arknetworks.am> <20230607130245.8048-4-ivan.malov@arknetworks.am> <1170a8ae-f3f6-88b1-c48c-ce4bb74710bc@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >> >> 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 >> Reviewed-by: Andy Moreton >> > > 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 >