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@xxxxxxxxxx> 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
