On 06/24/2017 09:24 AM, Finn Thain wrote:
> On Fri, 23 Jun 2017, Hannes Reinecke wrote:
>
>> The bus reset handler really is a host reset, so move it to
>> eh_bus_reset_handler.
>>
>> Signed-off-by: Hannes Reinecke <hare@xxxxxxxx>
>> ---
>> drivers/scsi/NCR5380.c | 13 ++++++++-----
>> drivers/scsi/arm/cumana_1.c | 2 +-
>> drivers/scsi/arm/oak.c | 2 +-
>> drivers/scsi/atari_scsi.c | 6 +++---
>> drivers/scsi/dmx3191d.c | 2 +-
>> drivers/scsi/g_NCR5380.c | 4 ++--
>> drivers/scsi/mac_scsi.c | 4 ++--
>> drivers/scsi/sun3_scsi.c | 4 ++--
>> 8 files changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>> index acc3344..e877fb9 100644
>> --- a/drivers/scsi/NCR5380.c
>> +++ b/drivers/scsi/NCR5380.c
>> @@ -2296,24 +2296,24 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
>>
>>
>> /**
>> - * NCR5380_bus_reset - reset the SCSI bus
>> + * NCR5380_host_reset - reset the SCSI host
>> * @cmd: SCSI command undergoing EH
>> *
>> * Returns SUCCESS
>> */
>>
>> -static int NCR5380_bus_reset(struct scsi_cmnd *cmd)
>> +static int NCR5380_host_reset(struct scsi_cmnd *cmd)
>> {
>> struct Scsi_Host *instance = cmd->device->host;
>> struct NCR5380_hostdata *hostdata = shost_priv(instance);
>> int i;
>> unsigned long flags;
>> - struct NCR5380_cmd *ncmd;
>> + struct NCR5380_cmd *ncmd, *tmp;
>>
>
> Do you need to introduce another temporary command pointer for this?
>
>> spin_lock_irqsave(&hostdata->lock, flags);
>>
>> #if (NDEBUG & NDEBUG_ANY)
>> - scmd_printk(KERN_INFO, cmd, __func__);
>> + shost_printk(KERN_INFO, instance, __func__);
>> #endif
>> NCR5380_dprint(NDEBUG_ANY, instance);
>> NCR5380_dprint_phase(NDEBUG_ANY, instance);
>> @@ -2331,7 +2331,10 @@ static int NCR5380_bus_reset(struct scsi_cmnd *cmd)
>> * commands!
>> */
>>
>> - if (list_del_cmd(&hostdata->unissued, cmd)) {
>> + list_for_each_entry_safe(ncmd, tmp, &hostdata->unissued, list) {
>> + struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
>> +
>> + list_del_init(&ncmd->list);
>> cmd->result = DID_RESET << 16;
>> cmd->scsi_done(cmd);
>> }
>
> For the sake of consistency, why didn't you use the same style that is
> used later in this routine? I.e.
>
> list_for_each_entry(ncmd, &hostdata->unissued, list) {
> struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
>
> cmd->result = DID_RESET << 16;
> cmd->scsi_done(cmd);
> }
> INIT_LIST_HEAD(&hostdata->unissued);
>
> Either way,
>
> Acked-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
>
> Thanks.
>
As the driver was switch to using embedded private data area we need to
ensure that we're not touching the data area anymore once we call
->done(); the command might be reused after that.
Once the command is reused the 'list' structure in the embedded data
area will be reset, resulting in a list corruption for the 'unissued' list.
But as we're running under EH here where we don't have asynchronous I/O
submissions I guess we should be fine with your suggestion.
Will be updating the patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
