linux-kernel - RE: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry

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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SJ0PR11MB5896888C81BFF2EB81E2B533C3C42@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date: Thu, 20 Feb 2025 03:10:26 +0000
From: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, "oe-kbuild@...ts.linux.dev"
	<oe-kbuild@...ts.linux.dev>
CC: "lkp@...el.com" <lkp@...el.com>, "oe-kbuild-all@...ts.linux.dev"
	<oe-kbuild-all@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Martin K. Petersen"
	<martin.petersen@...cle.com>, "Sesidhar Baddela (sebaddel)"
	<sebaddel@...co.com>, "Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>,
	"Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>, "Arun Easi (aeasi)"
	<aeasi@...co.com>
Subject: RE: drivers/scsi/fnic/fdls_disc.c:263
 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

On Saturday, February 15, 2025 1:03 PM, Karan Tilak Kumar (kartilak) wrote:
>
> On Friday, February 14, 2025 7:03 AM, Dan Carpenter <dan.carpenter@...aro.org> wrote:
> >
> > On Fri, Feb 14, 2025 at 05:42:25PM +0300, Dan Carpenter wrote:
> > >
> > > I guess we can't call schedule_delayed_work() without holding the
> > > spin_lock?  It's a strange thing.
> >
> > Actually, I see now that we're just dropping the lock to avoid a sleeping
> > in atomic warning with the GFP_ATOMIC allocation...  I bet you could make
> > the code under the lock much smaller.  Just the test_and_clear_bit() stuff
> > basically.
> >
> > regards,
> > dan carpenter

The window can be further reduced by adding the lock/unlock around the list_add_tail. But this looks good too.

> > diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c
> > index 11211c469583..ed141236a4aa 100644
> > --- a/drivers/scsi/fnic/fdls_disc.c
> > +++ b/drivers/scsi/fnic/fdls_disc.c
> > @@ -308,31 +308,22 @@ void fdls_schedule_oxid_free_retry_work(struct work_struct *work)
> > struct fnic *fnic = iport->fnic;
> > struct reclaim_entry_s *reclaim_entry;
> > unsigned long delay_j = msecs_to_jiffies(OXID_RECLAIM_TOV(iport));
> > +     unsigned long flags;
> > int idx;
> >
> > -     spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> > -
> > for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) {
> >
> > FNIC_FCS_DBG(KERN_INFO, fnic->host, fnic->fnic_num,
> > "Schedule oxid free. oxid idx: %d\n", idx);
> >
> > -             spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> > -     reclaim_entry = (struct reclaim_entry_s *)
> > -     kzalloc(sizeof(struct reclaim_entry_s), GFP_KERNEL);
> > -             spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> > -
> > +             reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL);
> > if (!reclaim_entry) {
> > -                     FNIC_FCS_DBG(KERN_WARNING, fnic->host, fnic->fnic_num,
> > -                             "Failed to allocate memory for reclaim struct for oxid idx: 0x%x\n",
> > -                             idx);
> > -
> > schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry,
> > msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME));
> > -                     spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> > return;
> > }
> >
> > +             spin_lock_irqsave(&fnic->fnic_lock, flags);
> > if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) {
> > reclaim_entry->oxid_idx = idx;
> > reclaim_entry->expires = round_jiffies(jiffies + delay_j);
> > @@ -342,9 +333,8 @@ void fdls_schedule_oxid_free_retry_work(struct work_struct *work)
> > /* unlikely scenario, free the allocated memory and continue */
> > kfree(reclaim_entry);
> > }
> > -}
> > -
> > -     spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> > +             spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> > +     }
> > }
> >
> > static bool fdls_is_oxid_fabric_req(uint16_t oxid)
> >
> >
>
> Thanks for the comments and changes Dan. Appreciate your help.
> The team will review these changes and get back to you.
>
> Regards,
> Karan
>

Please let us know if you would like to send out a patch for this. We will review and sign off on it.
Thanks for your review and changes.

Regards,
Karan