| Message ID | 20210421174749.11221-16-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | SCSI result cleanup, part 2 | expand |
On 4/21/21 10:47 AM, Hannes Reinecke wrote: > Use accessors to set the SCSI result. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/NCR5380.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c > index d7594b794d3c..855edda9db41 100644 > --- a/drivers/scsi/NCR5380.c > +++ b/drivers/scsi/NCR5380.c > @@ -538,7 +538,7 @@ static void complete_cmd(struct Scsi_Host *instance, > > if (hostdata->sensing == cmd) { > /* Autosense processing ends here */ > - if (status_byte(cmd->result) != GOOD) { > + if (get_status_byte(cmd) != SAM_STAT_GOOD) { > scsi_eh_restore_cmnd(cmd, &hostdata->ses); > } else { > scsi_eh_restore_cmnd(cmd, &hostdata->ses); Do all SCSI devices from the nineties report SCSI status values with the lower bit set to 0? If so, can the status_byte() macro be removed entirely? Thanks, Bart.
On 4/21/21 11:11 PM, Bart Van Assche wrote: > On 4/21/21 10:47 AM, Hannes Reinecke wrote: >> Use accessors to set the SCSI result. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/NCR5380.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c >> index d7594b794d3c..855edda9db41 100644 >> --- a/drivers/scsi/NCR5380.c >> +++ b/drivers/scsi/NCR5380.c >> @@ -538,7 +538,7 @@ static void complete_cmd(struct Scsi_Host *instance, >> if (hostdata->sensing == cmd) { >> /* Autosense processing ends here */ >> - if (status_byte(cmd->result) != GOOD) { >> + if (get_status_byte(cmd) != SAM_STAT_GOOD) { >> scsi_eh_restore_cmnd(cmd, &hostdata->ses); >> } else { >> scsi_eh_restore_cmnd(cmd, &hostdata->ses); > > Do all SCSI devices from the nineties report SCSI status values with the > lower bit set to 0? If so, can the status_byte() macro be removed entirely? > As indicated in the previous reply, yes, that is the plan (removing the status_byte() macro). And the drivers will have to report SCSI status values with the lower bit cleared, otherwise the linux SCSI status codes would never have worked in the first place. So I'll be adding a new patch for dropping the 'status_byte()' macro in the next round. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On Wed, 21 Apr 2021, Hannes Reinecke wrote: > Use accessors to set the SCSI result. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/NCR5380.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c > index d7594b794d3c..855edda9db41 100644 > --- a/drivers/scsi/NCR5380.c > +++ b/drivers/scsi/NCR5380.c > @@ -538,7 +538,7 @@ static void complete_cmd(struct Scsi_Host *instance, > > if (hostdata->sensing == cmd) { > /* Autosense processing ends here */ > - if (status_byte(cmd->result) != GOOD) { > + if (get_status_byte(cmd) != SAM_STAT_GOOD) { > scsi_eh_restore_cmnd(cmd, &hostdata->ses); > } else { > scsi_eh_restore_cmnd(cmd, &hostdata->ses); > @@ -567,18 +567,19 @@ static int NCR5380_queue_command(struct Scsi_Host *instance, > struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd); > unsigned long flags; > > + set_host_byte(cmd, DID_OK); > + set_status_byte(cmd, SAM_STAT_GOOD); > #if (NDEBUG & NDEBUG_NO_WRITE) > switch (cmd->cmnd[0]) { > case WRITE_6: > case WRITE_10: > shost_printk(KERN_DEBUG, instance, "WRITE attempted with NDEBUG_NO_WRITE set\n"); > - cmd->result = (DID_ERROR << 16); > + set_host_byte(cmd, DID_ERROR); > cmd->scsi_done(cmd); > return 0; > } > #endif /* (NDEBUG & NDEBUG_NO_WRITE) */ > > - cmd->result = 0; > Please remove the superfluous blank line.
On 4/21/21 11:37 PM, Hannes Reinecke wrote: > On 4/21/21 11:11 PM, Bart Van Assche wrote: >> Do all SCSI devices from the nineties report SCSI status values with >> the lower bit set to 0? If so, can the status_byte() macro be removed >> entirely? >> > As indicated in the previous reply, yes, that is the plan (removing the > status_byte() macro). And the drivers will have to report SCSI status > values with the lower bit cleared, otherwise the linux SCSI status codes > would never have worked in the first place. Please elaborate the above further. My understanding is that SCSI-2 defines bits 0, 6 and 7 of the status byte as reserved while SAM-2 specifies that these bits must be zero for the status codes that also have been defined in SCSI-2. Is it safe to assume that all SCSI-2 devices set the reserved bits to zero? Thanks, Bart.
On 4/22/21 6:10 PM, Bart Van Assche wrote: > On 4/21/21 11:37 PM, Hannes Reinecke wrote: >> On 4/21/21 11:11 PM, Bart Van Assche wrote: >>> Do all SCSI devices from the nineties report SCSI status values with >>> the lower bit set to 0? If so, can the status_byte() macro be removed >>> entirely? >>> >> As indicated in the previous reply, yes, that is the plan (removing the >> status_byte() macro). And the drivers will have to report SCSI status >> values with the lower bit cleared, otherwise the linux SCSI status codes >> would never have worked in the first place. > > Please elaborate the above further. My understanding is that SCSI-2 > defines bits 0, 6 and 7 of the status byte as reserved while SAM-2 > specifies that these bits must be zero for the status codes that also > have been defined in SCSI-2. Is it safe to assume that all SCSI-2 > devices set the reserved bits to zero? > Yes. At least we haven't encountered any devices which do. And are quite unlikely to find any new or unknown SCSI-2 devices, as the market for SCSI parallel drives is _quite_ small :-) Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, 90409 Nürnberg GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c index d7594b794d3c..855edda9db41 100644 --- a/drivers/scsi/NCR5380.c +++ b/drivers/scsi/NCR5380.c @@ -538,7 +538,7 @@ static void complete_cmd(struct Scsi_Host *instance, if (hostdata->sensing == cmd) { /* Autosense processing ends here */ - if (status_byte(cmd->result) != GOOD) { + if (get_status_byte(cmd) != SAM_STAT_GOOD) { scsi_eh_restore_cmnd(cmd, &hostdata->ses); } else { scsi_eh_restore_cmnd(cmd, &hostdata->ses); @@ -567,18 +567,19 @@ static int NCR5380_queue_command(struct Scsi_Host *instance, struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd); unsigned long flags; + set_host_byte(cmd, DID_OK); + set_status_byte(cmd, SAM_STAT_GOOD); #if (NDEBUG & NDEBUG_NO_WRITE) switch (cmd->cmnd[0]) { case WRITE_6: case WRITE_10: shost_printk(KERN_DEBUG, instance, "WRITE attempted with NDEBUG_NO_WRITE set\n"); - cmd->result = (DID_ERROR << 16); + set_host_byte(cmd, DID_ERROR); cmd->scsi_done(cmd); return 0; } #endif /* (NDEBUG & NDEBUG_NO_WRITE) */ - cmd->result = 0; spin_lock_irqsave(&hostdata->lock, flags); @@ -1154,7 +1155,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd) if (!hostdata->selecting) return false; - cmd->result = DID_BAD_TARGET << 16; + set_host_byte(cmd, DID_BAD_TARGET); complete_cmd(instance, cmd); dsprintk(NDEBUG_SELECTION, instance, "target did not respond within 250ms\n"); @@ -1203,7 +1204,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd) NCR5380_transfer_pio(instance, &phase, &len, &data, 0); if (len) { NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE); - cmd->result = DID_ERROR << 16; + set_host_byte(cmd, DID_ERROR); complete_cmd(instance, cmd); dsprintk(NDEBUG_SELECTION, instance, "IDENTIFY message transfer failed\n"); ret = false; @@ -1743,7 +1744,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) shost_printk(KERN_DEBUG, instance, "NDEBUG_NO_DATAOUT set, attempted DATAOUT aborted\n"); sink = 1; do_abort(instance, 0); - cmd->result = DID_ERROR << 16; + set_host_byte(cmd, DID_ERROR); complete_cmd(instance, cmd); hostdata->connected = NULL; hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun); @@ -1826,9 +1827,8 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) hostdata->connected = NULL; hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun); - cmd->result &= ~0xffff; - cmd->result |= cmd->SCp.Status; - cmd->result |= cmd->SCp.Message << 8; + set_msg_byte(cmd, cmd->SCp.Message); + set_status_byte(cmd, cmd->SCp.Status); set_resid_from_SCp(cmd); @@ -1980,7 +1980,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) if (msgout == ABORT) { hostdata->connected = NULL; hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun); - cmd->result = DID_ERROR << 16; + set_host_byte(cmd, DID_ERROR); complete_cmd(instance, cmd); return; } @@ -2261,7 +2261,7 @@ static int NCR5380_abort(struct scsi_cmnd *cmd) if (list_del_cmd(&hostdata->unissued, cmd)) { dsprintk(NDEBUG_ABORT, instance, "abort: removed %p from issue queue\n", cmd); - cmd->result = DID_ABORT << 16; + set_host_byte(cmd, DID_ABORT); cmd->scsi_done(cmd); /* No tag or busy flag to worry about */ goto out; } @@ -2270,7 +2270,7 @@ static int NCR5380_abort(struct scsi_cmnd *cmd) dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p == selecting\n", cmd); hostdata->selecting = NULL; - cmd->result = DID_ABORT << 16; + set_host_byte(cmd, DID_ABORT); complete_cmd(instance, cmd); goto out; } @@ -2341,7 +2341,7 @@ static void bus_reset_cleanup(struct Scsi_Host *instance) */ if (hostdata->selecting) { - hostdata->selecting->result = DID_RESET << 16; + set_host_byte(hostdata->selecting, DID_RESET); complete_cmd(instance, hostdata->selecting); hostdata->selecting = NULL; } @@ -2399,7 +2399,7 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd) list_for_each_entry(ncmd, &hostdata->unissued, list) { struct scsi_cmnd *scmd = NCR5380_to_scmd(ncmd); - scmd->result = DID_RESET << 16; + set_host_byte(scmd, DID_RESET); scmd->scsi_done(scmd); } INIT_LIST_HEAD(&hostdata->unissued);
Use accessors to set the SCSI result. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/NCR5380.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)