* [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
@ 2019-05-02 0:47 Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states Tyrel Datwyler
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Tyrel Datwyler @ 2019-05-02 0:47 UTC (permalink / raw)
To: james.bottomley
Cc: Tyrel Datwyler, martin.petersen, linux-scsi, Tyrel Datwyler,
brking, linuxppc-dev
From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
Wire up the host_reset function in our driver_template to allow a user
requested adpater reset via the host_reset sysfs attribute.
Example:
echo "adapter" > /sys/class/scsi_host/host0/host_reset
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 8cec5230fe31..1c37244f16a0 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2050,6 +2050,18 @@ static struct device_attribute ibmvscsi_host_config = {
.show = show_host_config,
};
+static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
+{
+ struct ibmvscsi_host_data *hostdata = shost_priv(shost);
+
+ vio_disable_interrupts(to_vio_dev(hostdata->dev));
+ dev_info(hostdata->dev, "Initiating adapter reset!\n");
+ ibmvscsi_reset_host(hostdata);
+ vio_enable_interrupts(to_vio_dev(hostdata->dev));
+
+ return 0;
+}
+
static struct device_attribute *ibmvscsi_attrs[] = {
&ibmvscsi_host_vhost_loc,
&ibmvscsi_host_vhost_name,
@@ -2076,6 +2088,7 @@ static struct scsi_host_template driver_template = {
.eh_host_reset_handler = ibmvscsi_eh_host_reset_handler,
.slave_configure = ibmvscsi_slave_configure,
.change_queue_depth = ibmvscsi_change_queue_depth,
+ .host_reset = ibmvscsi_host_reset,
.cmd_per_lun = IBMVSCSI_CMDS_PER_LUN_DEFAULT,
.can_queue = IBMVSCSI_MAX_REQUESTS_DEFAULT,
.this_id = -1,
--
2.18.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states
2019-05-02 0:47 [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Tyrel Datwyler
@ 2019-05-02 0:47 ` Tyrel Datwyler
2019-05-02 21:43 ` Brian King
2019-05-02 0:47 ` [PATCH 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON Tyrel Datwyler
2019-05-02 21:50 ` [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Brian King
2 siblings, 1 reply; 7+ messages in thread
From: Tyrel Datwyler @ 2019-05-02 0:47 UTC (permalink / raw)
To: james.bottomley
Cc: Tyrel Datwyler, martin.petersen, linux-scsi, Tyrel Datwyler,
brking, linuxppc-dev
From: Tyrel Datwyler <tyreld@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@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);
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
vio_enable_interrupts(to_vio_dev(hostdata->dev));
- } else if (hostdata->reenable_crq) {
- smp_rmb();
+ break;
+ case IBMVSCSI_HOST_ACTION_REENABLE:
action = "enable";
rc = ibmvscsi_reenable_crq_queue(&hostdata->queue, hostdata);
- hostdata->reenable_crq = 0;
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
- } else
- return;
+ break;
+ default:
+ break;
+ }
+
+ hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
if (rc) {
atomic_set(&hostdata->request_limit, -1);
@@ -2147,19 +2153,32 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
scsi_unblock_requests(hostdata->host);
}
-static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+static int __ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
{
if (kthread_should_stop())
return 1;
- else if (hostdata->reset_crq) {
- smp_rmb();
- return 1;
- } else if (hostdata->reenable_crq) {
- smp_rmb();
- return 1;
+ switch (hostdata->action) {
+ case IBMVSCSI_HOST_ACTION_NONE:
+ return 0;
+ case IBMVSCSI_HOST_ACTION_RESET:
+ case IBMVSCSI_HOST_ACTION_REENABLE:
+ default:
+ break;
}
- return 0;
+ return 1;
+}
+
+static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+{
+ unsigned long flags;
+ int rc;
+
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
+ rc = __ibmvscsi_work_to_do(hostdata);
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+
+ return rc;
}
static int ibmvscsi_work(void *data)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 3a7875575616..04bcbc832dc9 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -88,13 +88,18 @@ struct event_pool {
dma_addr_t iu_token;
};
+enum ibmvscsi_host_action {
+ IBMVSCSI_HOST_ACTION_NONE = 0,
+ IBMVSCSI_HOST_ACTION_RESET,
+ IBMVSCSI_HOST_ACTION_REENABLE,
+};
+
/* all driver data associated with a host adapter */
struct ibmvscsi_host_data {
struct list_head host_list;
atomic_t request_limit;
int client_migrated;
- int reset_crq;
- int reenable_crq;
+ enum ibmvscsi_host_action action;
struct device *dev;
struct event_pool pool;
struct crq_queue queue;
--
2.18.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON
2019-05-02 0:47 [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states Tyrel Datwyler
@ 2019-05-02 0:47 ` Tyrel Datwyler
2019-05-02 21:50 ` [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Brian King
2 siblings, 0 replies; 7+ messages in thread
From: Tyrel Datwyler @ 2019-05-02 0:47 UTC (permalink / raw)
To: james.bottomley
Cc: Tyrel Datwyler, martin.petersen, linux-scsi, Tyrel Datwyler,
brking, linuxppc-dev
From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
After a successful SRP login response we call scsi_unblock_requests() to
kick any pending IO's. The callback to process this SRP response happens in
a tasklet and therefore is in softirq context. The result of such is
that when blk-mq is enabled it is no longer safe to call
scsi_unblock_requests() from this context. The result of duing so
triggers the following WARN_ON splat in dmesg after a host reset or CRQ
reenablement.
WARNING: CPU: 0 PID: 0 at block/blk-mq.c:1375 __blk_mq_run_hw_queue+0x120/0x180
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc8 #4
NIP [c0000000009771e0] __blk_mq_run_hw_queue+0x120/0x180
LR [c000000000977484] __blk_mq_delay_run_hw_queue+0x244/0x250
Call Trace:
__blk_mq_delay_run_hw_queue+0x244/0x250
blk_mq_run_hw_queue+0x8c/0x1c0
blk_mq_run_hw_queues+0x60/0x90
scsi_run_queue+0x1e4/0x3b0
scsi_run_host_queues+0x48/0x80
login_rsp+0xb0/0x100
ibmvscsi_handle_crq+0x30c/0x3e0
ibmvscsi_task+0x54/0xe0
tasklet_action_common.isra.3+0xc4/0x1a0
__do_softirq+0x174/0x3f4
irq_exit+0xf0/0x120
__do_irq+0xb0/0x210
call_do_irq+0x14/0x24
do_IRQ+0x9c/0x130
hardware_interrupt_common+0x14c/0x150
This patch fixes the issue by introducing a new host action for
unblocking the scsi requests in our seperate work thread.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 683139e6c63f..c1d83eb5c5f7 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1179,7 +1179,8 @@ static void login_rsp(struct srp_event_struct *evt_struct)
be32_to_cpu(evt_struct->xfer_iu->srp.login_rsp.req_lim_delta));
/* If we had any pending I/Os, kick them */
- scsi_unblock_requests(hostdata->host);
+ hostdata->action = IBMVSCSI_HOST_ACTION_UNBLOCK;
+ wake_up(&hostdata->work_wait_q);
}
/**
@@ -2125,6 +2126,7 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
case IBMVSCSI_HOST_ACTION_NONE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
break;
case IBMVSCSI_HOST_ACTION_RESET:
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
@@ -2162,6 +2164,7 @@ static int __ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
return 0;
case IBMVSCSI_HOST_ACTION_RESET:
case IBMVSCSI_HOST_ACTION_REENABLE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
default:
break;
}
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 04bcbc832dc9..d9bf502334ba 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -92,6 +92,7 @@ enum ibmvscsi_host_action {
IBMVSCSI_HOST_ACTION_NONE = 0,
IBMVSCSI_HOST_ACTION_RESET,
IBMVSCSI_HOST_ACTION_REENABLE,
+ IBMVSCSI_HOST_ACTION_UNBLOCK,
};
/* all driver data associated with a host adapter */
--
2.18.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states
2019-05-02 0:47 ` [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states Tyrel Datwyler
@ 2019-05-02 21:43 ` Brian King
2019-05-03 0:35 ` Tyrel Datwyler
0 siblings, 1 reply; 7+ messages in thread
From: Brian King @ 2019-05-02 21:43 UTC (permalink / raw)
To: Tyrel Datwyler, james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
> From: Tyrel Datwyler <tyreld@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@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.
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
--
Brian King
Power Linux I/O
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
2019-05-02 0:47 [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON Tyrel Datwyler
@ 2019-05-02 21:50 ` Brian King
2019-05-03 0:37 ` Tyrel Datwyler
2 siblings, 1 reply; 7+ messages in thread
From: Brian King @ 2019-05-02 21:50 UTC (permalink / raw)
To: Tyrel Datwyler, james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
> From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>
> Wire up the host_reset function in our driver_template to allow a user
> requested adpater reset via the host_reset sysfs attribute.
>
> Example:
>
> echo "adapter" > /sys/class/scsi_host/host0/host_reset
>
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 8cec5230fe31..1c37244f16a0 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2050,6 +2050,18 @@ static struct device_attribute ibmvscsi_host_config = {
> .show = show_host_config,
> };
>
> +static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
> +{
> + struct ibmvscsi_host_data *hostdata = shost_priv(shost);
> +
> + vio_disable_interrupts(to_vio_dev(hostdata->dev));
> + dev_info(hostdata->dev, "Initiating adapter reset!\n");
> + ibmvscsi_reset_host(hostdata);
> + vio_enable_interrupts(to_vio_dev(hostdata->dev));
Is it necessary to disable / enable interrupts around the call to ibmvscsi_reset_host?
I don't know why we'd need to do that before calling the reset as we have other
cases, like ibmvscsi_timeout where we don't bother doing this. Also, at the end
of the reset we look to be already enabling interrupts.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states
2019-05-02 21:43 ` Brian King
@ 2019-05-03 0:35 ` Tyrel Datwyler
0 siblings, 0 replies; 7+ messages in thread
From: Tyrel Datwyler @ 2019-05-03 0:35 UTC (permalink / raw)
To: Brian King, Tyrel Datwyler, james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen
On 05/02/2019 02:43 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler <tyreld@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@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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
2019-05-02 21:50 ` [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Brian King
@ 2019-05-03 0:37 ` Tyrel Datwyler
0 siblings, 0 replies; 7+ messages in thread
From: Tyrel Datwyler @ 2019-05-03 0:37 UTC (permalink / raw)
To: Brian King, Tyrel Datwyler, james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen
On 05/02/2019 02:50 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>
>> Wire up the host_reset function in our driver_template to allow a user
>> requested adpater reset via the host_reset sysfs attribute.
>>
>> Example:
>>
>> echo "adapter" > /sys/class/scsi_host/host0/host_reset
>>
>> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 8cec5230fe31..1c37244f16a0 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -2050,6 +2050,18 @@ static struct device_attribute ibmvscsi_host_config = {
>> .show = show_host_config,
>> };
>>
>> +static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
>> +{
>> + struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>> +
>> + vio_disable_interrupts(to_vio_dev(hostdata->dev));
>> + dev_info(hostdata->dev, "Initiating adapter reset!\n");
>> + ibmvscsi_reset_host(hostdata);
>> + vio_enable_interrupts(to_vio_dev(hostdata->dev));
>
> Is it necessary to disable / enable interrupts around the call to ibmvscsi_reset_host?
> I don't know why we'd need to do that before calling the reset as we have other
> cases, like ibmvscsi_timeout where we don't bother doing this. Also, at the end
> of the reset we look to be already enabling interrupts.
Yeah, I think you are right. My initial line of thought was that we have
interrupts disabled in handle_crq when we do a reset, but yeah we clearly call
it in the case of a timeout with them enabled.
-Tyrel
>
> Thanks,
>
> Brian
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-03 0:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-02 0:47 [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states Tyrel Datwyler
2019-05-02 21:43 ` Brian King
2019-05-03 0:35 ` Tyrel Datwyler
2019-05-02 0:47 ` [PATCH 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON Tyrel Datwyler
2019-05-02 21:50 ` [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template Brian King
2019-05-03 0:37 ` Tyrel Datwyler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.