Re: [PATCH 12/14] scsi: qla2xxx: Yield work_lock in fabric scan path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla2xxx: wrong lock targeted with trylock loop in qla_fab_scan_finish()
--

commit 97d6a044424099a02acc895f356a103166186107
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Yield work_lock in fabric scan path

This commit replaces spin_lock_irqsave(&vha->work_lock, flags) in
qla_fab_scan_finish() with a spin_trylock_irqsave() and cond_resched() loop
to avoid a hard lockup observed during vport deletion.

> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index efe98609c149d..31308a6148585 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -3386,7 +3386,9 @@ void qla_fab_scan_finish(scsi_qla_host_t *vha, srb_t *sp)
>  	recheck = 1;
>  out:
>  	qla24xx_sp_unmap(vha, sp);
> -	spin_lock_irqsave(&vha->work_lock, flags);
> +	/* Avoid hard lockup: yield if work_lock is held instead of spinning */
> +	while (!spin_trylock_irqsave(&vha->work_lock, flags))
> +		cond_resched();
>  	vha->scan.scan_flags &= ~SF_SCANNING;
>  	spin_unlock_irqrestore(&vha->work_lock, flags);
>  

[Severity: High]
Is this targeting the right spinlock for the hard lockup shown in the commit
message, and is a trylock loop safe here?

In the qla2x00_iocb_work_fn() -> qla2x00_do_work() -> qla_fab_scan_finish()
path, the static helper qla2x00_is_a_vp() is inlined into the
max_fibre_devices scan loop in qla_fab_scan_finish():

drivers/scsi/qla2xxx/qla_gs.c:qla2x00_is_a_vp() {
	...
	spin_lock_irqsave(&ha->vport_slock, flags);
	list_for_each_entry(vp, &ha->vp_list, list) {
		twwn = wwn_to_u64(vp->port_name);
		if (wwn == twwn) {
			rc = 1;
			break;
		}
	}
	spin_unlock_irqrestore(&ha->vport_slock, flags);
	...
}

drivers/scsi/qla2xxx/qla_gs.c:qla_fab_scan_finish() {
	...
	for (i = 0; i < vha->hw->max_fibre_devices; i++) {
		...
		/* Bypass virtual ports of the same host. */
		if (qla2x00_is_a_vp(vha, wwn))
			continue;
		...
	}
}

Looking at the registers in the backtrace:
- rbp is 0xffff98b803e60a58, which is vha (at the shost_priv offset 0xa58),
  where &vha->work_lock at offset 0x30 would be 0xffff98b803e60a88.
- rdi (the lock pointer passed to _raw_spin_lock_irqsave) is
  0xffff98b149ff6488, which is at offset 0x488 of struct qla_hw_data *ha
  (0xffff98b149ff6000), matching &ha->vport_slock (as also seen in commits
  79b305e1e477, 0334585318b2, and 200e63c70fc1).
- r10 and r15 hold 0x2100000eddd08241 (the 64-bit port name wwn passed to
  qla2x00_is_a_vp(vha, wwn)), r13 holds 0xffffb9a8a4555000 (vha->scan.l),
  rcx holds 0x800 (vha->hw->max_fibre_devices), and r12 holds 0xc000
  (2048 * sizeof(struct fab_scan_rp)).

This indicates the worker was spinning on &ha->vport_slock inside
qla2x00_is_a_vp() rather than on &vha->work_lock at the out label.

In addition, vha->work_lock is not taken in the vport delete path
(qla24xx_vport_delete() -> qla24xx_disable_vp() ->
qla24xx_deallocate_vp_id()) and is only held across short, non-blocking flag
and list updates with interrupts disabled. By contrast, ha->vport_slock
protects ha->vp_list during vport deletion, where several
list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) loops (such as in
qla2x00_abort_isp_cleanup(), qla2x00_abort_isp(), qla2x00_quiesce_io(),
qla82xx_restart_isp(), and qla_pci_error_cleanup()) drop and re-acquire
ha->vport_slock without refreshing tvp if qla24xx_deallocate_vp_id() removes
an entry via list_del_init(&vha->list).

Could replacing spin_lock_irqsave(&vha->work_lock, flags) with a
spin_trylock_irqsave() + cond_resched() loop also introduce regressions by
bypassing queued spinlock fairness and realtime priority inheritance,
busy-looping without cpu_relax() while toggling interrupts when
need_resched() is false, or livelocking the qla2xxx_wq worker if the lock is
ever stuck (leaving scan_flags with SF_SCANNING set and hanging
flush_workqueue(vha->hw->wq) in qla2x00_wait_for_sess_deletion())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021102310.3854868-1-njavali@marvell.com?part=12