On 11/1/19 5:26 PM, Bart Van Assche wrote:
On 10/31/19 4:04 AM, Hannes Reinecke wrote:
Use standard SAM status codes and omit the explicit shift to convert
to linux-specific ones.
Isn't an explicit shift required to convert *from* linux-specific codes
instead of for converting *to* linux-specific ones?
drivers/ata/libata-scsi.c | 2 +-
drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
drivers/scsi/3w-9xxx.c | 5 +++--
drivers/scsi/3w-sas.c | 3 ++-
drivers/scsi/3w-xxxx.c | 4 ++--
drivers/scsi/arcmsr/arcmsr_hba.c | 4 ++--
drivers/scsi/bfa/bfad_im.c | 2 +-
drivers/scsi/dc395x.c | 18 +++++-------------
drivers/scsi/dpt_i2o.c | 2 +-
drivers/scsi/gdth.c | 12 ++++++------
drivers/scsi/megaraid.c | 10 +++++-----
drivers/scsi/megaraid/megaraid_mbox.c | 12 ++++++------
drivers/scsi/pcmcia/nsp_cs.c | 2 +-
13 files changed, 36 insertions(+), 42 deletions(-)
Should this patch be split into one patch per driver such? If this patch
would introduce a regression that will make it easier to address such
regressions. Splitting this patch will also make reviewing easier.
If you think it's worthwhile ... sure.
I'm already at 24 patches; splitting this off would get me an additional
12 patches, all of which are basically one-liners.
diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index 3337b1e80412..ada77c136f8b 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -1018,7 +1018,8 @@ static int twa_fill_sense(TW_Device_Extension
*tw_dev, int request_id, int copy_
if (copy_sense) {
memcpy(tw_dev->srb[request_id]->sense_buffer,
full_command_packet->header.sense_data, TW_SENSE_DATA_LENGTH);
- tw_dev->srb[request_id]->result =
(full_command_packet->command.newcommand.status << 1);
+ tw_dev->srb[request_id]->result =
+ full_command_packet->command.newcommand.status;
retval = TW_ISR_DONT_RESULT;
goto out;
}
Does this change preserve the behavior of this LLD?
Hmm. Actually, I don't know; it feels a bit weird having a firmware
interface returning linux-specific status codes.
I'll do a bit of digging to see if this isn't an error in the driver.
diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
index dda6fa857709..d11f62c60877 100644
--- a/drivers/scsi/3w-sas.c
+++ b/drivers/scsi/3w-sas.c
@@ -891,7 +891,8 @@ static int twl_fill_sense(TW_Device_Extension
*tw_dev, int i, int request_id, in
if (copy_sense) {
memcpy(tw_dev->srb[request_id]->sense_buffer,
header->sense_data, TW_SENSE_DATA_LENGTH);
- tw_dev->srb[request_id]->result =
(full_command_packet->command.newcommand.status << 1);
+ tw_dev->srb[request_id]->result =
+ full_command_packet->command.newcommand.status;
goto out;
}
Same question here.
... and same answer here :-)
diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 6b5841b1c06e..e3cbe5d20aca 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -150,7 +150,7 @@ bfa_cb_tskim_done(void *bfad, struct bfad_tskim_s
*dtsk,
struct scsi_cmnd *cmnd = (struct scsi_cmnd *)dtsk;
wait_queue_head_t *wq;
- cmnd->SCp.Status |= tsk_status << 1;
+ cmnd->SCp.Status |= tsk_status;
set_bit(IO_DONE_BIT, (unsigned long *)&cmnd->SCp.Status);
wq = (wait_queue_head_t *) cmnd->SCp.ptr;
cmnd->SCp.ptr = NULL;
Same question here.
Here it's actually obvious; in it's current form SCp.status holds the
linux status, and it's always shifted back and forth when reading or
writing it to hardware-related structures.
So yeah, that does warrant a separate patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
