[PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states

[Tyrel Datwyler]

On 05/02/2019 02:43 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler <tyreld at linux.vnet.ibm.com>
>>
>> The current implemenation relies on two flags in the drivers private host
>> structure to signal the need for a host reset or to reenable the CRQ after a
>> LPAR migration. This patch does away with those flags and introduces a single
>> action flag and defined enums for the supported kthread work actions. Lastly,
>> the if/else logic is replaced with a switch statement.
>>
>> Signed-off-by: Tyrel Datwyler <tyreld at linux.ibm.com>
>> ---
>>  drivers/scsi/ibmvscsi/ibmvscsi.c | 57 +++++++++++++++++++++-----------
>>  drivers/scsi/ibmvscsi/ibmvscsi.h |  9 +++--
>>  2 files changed, 45 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 1c37244f16a0..683139e6c63f 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -828,7 +828,7 @@ static void ibmvscsi_reset_host(struct ibmvscsi_host_data *hostdata)
>>  	atomic_set(&hostdata->request_limit, 0);
>>  
>>  	purge_requests(hostdata, DID_ERROR);
>> -	hostdata->reset_crq = 1;
>> +	hostdata->action = IBMVSCSI_HOST_ACTION_RESET;
>>  	wake_up(&hostdata->work_wait_q);
>>  }
>>  
>> @@ -1797,7 +1797,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>  			/* We need to re-setup the interpartition connection */
>>  			dev_info(hostdata->dev, "Re-enabling adapter!\n");
>>  			hostdata->client_migrated = 1;
>> -			hostdata->reenable_crq = 1;
>> +			hostdata->action = IBMVSCSI_HOST_ACTION_REENABLE;
>>  			purge_requests(hostdata, DID_REQUEUE);
>>  			wake_up(&hostdata->work_wait_q);
>>  		} else {
>> @@ -2118,26 +2118,32 @@ static unsigned long ibmvscsi_get_desired_dma(struct vio_dev *vdev)
>>  
>>  static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>>  {
>> +	unsigned long flags;
>>  	int rc;
>>  	char *action = "reset";
>>  
>> -	if (hostdata->reset_crq) {
>> -		smp_rmb();
>> -		hostdata->reset_crq = 0;
>> -
>> +	spin_lock_irqsave(hostdata->host->host_lock, flags);
>> +	switch (hostdata->action) {
>> +	case IBMVSCSI_HOST_ACTION_NONE:
>> +		break;
>> +	case IBMVSCSI_HOST_ACTION_RESET:
>>  		rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
> 
> Looks like you are now calling ibmvscsi_reset_crq_queue with the host_lock held.
> However, ibmvscsi_reset_crq_queue can call msleep.

Good catch. I remember thinking that needed to run lockless, but clearly failed
to release and re-grab the lock around that call.

-Tyrel

> 
> This had been implemented as separate reset_crq and reenable_crq fields
> so that it could run lockless. I'm not opposed to changing this to a single
> field in general, we just need to be careful where we are adding locking.
> 
> Thanks,
> 
> Brian
>