linux-kernel - Re: [PATCH v7 11/15] scsi: fnic: Modify fnic interfaces to use FDLS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <232ddea0-3c7b-45c3-8851-566118a893e3@stanley.mountain>
Date: Thu, 9 Jan 2025 11:23:58 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
Cc: "Sesidhar Baddela (sebaddel)" <sebaddel@...co.com>,
	"Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>,
	"Dhanraj Jhawar (djhawar)" <djhawar@...co.com>,
	"Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>,
	"Masa Kai (mkai2)" <mkai2@...co.com>,
	"Satish Kharat (satishkh)" <satishkh@...co.com>,
	"Arun Easi (aeasi)" <aeasi@...co.com>,
	"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
	"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 11/15] scsi: fnic: Modify fnic interfaces to use FDLS

On Wed, Jan 08, 2025 at 09:40:48PM +0000, Karan Tilak Kumar (kartilak) wrote:
> On Tuesday, January 7, 2025 5:18 AM, Dan Carpenter <dan.carpenter@...aro.org> wrote:
> > > @@ -1236,8 +1286,10 @@ static void __exit fnic_cleanup_module(void)
> > >  {
> > >     pci_unregister_driver(&fnic_driver);
> > >     destroy_workqueue(fnic_event_queue);
> > > -   if (fnic_fip_queue)
> > > +   if (fnic_fip_queue) {
> > > +           flush_workqueue(fnic_fip_queue);
> > >             destroy_workqueue(fnic_fip_queue);
> >
> > I don't think this change is necessary or related.  But if it is then it
> > needs to be split into its own patch with a Fixes tag.
> 
> Thanks Dan.
> We believe it is necessary to flush the frames in the fip queue before cleaning up.
> We would like to keep this as it is.

The issue with the patch is that it should have been split up into
probably five separate small patches.  Each change needs to be considered
on its own and explained why it's required.  This flush_workqueue()
change wasn't even mentioned in the commit message at all.  I don't blame
*you* for that because you didn't know but someone should have told you.

With regards to flush_workqueue(), I have looked some more today and the
flush_workqueue() is not required so this chunk does not need to be
backported to -stable kernels.  But if it had been required, there is no
way we could have done that with it all mixed together with other
changes.

I think there is a tool out somewhere which complains about code like
this because I've seen a lot of patches removing the extra call to
flush_workqueue().

97d26ae764a4 ("bcache: remove unnecessary flush_workqueue")
fb4b9685779f ("EDAC/wq: Remove unneeded flush_workqueue()")
d81c7cdd7a6d ("net/tls: Remove redundant workqueue flush before destroy")
etc..

regards,
dan carpenter