On 05/02/2019 02:43 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler <tyreld@xxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> ---
>> 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
>
