Re: [PATCH 05/24] scsi: use standard SAM status codes

From: Bart Van Assche <bvanassche@acm.org>
To: Hannes Reinecke <hare@suse.de>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>,
	James Bottomley <james.bottomley@hansenpartnership.com>,
	linux-scsi@vger.kernel.org
Subject: Re: [PATCH 05/24] scsi: use standard SAM status codes
Date: Fri, 1 Nov 2019 09:26:13 -0700	[thread overview]
Message-ID: <8c3245df-97e3-95aa-498f-4ed47db96132@acm.org> (raw)
In-Reply-To: <20191031110452.73463-6-hare@suse.de>

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.

> 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?

> 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.

> 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.

Thanks,

Bart.