Re: [PATCH v16 09/12] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ibmvfc: same-class q_lock nesting in ibmvfc_drain_async_subq()
--

commit d3f6ce8f09b129387eea8b24d92aacbad8e6f279
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

This commit adds ibmvfc_interrupt_async_subq(), ibmvfc_drain_async_subq(),
and ibmvfc_next_async_scrq() to handle and drain asynchronous sub-CRQ events
from the adapter.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 64981a5a39fbf..2cb38757333a4 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4387,6 +4387,72 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>  	return crq;
>  }
>  
> +static struct ibmvfc_async_sub_crq *ibmvfc_next_async_scrq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_async_sub_crq *crq;
> +
> +	crq = &scrq->msgs.async_scrq[scrq->cur];
> +	if (crq->valid & 0x80) {
> +		if (++scrq->cur == scrq->size)
> +			scrq->cur = 0;
> +		rmb();
> +	} else
> +		crq = NULL;
> +
> +	return crq;
> +}
> +
> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_host *vhost = scrq->vhost;
> +	struct ibmvfc_async_crq_event ae = {
> +		.type = IBMVFC_ASYNC_CRQ_SUB,
> +	};
> +	struct ibmvfc_async_sub_crq *crq;
> +	unsigned long flags;
> +	int done = 0;
> +
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +	spin_lock(scrq->q_lock);
> +	while (!done) {
> +		while ((crq = ibmvfc_next_async_scrq(scrq)) != NULL) {
> +			ae.subq = *crq;
> +			ibmvfc_handle_async(&ae, scrq->vhost);

[Severity: High]
Can calling ibmvfc_handle_async() while holding scrq->q_lock trigger a
lockdep recursive locking warning?

Every struct ibmvfc_queue (including async_scrqs, scsi_scrqs, and nvme_scrqs)
initializes queue->_lock at the same call site in ibmvfc_alloc_queue():

	spin_lock_init(&queue->_lock);
	queue->q_lock = &queue->_lock;

This places scrq->q_lock, scsi_q[i].q_lock, and nvme_q[i].q_lock into the
same lockdep class.

When ibmvfc_drain_async_subq() processes a reset-triggering async event
(such as IBMVFC_AE_RESUME, IBMVFC_AE_LINK_UP, IBMVFC_AE_SCN_FABRIC, or
IBMVFC_AE_SCN_DOMAIN) while vhost->action is IBMVFC_HOST_ACTION_LOGO_WAIT
(for example, if a second async event arrives while NPIV logout is in flight
with vhost->using_channels set) or vhost->state is IBMVFC_HOST_OFFLINE, the
call chain reaches ibmvfc_purge_requests():

  ibmvfc_drain_async_subq()
    spin_lock(scrq->q_lock)
    ibmvfc_handle_async()
      __ibmvfc_reset_host()
        ibmvfc_hard_reset_host()
          ibmvfc_purge_requests()

In ibmvfc_purge_requests(), the loops over active SCSI and NVMe sub-queues
acquire scsi_q[i].q_lock and nvme_q[i].q_lock at subclass 0 while
scrq->q_lock of the same lockdep class is still held:

	for (i = 0; i < shwqs; i++) {
		spin_lock_irqsave(scsi_q[i].q_lock, flags);
		spin_lock(&scsi_q[i].l_lock);
		...
	}

	for (i = 0; i < nhwqs; i++) {
		spin_lock_irqsave(nvme_q[i].q_lock, flags);
		spin_lock(&nvme_q[i].l_lock);
		...
	}

Should async sub-queue locks use a separate lockdep class or subclass, or
should ibmvfc_handle_async() be called without holding scrq->q_lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-ibmvfc-fpin-support-v16-0-a900aa657ed5@linux.ibm.com?part=9