Re: [net 03/14] net/mlx5: SF: Fix probing active SFs during driver probe phase

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Parav Pandit <parav@nvidia.com>
Cc: Saeed Mahameed <saeed@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	Saeed Mahameed <saeedm@nvidia.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"Tariq Toukan" <tariqt@nvidia.com>, Shay Drory <shayd@nvidia.com>
Subject: Re: [net 03/14] net/mlx5: SF: Fix probing active SFs during driver probe phase
Date: Wed, 23 Nov 2022 18:44:54 +0100	[thread overview]
Message-ID: <Y35cFrOguHY5uS6Y@boxer> (raw)
In-Reply-To: <PH0PR12MB5481CCCC9A6F7649EBEAF538DC0C9@PH0PR12MB5481.namprd12.prod.outlook.com>

On Wed, Nov 23, 2022 at 05:11:46PM +0000, Parav Pandit wrote:
> 
> 
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Wednesday, November 23, 2022 9:58 AM
> > 
> > On Mon, Nov 21, 2022 at 06:25:48PM -0800, Saeed Mahameed wrote:
> > > From: Shay Drory <shayd@nvidia.com>
> > >
> > > When SF devices and SF port representors are located on different
> > > functions, unloading and reloading of SF parent driver doesn't
> > > recreate the existing SF present in the device.
> > > Fix it by querying SFs and probe active SFs during driver probe phase.
> > 
> > Maybe shed some light on how it's actually being done? Have a few words
> > that you're adding a workqueue dedicated for that etc. There is also a new
> > mutex, I was always expecting that such mechanisms get a bit of
> > explanation/justification why there is a need for its introduction.
> 
> Linux kernel has a clear coding guideline is to not explain 'how' of the code.
> It is described in section 8 of [1].

I think that you're just being picky over here

> It largely expands to commit log too as code following [1].

Who told you that it applies to commit message?

> So, commit log and comment skipped the 'how' part. :)
> 
> You likely meant to ask why workqueue is used and not 'how'.
> Having in the code comment is more useful than the commit log here.

This is your subjective opinion about the usefulness and mine was to say
that commit message could be more elaborative. Series got merged anyway
from what I see so let's cut the discussion here.

> So, Shay already explained this in the code function mlx5_sf_dev_queue_active_work() where wq is created.
> 
> Regarding mutex, there is well defined mandatory checkpatch.pl requirement to explain what does this mutex protect.

P.S. Please fix your mail client to don't go over 80 chars per line

> This is also covered in the code comment.
> 
> [1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> 
> > 
> > Not sure if including some example reproducer in here is mandatory or not
> > (and therefore splat, if any). General feeling is that commit message could be
> > beefed up.
> > 
> > >
> > > Fixes: 90d010b8634b ("net/mlx5: SF, Add auxiliary device support")
> > > Signed-off-by: Shay Drory <shayd@nvidia.com>
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> > > ---
> > >  .../ethernet/mellanox/mlx5/core/sf/dev/dev.c  | 88
> > > +++++++++++++++++++
> > >  1 file changed, 88 insertions(+)
> > >