Re: [PATCH v3 82/88] scsi: qla2xxx: Serialize NVMe unsol ctx list with a per-fcport lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `unsol_ctx_head` list is functionally write-only and never traversed, making the list and the newly added `unsol_ctx_lock` dead state that should be removed instead of synchronized.
--

--- Patch [82]: [PATCH v3 82/88] scsi: qla2xxx: Serialize NVMe unsol ctx list with a per-fcport lock ---
commit f8ccd94b18e0f83a1d0b27da47b7d2c7baf3e9c0
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Serialize NVMe unsol ctx list with a per-fcport lock

This commit introduces a per-fcport spinlock to serialize accesses to
the unsol_ctx_head list. It adds the lock to protect list_add_tail and
list_del operations from concurrent accesses in different contexts.

> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index bb4305f6a364c..afbd056148bd5 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
[ ... ]
> @@ -2645,6 +2645,8 @@ typedef struct fc_port {
>  	struct list_head list;
>  	struct scsi_qla_host *vha;
>  	struct list_head unsol_ctx_head;
> +	/* Serializes unsol_ctx_head against ISR, DPC and NVMe transport. */
> +	spinlock_t unsol_ctx_lock;

[Severity: Medium]
Is the unsol_ctx_head list ever actually read or traversed anywhere in the
codebase? Looking through the driver, it appears this list might be 
functionally write-only. If the list is dead state, would it be better
to completely remove unsol_ctx_head and uctx->elem rather than adding a
spinlock to protect them?

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 36b742f73abf3..beccece1e7d92 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
[ ... ]
> @@ -1439,7 +1446,9 @@ void qla2xxx_process_purls_iocb(void **pkt, struct rsp_que **rsp)
>  	uctx->ox_id = p->ox_id;
>  	qla_rport->uctx = uctx;
>  	INIT_LIST_HEAD(&uctx->elem);
> +	spin_lock_irqsave(&fcport->unsol_ctx_lock, flags);
>  	list_add_tail(&uctx->elem, &fcport->unsol_ctx_head);
> +	spin_unlock_irqrestore(&fcport->unsol_ctx_lock, flags);
>  	item->purls_context = (void *)uctx;

[Severity: Medium]
Does taking this new spinlock in the hot path to add an element to an 
unused list introduce unnecessary synchronization overhead? If the list
is never traversed, could we avoid this performance penalty by removing
the list operations entirely instead of serializing it?

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