drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Dan Carpenter]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   128c8f96eb8638c060cd3532dc394d046ce64fe1
commit: a63e78eb2b0f654b138abfc323f6bd7573e26145 scsi: fnic: Add support for fabric based solicited requests and responses
config: i386-randconfig-141-20250214 (https://download.01.org/0day-ci/archive/20250214/202502141403.1PcpwyJp-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202502141403.1PcpwyJp-lkp@intel.com/

smatch warnings:
drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting
drivers/scsi/fnic/fdls_disc.c:989 fdls_send_fabric_logo() warn: inconsistent indenting
drivers/scsi/fnic/fdls_disc.c:1953 fnic_fdls_validate_and_get_frame_type() warn: inconsistent indenting

vim +/cur_jiffies +166 drivers/scsi/fnic/fdls_disc.c

a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  244  void fdls_schedule_oxid_free_retry_work(struct work_struct *work)
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  245  {
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  246  	struct fnic_oxid_pool_s *oxid_pool = container_of(work,
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  247  		struct fnic_oxid_pool_s, schedule_oxid_free_retry.work);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  248  	struct fnic_iport_s *iport = container_of(oxid_pool,
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  249  		struct fnic_iport_s, oxid_pool);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  250  	struct fnic *fnic = iport->fnic;
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  251  	struct reclaim_entry_s *reclaim_entry;
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  252  	unsigned long delay_j = msecs_to_jiffies(OXID_RECLAIM_TOV(iport));
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  253  	int idx;
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  254  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  255  	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

Passing fnic->lock_flags is wrong.  spin_lock_irqsave() is not nestable
in the sense that the "flags" variable can't hold two values at once:

	orig_irq = save_original irq states and disable()
	orig_irq = save_original irq states and disable()
	restore(orig_irq)
	restore(orig_irq)

The second restore(orig_irq) is not going to restore the original states
it's going to leave them as disabled.

If fnic->lock_flags is holding anything useful when this is called, then it
is a bug or if anything uses it before we exit that will break us.  Just
declare "unsigned long flags;" locally.

a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  256  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  257  	for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) {
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  258  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  259  		FNIC_FCS_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  260  			"Schedule oxid free. oxid idx: %d\n", idx);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  261  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  262  		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 @263  	reclaim_entry = (struct reclaim_entry_s *)
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  264  	kzalloc(sizeof(struct reclaim_entry_s), GFP_KERNEL);


The indenting is off.  The normal way to write this is:

		reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL);

Remove the cast and change the sizeof().

a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  265  		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  266  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  267  		if (!reclaim_entry) {
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  268  			FNIC_FCS_DBG(KERN_WARNING, fnic->lport->host, fnic->fnic_num,
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  269  				"Failed to allocate memory for reclaim struct for oxid idx: 0x%x\n",
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  270  				idx);

Remove this DBG.  The truth is that for as long as anyone can remember
the kmalloc() allocator won't fail for tiny allocations.  We're more
likely to formalize this rule than we are to change it at this point.
Still, we check for errors until that day arrives.

But allocations are not supposed to print an error message on error.
It's just messy and it doesn't add any information that isn't already
in the stack traces that kmalloc() prints.

I guess we can't call schedule_delayed_work() without holding the
spin_lock?  It's a strange thing.

a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  271  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  272  			schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry,
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  273  				msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME));
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  274  			spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  275  			return;
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  276  		}
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  277  
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  278  		if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) {
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  279  			reclaim_entry->oxid_idx = idx;
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  280  			reclaim_entry->expires = round_jiffies(jiffies + delay_j);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  281  			list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  282  			schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  283  		} else {
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  284  			/* unlikely scenario, free the allocated memory and continue */
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  285  			kfree(reclaim_entry);
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  286  		}
a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  287  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Dan Carpenter]

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

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)

RE: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Karan Tilak Kumar (kartilak)]

On Friday, February 14, 2025 7:03 AM, Dan Carpenter <dan.carpenter@linaro.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
>
> 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

RE: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Karan Tilak Kumar (kartilak)]

