sparse review comments below...
On 06/28/2017 10:33 AM, Hannes Reinecke wrote:
When resetting a device we shouldn't depend on an existing SCSI
device, so use 'struct scsi_device' as argument for
eh_device_reset_handler().
Signed-off-by: Hannes Reinecke <hare@xxxxxxxx>
---
Documentation/scsi/scsi_eh.txt | 2 +-
Documentation/scsi/scsi_mid_low_api.txt | 4 +--
drivers/s390/scsi/zfcp_scsi.c | 4 +--
drivers/scsi/scsi_debug.c | 18 ++++++-------
drivers/scsi/scsi_error.c | 35 +++++++++++++++++--------
include/scsi/scsi_host.h | 2 +-
60 files changed, 287 insertions(+), 307 deletions(-)
diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index cb9f4bc22..f8a6566 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -206,7 +206,7 @@ hostt EH callbacks. Callbacks may be omitted and omitted ones are
considered to fail always.
int (* eh_abort_handler)(struct scsi_cmnd *);
-int (* eh_device_reset_handler)(struct scsi_cmnd *);
+int (* eh_device_reset_handler)(struct scsi_device *);
int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
int (* eh_host_reset_handler)(struct Scsi_Host *);
diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
index e2609a63..28dc029 100644
--- a/Documentation/scsi/scsi_mid_low_api.txt
+++ b/Documentation/scsi/scsi_mid_low_api.txt
@@ -874,7 +874,7 @@ Details:
/**
* eh_device_reset_handler - issue SCSI device reset
- * @scp: identifies SCSI device to be reset
+ * @sdev: identifies SCSI device to be reset
*
* Returns SUCCESS if command aborted else FAILED
*
@@ -887,7 +887,7 @@ Details:
*
* Optionally defined in: LLD
**/
- int eh_device_reset_handler(struct scsi_cmnd * scp)
+ int eh_device_reset_handler(struct scsi_device * sdev)
/**
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 92a3902..23b5a7d 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -304,9 +304,9 @@ static int zfcp_task_mgmt_function(struct scsi_device *sdev, u8 tm_flags)
return retval;
}
-static int zfcp_scsi_eh_device_reset_handler(struct scsi_cmnd *scpnt)
+static int zfcp_scsi_eh_device_reset_handler(struct scsi_device *sdev)
{
- return zfcp_task_mgmt_function(scpnt->device, FCP_TMF_LUN_RESET);
+ return zfcp_task_mgmt_function(sdev, FCP_TMF_LUN_RESET);
}
/*
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 37e511f..9029500 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3793,19 +3793,17 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
return SUCCESS;
}
-static int scsi_debug_device_reset(struct scsi_cmnd * SCpnt)
+static int scsi_debug_device_reset(struct scsi_device * sdp)
{
+ struct sdebug_dev_info *devip =
+ (struct sdebug_dev_info *)sdp->hostdata;
+
++num_dev_resets;
- if (SCpnt && SCpnt->device) {
- struct scsi_device *sdp = SCpnt->device;
- struct sdebug_dev_info *devip =
- (struct sdebug_dev_info *)sdp->hostdata;
- if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
- sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
- if (devip)
- set_bit(SDEBUG_UA_POR, devip->uas_bm);
- }
+ if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
+ sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
+ if (devip)
+ set_bit(SDEBUG_UA_POR, devip->uas_bm);
return SUCCESS;
}
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 368a961..4a7fe97f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -844,7 +844,7 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd->device);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
up to here it looks good,
however ...
@@ -1106,6 +1106,7 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
* scsi_eh_finish_cmd - Handle a cmd that eh is finished with.
__scsi_eh_finish_cmd ?
* @scmd: Original SCSI cmd that eh has finished.
* @done_q: Queue for processed commands.
+ * @result: Final command status to be set
*
* Notes:
* We don't want to use the normal command completion while we are are
Did this hunk slip in accidentally?
I don't see a new additional argument result being introduced with
either the new __scsi_eh_finish_cmd() nor the old scsi_eh_finish_cmd().
However, the former __scsi_eh_finish_cmd() seems to have an additional
argument @host_byte:. Maybe that should go where @result: is above,
because the old kernel doc now belongs to the new __scsi_eh_finish_cmd().
But then you'd also need to change the function name in the kernel doc?!
@@ -1114,10 +1115,18 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
* keep a list of pending commands for final completion, and once we
* are ready to leave error handling we handle completion for real.
*/
-void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
+void __scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q,
+ int host_byte)
Should new internal helper function be declared static?
{
+ if (host_byte)
+ set_host_byte(scmd, host_byte);
list_move_tail(&scmd->eh_entry, done_q);
}
+
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
+{
+ __scsi_eh_finish_cmd(scmd, done_q, 0);
+}
EXPORT_SYMBOL(scsi_eh_finish_cmd);
/**
@@ -1284,7 +1293,8 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
if (finish_cmds &&
(try_stu ||
scsi_eh_action(scmd, SUCCESS) == SUCCESS))
- scsi_eh_finish_cmd(scmd, done_q);
+ __scsi_eh_finish_cmd(scmd, done_q,
+ DID_RESET);
else
list_move_tail(&scmd->eh_entry, work_q);
}
@@ -1429,8 +1439,9 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
work_q, eh_entry) {
if (scmd->device == sdev &&
scsi_eh_action(scmd, rtn) != FAILED)
- scsi_eh_finish_cmd(scmd,
- done_q);
+ __scsi_eh_finish_cmd(scmd,
+ done_q,
+ DID_RESET);
}
}
} else {
@@ -1498,7 +1509,8 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
if (rtn == SUCCESS)
list_move_tail(&scmd->eh_entry, &check_list);
else if (rtn == FAST_IO_FAIL)
- scsi_eh_finish_cmd(scmd, done_q);
+ __scsi_eh_finish_cmd(scmd, done_q,
+ DID_TRANSPORT_DISRUPTED);
else
/* push back on work queue for further processing */
list_move(&scmd->eh_entry, work_q);
@@ -1563,8 +1575,9 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
if (channel == scmd_channel(scmd)) {
if (rtn == FAST_IO_FAIL)
- scsi_eh_finish_cmd(scmd,
- done_q);
+ __scsi_eh_finish_cmd(scmd,
+ done_q,
+ DID_TRANSPORT_DISRUPTED);
else
list_move_tail(&scmd->eh_entry,
&check_list);
@@ -1607,9 +1620,9 @@ static int scsi_eh_host_reset(struct Scsi_Host *shost,
if (rtn == SUCCESS) {
list_splice_init(work_q, &check_list);
} else if (rtn == FAST_IO_FAIL) {
- list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
- scsi_eh_finish_cmd(scmd, done_q);
- }
+ list_for_each_entry_safe(scmd, next, work_q, eh_entry)
+ __scsi_eh_finish_cmd(scmd, done_q,
+ DID_TRANSPORT_DISRUPTED);
I can somewhat imagine why you set DID_TRANSPORT_DISRUPTED for the cases
above where the eh callback told us FAST_IO_FAIL.
However, here you (now) seem to do it unconditionally. As a result this
seems to render all attempts to return either a proper FAST_IO_FAIL or
SUCCESS from a host_reset_handler() futile?
} else {
SCSI_LOG_ERROR_RECOVERY(3,
shost_printk(KERN_INFO, shost,
How is this related to just changing the callback argument?
Haven't we previously set the host_byte to anything?
Is the above part rather an independent change or even fix which should
live in a separate topical commit explaining why and what it does?
(I don't understand especially the DID_RESET cases.)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 8b93197..bc97e41 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -681,26 +681,26 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
return ret;
}
-static int virtscsi_device_reset(struct scsi_cmnd *sc)
+static int virtscsi_device_reset(struct scsi_device *sdev)
{
- struct virtio_scsi *vscsi = shost_priv(sc->device->host);
+ struct virtio_scsi *vscsi = shost_priv(sdev->host);
struct virtio_scsi_cmd *cmd;
- sdev_printk(KERN_INFO, sc->device, "device reset\n");
+ sdev_printk(KERN_INFO, sdev, "device reset\n");
cmd = mempool_alloc(virtscsi_cmd_pool, GFP_NOIO);
if (!cmd)
return FAILED;
memset(cmd, 0, sizeof(*cmd));
- cmd->sc = sc;
+ cmd->sc = NULL;
I hope we are protected to never land in virtscsi_complete_cmd() with
cmd->sc==NULL, not even e.g. if the virtio queue gets hot unplugged the
hard way in parallel (forcing some completion to drain).
I'm thinking of commit 773c7220e22d193e5667c352fcbf8d47eefc817f
("scsi: virtio_scsi: Reject commands when virtqueue is broken").
cmd->req.tmf = (struct virtio_scsi_ctrl_tmf_req){
.type = VIRTIO_SCSI_T_TMF,
.subtype = cpu_to_virtio32(vscsi->vdev,
VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET),
.lun[0] = 1,
- .lun[1] = sc->device->id,
- .lun[2] = (sc->device->lun >> 8) | 0x40,
- .lun[3] = sc->device->lun & 0xff,
+ .lun[1] = sdev->id,
+ .lun[2] = (sdev->lun >> 8) | 0x40,
+ .lun[3] = sdev->lun & 0xff,
};
return virtscsi_tmf(vscsi, cmd);
}
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 33bc523..7095076 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -145,7 +145,7 @@ struct scsi_host_template {
* Status: REQUIRED (at least one of them)
*/
int (* eh_abort_handler)(struct scsi_cmnd *);
- int (* eh_device_reset_handler)(struct scsi_cmnd *);
+ int (* eh_device_reset_handler)(struct scsi_device *);
int (* eh_target_reset_handler)(struct scsi_target *);
int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
int (* eh_host_reset_handler)(struct Scsi_Host *);
anything else, that I quoted but did not comment, looks good
--
Mit freundlichen Grüßen / Kind regards
Steffen Maier
Linux on z Systems Development
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
