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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

>> 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?

I'm rewriting ibmvfc_drain_async_subq() to create a list of events to
handle, similar to the way ibmvfc_drain_sub_crq() does it, then unlock
the queue lock, and then call ibmvfc_handle_async() for each event. This
resolves the lockdep issue.

-Dave