On Friday, February 14, 2025 6:42 AM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   128c8f96eb8638c060cd3532dc394d046ce64fe1
> commit: a63e78eb2b0f654b138abfc323f6bd7573e26145 scsi: fnic: Add support for fabric based solicited requests and responses
> config: i386-randconfig-141-20250214 (https://download.01.org/0day-ci/archive/20250214/202502141403.1PcpwyJp-lkp@intel.com/config)
> compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202502141403.1PcpwyJp-lkp@intel.com/
>
> smatch warnings:
> drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting
> drivers/scsi/fnic/fdls_disc.c:989 fdls_send_fabric_logo() warn: inconsistent indenting
> drivers/scsi/fnic/fdls_disc.c:1953 fnic_fdls_validate_and_get_frame_type() warn: inconsistent indenting
>
> vim +/cur_jiffies +166 drivers/scsi/fnic/fdls_disc.c
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  244  void fdls_schedule_oxid_free_retry_work(struct work_struct *work)
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  245  {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  246      struct fnic_oxid_pool_s *oxid_pool = container_of(work,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  247              struct fnic_oxid_pool_s, schedule_oxid_free_retry.work);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  248      struct fnic_iport_s *iport = container_of(oxid_pool,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  249              struct fnic_iport_s, oxid_pool);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  250      struct fnic *fnic = iport->fnic;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  251      struct reclaim_entry_s *reclaim_entry;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  252      unsigned long delay_j = msecs_to_jiffies(OXID_RECLAIM_TOV(iport));
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  253      int idx;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  254
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  255      spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
>
> Passing fnic->lock_flags is wrong.  spin_lock_irqsave() is not nestable
> in the sense that the "flags" variable can't hold two values at once:
>
> orig_irq = save_original irq states and disable()
> orig_irq = save_original irq states and disable()
> restore(orig_irq)
> restore(orig_irq)
>
> The second restore(orig_irq) is not going to restore the original states
> it's going to leave them as disabled.
>
> If fnic->lock_flags is holding anything useful when this is called, then it
> is a bug or if anything uses it before we exit that will break us.  Just
> declare "unsigned long flags;" locally.
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  256
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  257      for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  258
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  259              FNIC_FCS_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  260                      "Schedule oxid free. oxid idx: %d\n", idx);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  261
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  262              spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 @263      reclaim_entry = (struct reclaim_entry_s *)
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  264      kzalloc(sizeof(struct reclaim_entry_s), GFP_KERNEL);

Thanks for this change, Dan. It looks good.

>
> The indenting is off.  The normal way to write this is:
>
> reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL);
>
> Remove the cast and change the sizeof().

Thanks for this change, Dan. It looks good.

>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  265              spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  266
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  267              if (!reclaim_entry) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  268                      FNIC_FCS_DBG(KERN_WARNING, fnic->lport->host, fnic->fnic_num,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  269                              "Failed to allocate memory for reclaim struct for oxid idx: 0x%x\n",
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  270                              idx);
>
> Remove this DBG.  The truth is that for as long as anyone can remember
> the kmalloc() allocator won't fail for tiny allocations.  We're more
> likely to formalize this rule than we are to change it at this point.
> Still, we check for errors until that day arrives.

Thanks for this change, Dan. It looks good.

> But allocations are not supposed to print an error message on error.
> It's just messy and it doesn't add any information that isn't already
> in the stack traces that kmalloc() prints.
>
> I guess we can't call schedule_delayed_work() without holding the
> spin_lock?  It's a strange thing.
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  271
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  272                      schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  273                              msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME));
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  274                      spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  275                      return;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  276              }
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  277
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  278              if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  279                      reclaim_entry->oxid_idx = idx;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  280                      reclaim_entry->expires = round_jiffies(jiffies + delay_j);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  281                      list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  282                      schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  283              } else {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  284                      /* unlikely scenario, free the allocated memory and continue */
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  285                      kfree(reclaim_entry);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  286              }
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11  287  }
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
>

Would you like to send out a patch for these changes? If so, we can review and sign-off on it.
If you would like us to send out a patch, that's fine too; we will address these changes in a future patch.

Regards,
Karan (on behalf of the Cisco team)

RE: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Karan Tilak Kumar (kartilak)]

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@linaro.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

Re: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Dan Carpenter]

On Thu, Feb 20, 2025 at 03:10:26AM +0000, Karan Tilak Kumar (kartilak) wrote:
> 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@linaro.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.
> 

Could you send the patch for this.  I don't know the code that well and
can't test it.

regards,
dan carpenter

RE: drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting

[Karan Tilak Kumar (kartilak)]

On Wednesday, February 19, 2025 9:50 PM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Thu, Feb 20, 2025 at 03:10:26AM +0000, Karan Tilak Kumar (kartilak) wrote:
> > 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@linaro.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.
> >
>
> Could you send the patch for this.  I don't know the code that well and
> can't test it.
>
> regards,
> dan carpenter
>
>

Sure Dan. I'll test the code and send out the patches.

Regards,
Karan