| Message ID | 20210817091456.73342-8-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SCSI EH argument reshuffle part II | expand |
On Tue, Aug 17, 2021 at 11:14:12AM +0200, Hannes Reinecke wrote: > When calling a host reset we shouldn't rely on the command triggering > the reset, so allow megaraid_abort_and_reset() to be called with a > NULL scb. > And drop the pointless 'bus_reset' and 'target_reset' handlers, which > just call the same function as host_reset. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/megaraid.c | 42 ++++++++++++++++------------------------- > 1 file changed, 16 insertions(+), 26 deletions(-) > > diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c > index 56910e94dbf2..7c53933fb1b4 100644 > --- a/drivers/scsi/megaraid.c > +++ b/drivers/scsi/megaraid.c > @@ -1905,7 +1905,7 @@ megaraid_reset(struct scsi_cmnd *cmd) > > spin_lock_irq(&adapter->lock); > > - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); > + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); > > /* > * This is required here to complete any completed requests > @@ -1944,7 +1944,7 @@ megaraid_abort_and_reset(adapter_t *adapter, struct scsi_cmnd *cmd, int aor) > > scb = list_entry(pos, scb_t, list); Ther's a dev_warn before this, which dereferences cmd. > - if (scb->cmd == cmd) { /* Found command */ > + if (!cmd || scb->cmd == cmd) { /* Found command */ > > scb->state |= aor; But more importantly, this function doesn't make much sense for the !cmd case, as it doesn't really do anything when not matched. It seems like the legacy megaraid driver should just stop calling it for resets.
On 8/17/21 2:26 PM, Christoph Hellwig wrote: > On Tue, Aug 17, 2021 at 11:14:12AM +0200, Hannes Reinecke wrote: >> When calling a host reset we shouldn't rely on the command triggering >> the reset, so allow megaraid_abort_and_reset() to be called with a >> NULL scb. >> And drop the pointless 'bus_reset' and 'target_reset' handlers, which >> just call the same function as host_reset. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/megaraid.c | 42 ++++++++++++++++------------------------- >> 1 file changed, 16 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c >> index 56910e94dbf2..7c53933fb1b4 100644 >> --- a/drivers/scsi/megaraid.c >> +++ b/drivers/scsi/megaraid.c >> @@ -1905,7 +1905,7 @@ megaraid_reset(struct scsi_cmnd *cmd) >> >> spin_lock_irq(&adapter->lock); >> >> - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); >> + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); >> >> /* >> * This is required here to complete any completed requests >> @@ -1944,7 +1944,7 @@ megaraid_abort_and_reset(adapter_t *adapter, struct scsi_cmnd *cmd, int aor) >> >> scb = list_entry(pos, scb_t, list); > > Ther's a dev_warn before this, which dereferences cmd. > >> - if (scb->cmd == cmd) { /* Found command */ >> + if (!cmd || scb->cmd == cmd) { /* Found command */ >> >> scb->state |= aor; > > But more importantly, this function doesn't make much sense for the > !cmd case, as it doesn't really do anything when not matched. It > seems like the legacy megaraid driver should just stop calling it > for resets. > Well, it does the usual RAID HBA host reset: wait for commands to complete (That's the '!cmd' case). So as such I guess there is a value in having a host reset function; we might, however, look at separating the functions. Cheers, Hannes
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index 56910e94dbf2..7c53933fb1b4 100644 --- a/drivers/scsi/megaraid.c +++ b/drivers/scsi/megaraid.c @@ -1905,7 +1905,7 @@ megaraid_reset(struct scsi_cmnd *cmd) spin_lock_irq(&adapter->lock); - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); /* * This is required here to complete any completed requests @@ -1944,7 +1944,7 @@ megaraid_abort_and_reset(adapter_t *adapter, struct scsi_cmnd *cmd, int aor) scb = list_entry(pos, scb_t, list); - if (scb->cmd == cmd) { /* Found command */ + if (!cmd || scb->cmd == cmd) { /* Found command */ scb->state |= aor; @@ -1963,31 +1963,23 @@ megaraid_abort_and_reset(adapter_t *adapter, struct scsi_cmnd *cmd, int aor) return FAILED; } - else { - - /* - * Not yet issued! Remove from the pending - * list - */ - dev_warn(&adapter->dev->dev, - "%s-[%x], driver owner\n", - (aor==SCB_ABORT) ? "ABORTING":"RESET", - scb->idx); - - mega_free_scb(adapter, scb); - - if( aor == SCB_ABORT ) { - cmd->result = (DID_ABORT << 16); - } - else { - cmd->result = (DID_RESET << 16); - } + /* + * Not yet issued! Remove from the pending + * list + */ + dev_warn(&adapter->dev->dev, + "%s-[%x], driver owner\n", + (cmd) ? "ABORTING":"RESET", + scb->idx); + mega_free_scb(adapter, scb); + if (cmd) { + cmd->result = (DID_ABORT << 16); list_add_tail(SCSI_LIST(cmd), - &adapter->completed_list); - - return SUCCESS; + &adapter->completed_list); } + + return SUCCESS; } } @@ -4135,8 +4127,6 @@ static struct scsi_host_template megaraid_template = { .sg_tablesize = MAX_SGLIST, .cmd_per_lun = DEF_CMD_PER_LUN, .eh_abort_handler = megaraid_abort, - .eh_device_reset_handler = megaraid_reset, - .eh_bus_reset_handler = megaraid_reset, .eh_host_reset_handler = megaraid_reset, .no_write_same = 1, };
When calling a host reset we shouldn't rely on the command triggering the reset, so allow megaraid_abort_and_reset() to be called with a NULL scb. And drop the pointless 'bus_reset' and 'target_reset' handlers, which just call the same function as host_reset. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/megaraid.c | 42 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)