Re: [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2022-05-30 16:15:10 [-0700], Davidlohr Bueso wrote:
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index d0eab5700dc5..31b1900489e7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -891,7 +891,7 @@ static void ibmvfc_release_crq_queue(struct ibmvfc_host *vhost)
>  
>  	ibmvfc_dbg(vhost, "Releasing CRQ\n");
>  	free_irq(vdev->irq, vhost);
> -	tasklet_kill(&vhost->tasklet);
> +        cancel_work_sync(&vhost->work);
s/ {8}/\t/

is there a reason not to use threaded interrupts? The workqueue _might_
migrate to another CPU. The locking ensures that nothing bad happens but
ibmvfc_tasklet() has this piece:

|         spin_lock_irqsave(vhost->host->host_lock, flags);
…
|                 while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
|                         ibmvfc_handle_async(async, vhost);
|                         async->valid = 0;
|                         wmb();
|                 }
…
|                 vio_enable_interrupts(vdev);
potentially enables interrupts which fires right away.

|                 if ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
|                         vio_disable_interrupts(vdev);

disables it again.

|         }
| 
|         spin_unlock(vhost->crq.q_lock);
|         spin_unlock_irqrestore(vhost->host->host_lock, flags);

If the worker runs on another CPU then the CPU servicing the interrupt
will be blocked on the lock which is not nice.

My guess is that you could enable interrupts right before unlocking but
is a different story.

>  	do {
>  		if (rc)
>  			msleep(100);

Sebastian