[PATCH 0/4] SCSI results rework preparation part 2

[PATCH 0/4] SCSI results rework preparation part 2

[Johannes Thumshirn]

As the first part of the preparation work for my scsi results handling
rework is done, here's the second batch.

The first patch changes the AAC_STAT_GOOD define into an open-coded
version in aacraid. The other three patches convert the three
ScsiResult() macros in bfa, lpfc and ncr53c8xx to their open-coded
equivalen.

This is helpful for subsequent refactoring, as it's easier to identify
which code parts need more work and Coccinelle Spatches are easier to
apply.

Johannes Thumshirn (4):
  scsi: aacraid: remove AAC_STAT_GOOD define
  scsi: bfa: remove ScsiResult macro
  scsi: lpfc: remove ScsiResult macro
  scsi: ncr53c8xx: remove ScsiResult macro

 drivers/scsi/aacraid/aachba.c | 41 ++++++++++++++++++++++++++--------------
 drivers/scsi/bfa/bfad_im.c    | 19 ++++++++-----------
 drivers/scsi/bfa/bfad_im.h    |  1 -
 drivers/scsi/lpfc/lpfc_crtn.h |  1 -
 drivers/scsi/lpfc/lpfc_scsi.c | 44 +++++++++++++++++++++----------------------
 drivers/scsi/ncr53c8xx.c      | 10 +++++-----
 6 files changed, 62 insertions(+), 54 deletions(-)

-- 
2.16.4

[PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Johannes Thumshirn]

Remove the AAC_STAT_GOOD definition and open code it in the places it
was used.

This will make subsequent refactoring in this area easier.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Dave Carroll <david.carroll@microsemi.com>
Cc: Raghava Aditya Renukunta <RaghavaAditya.Renukunta@microsemi.com>
---
 drivers/scsi/aacraid/aachba.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index a9831bd37a73..0b34d275523d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -115,8 +115,6 @@
 #define ASENCODE_LUN_FAILED_SELF_CONFIG		0x00
 #define ASENCODE_OVERLAPPED_COMMAND		0x00
 
-#define AAC_STAT_GOOD (DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD)
-
 #define BYTE0(x) (unsigned char)(x)
 #define BYTE1(x) (unsigned char)((x) >> 8)
 #define BYTE2(x) (unsigned char)((x) >> 16)
@@ -2962,7 +2960,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 
 	case SYNCHRONIZE_CACHE:
 		if (((aac_cache & 6) == 6) && dev->cache_protected) {
-			scsicmd->result = AAC_STAT_GOOD;
+			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+					  SAM_STAT_GOOD;
 			break;
 		}
 		/* Issue FIB to tell Firmware to flush it's cache */
@@ -2990,7 +2989,9 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				arr[1] = scsicmd->cmnd[2];
 				scsi_sg_copy_from_buffer(scsicmd, &inq_data,
 							 sizeof(inq_data));
-				scsicmd->result = AAC_STAT_GOOD;
+				scsicmd->result = DID_OK << 16 |
+						  COMMAND_COMPLETE << 8 |
+						  SAM_STAT_GOOD;
 			} else if (scsicmd->cmnd[2] == 0x80) {
 				/* unit serial number page */
 				arr[3] = setinqserial(dev, &arr[4],
@@ -3001,7 +3002,9 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				if (aac_wwn != 2)
 					return aac_get_container_serial(
 						scsicmd);
-				scsicmd->result = AAC_STAT_GOOD;
+				scsicmd->result = DID_OK << 16 |
+						  COMMAND_COMPLETE << 8 |
+						  SAM_STAT_GOOD;
 			} else if (scsicmd->cmnd[2] == 0x83) {
 				/* vpd page 0x83 - Device Identification Page */
 				char *sno = (char *)&inq_data;
@@ -3010,7 +3013,9 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				if (aac_wwn != 2)
 					return aac_get_container_serial(
 						scsicmd);
-				scsicmd->result = AAC_STAT_GOOD;
+				scsicmd->result = DID_OK << 16 |
+						  COMMAND_COMPLETE << 8 |
+						  SAM_STAT_GOOD;
 			} else {
 				/* vpd page not implemented */
 				scsicmd->result = DID_OK << 16 |
@@ -3041,7 +3046,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			inq_data.inqd_pdt = INQD_PDT_PROC;	/* Processor device */
 			scsi_sg_copy_from_buffer(scsicmd, &inq_data,
 						 sizeof(inq_data));
-			scsicmd->result = AAC_STAT_GOOD;
+			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+					  SAM_STAT_GOOD;
 			break;
 		}
 		if (dev->in_reset)
@@ -3090,7 +3096,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		/* Do not cache partition table for arrays */
 		scsicmd->device->removable = 1;
 
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 	}
 
@@ -3116,7 +3123,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		scsi_sg_copy_from_buffer(scsicmd, cp, sizeof(cp));
 		/* Do not cache partition table for arrays */
 		scsicmd->device->removable = 1;
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 	}
 
@@ -3195,7 +3203,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		scsi_sg_copy_from_buffer(scsicmd,
 					 (char *)&mpd,
 					 mode_buf_length);
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 	}
 	case MODE_SENSE_10:
@@ -3272,7 +3281,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 					 (char *)&mpd10,
 					 mode_buf_length);
 
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 	}
 	case REQUEST_SENSE:
@@ -3281,7 +3291,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				sizeof(struct sense_data));
 		memset(&dev->fsa_dev[cid].sense_data, 0,
 				sizeof(struct sense_data));
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 
 	case ALLOW_MEDIUM_REMOVAL:
@@ -3291,7 +3302,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		else
 			fsa_dev_ptr[cid].locked = 0;
 
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 	/*
 	 *	These commands are all No-Ops
@@ -3315,7 +3327,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 	case REZERO_UNIT:
 	case REASSIGN_BLOCKS:
 	case SEEK_10:
-		scsicmd->result = AAC_STAT_GOOD;
+		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+				  SAM_STAT_GOOD;
 		break;
 
 	case START_STOP:
-- 
2.16.4

Re: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Bart Van Assche]

On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
>  		if (((aac_cache & 6) == 6) && dev->cache_protected) {
> -			scsicmd->result = AAC_STAT_GOOD;
> +			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> +					  SAM_STAT_GOOD;

Does AAC_STAT_GOOD really have to be expanded in full multiple times?
Have you considered to replace AAC_STAT_GOOD by the numerical constant
0 instead?

Thanks,

Bart.

Re: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Johannes Thumshirn]

On Thu, Jul 05, 2018 at 05:24:23PM +0000, Bart Van Assche wrote:
> On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> >  		if (((aac_cache & 6) == 6) && dev->cache_protected) {
> > -			scsicmd->result = AAC_STAT_GOOD;
> > +			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> > +					  SAM_STAT_GOOD;
> 
> Does AAC_STAT_GOOD really have to be expanded in full multiple times?
> Have you considered to replace AAC_STAT_GOOD by the numerical constant
> 0 instead?

This won't work anymore once we start splitting ->result into 4
unrelated enums. That's why I prefer this way.

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Bart Van Assche]

On Fri, 2018-07-06 at 10:04 +0200, Johannes Thumshirn wrote:
> On Thu, Jul 05, 2018 at 05:24:23PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> > >  		if (((aac_cache & 6) == 6) && dev->cache_protected) {
> > > -			scsicmd->result = AAC_STAT_GOOD;
> > > +			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> > > +					  SAM_STAT_GOOD;
> > 
> > Does AAC_STAT_GOOD really have to be expanded in full multiple times?
> > Have you considered to replace AAC_STAT_GOOD by the numerical constant
> > 0 instead?
> 
> This won't work anymore once we start splitting ->result into 4
> unrelated enums. That's why I prefer this way.

That explanation does not make sense to me. There is plenty of code in
the upstream kernel that assigns zero to .result, so you will have to
deal with that pattern anyway.

Bart.

RE: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Dave Carroll]

> 
> 
> Remove the AAC_STAT_GOOD definition and open code it in the places it was
> used.
> 
> This will make subsequent refactoring in this area easier.
> 
Please don't ... the definition itself was added to make refactoring easier.

-Dave

Re: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Bart Van Assche]

On 07/05/18 10:42, Dave Carroll wrote:
>> Remove the AAC_STAT_GOOD definition and open code it in the places it was
>> used.
>>
>> This will make subsequent refactoring in this area easier.
>>
> Please don't ... the definition itself was added to make refactoring easier.

That's a vague statement. Which kind of refactoring do you plan to make?

Bart.

Re: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Johannes Thumshirn]

On Thu, Jul 05, 2018 at 05:42:02PM +0000, Dave Carroll wrote:
> > Remove the AAC_STAT_GOOD definition and open code it in the places it was
> > used.
> > 
> > This will make subsequent refactoring in this area easier.
> > 
> Please don't ... the definition itself was added to make refactoring easier.

Well in the end scsi_cmnd::result as we know it currently will go
away, so splitting up the define is rather crucial for this. I could
do it when changing the ->result from one integer into 4 u8s but doing
it in a separate preparation step would be preferable to me and it
doesn't hurt accraid at all.

For more information please see [1] and [2]. A first preparation
series [3] has already landed upstream.

[1] https://marc.info/?l=linux-scsi&m=152300071418035&w=2
[2] https://marc.info/?l=linux-scsi&m=152406381222604&w=2
[3] https://marc.info/?l=linux-scsi&m=152887646121085&w=2

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 1/4] scsi: aacraid: remove AAC_STAT_GOOD define

[Dave Carroll]

> 
> On Thu, Jul 05, 2018 at 05:42:02PM +0000, Dave Carroll wrote:
> > > Remove the AAC_STAT_GOOD definition and open code it in the places
> > > it was used.
> > >
> > > This will make subsequent refactoring in this area easier.
> > >
> > Please don't ... the definition itself was added to make refactoring easier.
> 
> Well in the end scsi_cmnd::result as we know it currently will go away, so
> splitting up the define is rather crucial for this. I could do it when changing the -
> >result from one integer into 4 u8s but doing it in a separate preparation step
> would be preferable to me and it doesn't hurt accraid at all.
> 

Thanks for the explanation Johannes, the routine is unwieldy already, and it just 
didn't feel right adding back all those lines ... Looking forward to the next step ...

Reviewed-by: Dave Carroll <david.carroll@microsemi.com>

[PATCH 2/4] scsi: bfa: remove ScsiResult macro

[Johannes Thumshirn]

Remove the ScsiResult macro and open code it on all call sites.

This will make subsequent refactoring in this area easier.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/bfa/bfad_im.c | 19 ++++++++-----------
 drivers/scsi/bfa/bfad_im.h |  1 -
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index c05d6e91e4bd..c4a33317d344 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -70,21 +70,18 @@ bfa_cb_ioim_done(void *drv, struct bfad_ioim_s *dio,
 				host_status = DID_ERROR;
 			}
 		}
-		cmnd->result = ScsiResult(host_status, scsi_status);
+		cmnd->result = host_status << 16 | scsi_status;
 
 		break;
 
 	case BFI_IOIM_STS_TIMEDOUT:
-		host_status = DID_TIME_OUT;
-		cmnd->result = ScsiResult(host_status, 0);
+		cmnd->result = DID_TIME_OUT << 16;
 		break;
 	case BFI_IOIM_STS_PATHTOV:
-		host_status = DID_TRANSPORT_DISRUPTED;
-		cmnd->result = ScsiResult(host_status, 0);
+		cmnd->result = DID_TRANSPORT_DISRUPTED << 16;
 		break;
 	default:
-		host_status = DID_ERROR;
-		cmnd->result = ScsiResult(host_status, 0);
+		cmnd->result = DID_ERROR << 16;
 	}
 
 	/* Unmap DMA, if host is NULL, it means a scsi passthru cmd */
@@ -117,7 +114,7 @@ bfa_cb_ioim_good_comp(void *drv, struct bfad_ioim_s *dio)
 	struct bfad_itnim_data_s *itnim_data;
 	struct bfad_itnim_s *itnim;
 
-	cmnd->result = ScsiResult(DID_OK, SCSI_STATUS_GOOD);
+	cmnd->result = DID_OK << 16 | SCSI_STATUS_GOOD;
 
 	/* Unmap DMA, if host is NULL, it means a scsi passthru cmd */
 	if (cmnd->device->host != NULL)
@@ -144,7 +141,7 @@ bfa_cb_ioim_abort(void *drv, struct bfad_ioim_s *dio)
 	struct scsi_cmnd *cmnd = (struct scsi_cmnd *)dio;
 	struct bfad_s         *bfad = drv;
 
-	cmnd->result = ScsiResult(DID_ERROR, 0);
+	cmnd->result = DID_ERROR << 16;
 
 	/* Unmap DMA, if host is NULL, it means a scsi passthru cmd */
 	if (cmnd->device->host != NULL)
@@ -1253,14 +1250,14 @@ bfad_im_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done) (struct scsi_cmnd
 		printk(KERN_WARNING
 			"bfad%d, queuecommand %p %x failed, BFA stopped\n",
 		       bfad->inst_no, cmnd, cmnd->cmnd[0]);
-		cmnd->result = ScsiResult(DID_NO_CONNECT, 0);
+		cmnd->result = DID_NO_CONNECT << 16;
 		goto out_fail_cmd;
 	}
 
 
 	itnim = itnim_data->itnim;
 	if (!itnim) {
-		cmnd->result = ScsiResult(DID_IMM_RETRY, 0);
+		cmnd->result = DID_IMM_RETRY << 16;
 		goto out_fail_cmd;
 	}
 
diff --git a/drivers/scsi/bfa/bfad_im.h b/drivers/scsi/bfa/bfad_im.h
index af66275570c3..e61ed8dad0b4 100644
--- a/drivers/scsi/bfa/bfad_im.h
+++ b/drivers/scsi/bfa/bfad_im.h
@@ -44,7 +44,6 @@ u32 bfad_im_supported_speeds(struct bfa_s *bfa);
 #define MAX_FCP_LUN 16384
 #define BFAD_TARGET_RESET_TMO 60
 #define BFAD_LUN_RESET_TMO 60
-#define ScsiResult(host_code, scsi_code) (((host_code) << 16) | scsi_code)
 #define BFA_QUEUE_FULL_RAMP_UP_TIME 120
 
 /*
-- 
2.16.4

Re: [PATCH 2/4] scsi: bfa: remove ScsiResult macro

[Bart Van Assche]

On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> -	cmnd->result = ScsiResult(DID_OK, SCSI_STATUS_GOOD);
> +	cmnd->result = DID_OK << 16 | SCSI_STATUS_GOOD;

Please consider to remove the SCSI_STATUS_GOOD constant since it is
non-standard and since it used by the bfa driver only. Additionally, since
SCSI_STATUS_GOOD == 0, please leave out "| SCSI_STATUS_GOOD".

Thanks,

Bart.

Re: [PATCH 2/4] scsi: bfa: remove ScsiResult macro

[Johannes Thumshirn]

On Thu, Jul 05, 2018 at 05:00:10PM +0000, Bart Van Assche wrote:
> On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> > -	cmnd->result = ScsiResult(DID_OK, SCSI_STATUS_GOOD);
> > +	cmnd->result = DID_OK << 16 | SCSI_STATUS_GOOD;
> 
> Please consider to remove the SCSI_STATUS_GOOD constant since it is
> non-standard and since it used by the bfa driver only. Additionally, since
> SCSI_STATUS_GOOD == 0, please leave out "| SCSI_STATUS_GOOD".

Can we agree on using SAM_STAT_GOOD here? I really don't want to leave
out the trailing "| 0" parts in this stage of the refactoring yet. I'm
still undecided if I don't want a set_scsi_cmnd_status(cmnd, foo, bar,
baz, frob); function in an intermediate step.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/4] scsi: lpfc: remove ScsiResult macro

[Johannes Thumshirn]

Remove the ScsiResult macro and open code it on all call sites.

This will make subsequent refactoring in this area easier.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: James Smart <james.smart@broadcom.com>
Cc: Dick Kennedy <dick.kennedy@broadcom.com>
---
 drivers/scsi/lpfc/lpfc_crtn.h |  1 -
 drivers/scsi/lpfc/lpfc_scsi.c | 44 +++++++++++++++++++++----------------------
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_crtn.h b/drivers/scsi/lpfc/lpfc_crtn.h
index 4ae9ba425e78..30f9e3ff6cb5 100644
--- a/drivers/scsi/lpfc/lpfc_crtn.h
+++ b/drivers/scsi/lpfc/lpfc_crtn.h
@@ -469,7 +469,6 @@ int lpfc_parse_vpd(struct lpfc_hba *, uint8_t *, int);
 void lpfc_start_fdiscs(struct lpfc_hba *phba);
 struct lpfc_vport *lpfc_find_vport_by_vpid(struct lpfc_hba *, uint16_t);
 struct lpfc_sglq *__lpfc_get_active_sglq(struct lpfc_hba *, uint16_t);
-#define ScsiResult(host_code, scsi_code) (((host_code) << 16) | scsi_code)
 #define HBA_EVENT_RSCN                   5
 #define HBA_EVENT_LINK_UP                2
 #define HBA_EVENT_LINK_DOWN              3
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index a94fb9f8bb44..924d7d672e9f 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -3017,8 +3017,8 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 	if (err_type == BGS_GUARD_ERR_MASK) {
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 					0x10, 0x1);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 		phba->bg_guard_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9069 BLKGRD: LBA %lx grd_tag error %x != %x\n",
@@ -3028,8 +3028,8 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 	} else if (err_type == BGS_REFTAG_ERR_MASK) {
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 					0x10, 0x3);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 
 		phba->bg_reftag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
@@ -3040,8 +3040,8 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 	} else if (err_type == BGS_APPTAG_ERR_MASK) {
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 					0x10, 0x2);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 
 		phba->bg_apptag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
@@ -3096,7 +3096,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd,
 	spin_unlock(&_dump_buf_lock);
 
 	if (lpfc_bgs_get_invalid_prof(bgstat)) {
-		cmd->result = ScsiResult(DID_ERROR, 0);
+		cmd->result = DID_ERROR << 16;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9072 BLKGRD: Invalid BG Profile in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
@@ -3108,7 +3108,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd,
 	}
 
 	if (lpfc_bgs_get_uninit_dif_block(bgstat)) {
-		cmd->result = ScsiResult(DID_ERROR, 0);
+		cmd->result = DID_ERROR << 16;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9073 BLKGRD: Invalid BG PDIF Block in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
@@ -3124,8 +3124,8 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd,
 
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 				0x10, 0x1);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 		phba->bg_guard_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9055 BLKGRD: Guard Tag error in cmd"
@@ -3140,8 +3140,8 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd,
 
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 				0x10, 0x3);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 
 		phba->bg_reftag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
@@ -3157,8 +3157,8 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd,
 
 		scsi_build_sense_buffer(1, cmd->sense_buffer, ILLEGAL_REQUEST,
 				0x10, 0x2);
-		cmd->result = DRIVER_SENSE << 24
-			| ScsiResult(DID_ABORT, SAM_STAT_CHECK_CONDITION);
+		cmd->result = DRIVER_SENSE << 24 | DID_ABORT << 16 |
+			      SAM_STAT_CHECK_CONDITION;
 
 		phba->bg_apptag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
@@ -3866,7 +3866,7 @@ lpfc_handle_fcp_err(struct lpfc_vport *vport, struct lpfc_scsi_buf *lpfc_cmd,
 	}
 
  out:
-	cmnd->result = ScsiResult(host_status, scsi_status);
+	cmnd->result = host_status << 16 | scsi_status;
 	lpfc_send_scsi_error_event(vport->phba, vport, lpfc_cmd, rsp_iocb);
 }
 
@@ -4019,7 +4019,7 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
 			break;
 		case IOSTAT_NPORT_BSY:
 		case IOSTAT_FABRIC_BSY:
-			cmd->result = ScsiResult(DID_TRANSPORT_DISRUPTED, 0);
+			cmd->result = DID_TRANSPORT_DISRUPTED << 16;
 			fast_path_evt = lpfc_alloc_fast_evt(phba);
 			if (!fast_path_evt)
 				break;
@@ -4053,14 +4053,14 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
 			    lpfc_cmd->result == IOERR_ELXSEC_CRYPTO_ERROR ||
 			    lpfc_cmd->result ==
 					IOERR_ELXSEC_CRYPTO_COMPARE_ERROR) {
-				cmd->result = ScsiResult(DID_NO_CONNECT, 0);
+				cmd->result = DID_NO_CONNECT << 16;
 				break;
 			}
 			if (lpfc_cmd->result == IOERR_INVALID_RPI ||
 			    lpfc_cmd->result == IOERR_NO_RESOURCES ||
 			    lpfc_cmd->result == IOERR_ABORT_REQUESTED ||
 			    lpfc_cmd->result == IOERR_SLER_CMD_RCV_FAILURE) {
-				cmd->result = ScsiResult(DID_REQUEUE, 0);
+				cmd->result = DID_REQUEUE << 16;
 				break;
 			}
 			if ((lpfc_cmd->result == IOERR_RX_DMA_FAILED ||
@@ -4094,16 +4094,16 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
 			}
 		/* else: fall through */
 		default:
-			cmd->result = ScsiResult(DID_ERROR, 0);
+			cmd->result = DID_ERROR << 16;
 			break;
 		}
 
 		if (!pnode || !NLP_CHK_NODE_ACT(pnode)
 		    || (pnode->nlp_state != NLP_STE_MAPPED_NODE))
-			cmd->result = ScsiResult(DID_TRANSPORT_DISRUPTED,
-						 SAM_STAT_BUSY);
+			cmd->result = DID_TRANSPORT_DISRUPTED << 16 |
+				      SAM_STAT_BUSY;
 	} else
-		cmd->result = ScsiResult(DID_OK, 0);
+		cmd->result = DID_OK << 16;
 
 	if (cmd->result || lpfc_cmd->fcp_rsp->rspSnsLen) {
 		uint32_t *lp = (uint32_t *)cmd->sense_buffer;
-- 
2.16.4

Re: [PATCH 3/4] scsi: lpfc: remove ScsiResult macro

[Bart Van Assche]

On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> Remove the ScsiResult macro and open code it on all call sites.
> 
> This will make subsequent refactoring in this area easier.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

[PATCH 4/4] scsi: ncr53c8xx: remove ScsiResult macro

[Johannes Thumshirn]

Remove the ScsiResult macro and open code it on all call sites.

This will make subsequent refactoring in this area easier.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/ncr53c8xx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ncr53c8xx.c b/drivers/scsi/ncr53c8xx.c
index dc4e801b2cef..6cd3e289ef99 100644
--- a/drivers/scsi/ncr53c8xx.c
+++ b/drivers/scsi/ncr53c8xx.c
@@ -4611,7 +4611,7 @@ static int ncr_reset_bus (struct ncb *np, struct scsi_cmnd *cmd, int sync_reset)
  * in order to keep it alive.
  */
 	if (!found && sync_reset && !retrieve_from_waiting_list(0, np, cmd)) {
-		cmd->result = ScsiResult(DID_RESET, 0);
+		cmd->result = DID_RESET << 16;
 		ncr_queue_done_cmd(np, cmd);
 	}
 
@@ -4957,7 +4957,7 @@ void ncr_complete (struct ncb *np, struct ccb *cp)
 		/*
 		**   Check condition code
 		*/
-		cmd->result = ScsiResult(DID_OK, S_CHECK_COND);
+		cmd->result = DID_OK << 16 | S_CHECK_COND;
 
 		/*
 		**	Copy back sense data to caller's buffer.
@@ -4978,7 +4978,7 @@ void ncr_complete (struct ncb *np, struct ccb *cp)
 		/*
 		**   Reservation Conflict condition code
 		*/
-		cmd->result = ScsiResult(DID_OK, S_CONFLICT);
+		cmd->result = DID_OK << 16 | S_CONFLICT;
 	
 	} else if ((cp->host_status == HS_COMPLETE)
 		&& (cp->scsi_status == S_BUSY ||
@@ -8043,7 +8043,7 @@ printk("ncr53c8xx_queue_command\n");
      spin_lock_irqsave(&np->smp_lock, flags);
 
      if ((sts = ncr_queue_command(np, cmd)) != DID_OK) {
-	  cmd->result = ScsiResult(sts, 0);
+	  cmd->result = sts << 16;
 #ifdef DEBUG_NCR53C8XX
 printk("ncr53c8xx : command not queued - result=%d\n", sts);
 #endif
@@ -8234,7 +8234,7 @@ static void process_waiting_list(struct ncb *np, int sts)
 #ifdef DEBUG_WAITING_LIST
 	printk("%s: cmd %lx done forced sts=%d\n", ncr_name(np), (u_long) wcmd, sts);
 #endif
-			wcmd->result = ScsiResult(sts, 0);
+			wcmd->result = sts << 16;
 			ncr_queue_done_cmd(np, wcmd);
 		}
 	}
-- 
2.16.4

Re: [PATCH 4/4] scsi: ncr53c8xx: remove ScsiResult macro

[Bart Van Assche]

On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> -		cmd->result = ScsiResult(DID_OK, S_CHECK_COND);
> +		cmd->result = DID_OK << 16 | S_CHECK_COND;

Since DID_OK == 0, do we really need "DID_OK << 16 |" here?

Thanks,

Bart.

Re: [PATCH 4/4] scsi: ncr53c8xx: remove ScsiResult macro

[Johannes Thumshirn]

On Thu, Jul 05, 2018 at 05:04:05PM +0000, Bart Van Assche wrote:
> On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> > -		cmd->result = ScsiResult(DID_OK, S_CHECK_COND);
> > +		cmd->result = DID_OK << 16 | S_CHECK_COND;
> 
> Since DID_OK == 0, do we really need "DID_OK << 16 |" here?

Please see my answers to the other patches on this.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/4] scsi: ncr53c8xx: remove ScsiResult macro

[Bart Van Assche]

On Fri, 2018-07-06 at 10:09 +0200, Johannes Thumshirn wrote:
> On Thu, Jul 05, 2018 at 05:04:05PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> > > -		cmd->result = ScsiResult(DID_OK, S_CHECK_COND);
> > > +		cmd->result = DID_OK << 16 | S_CHECK_COND;
> > 
> > Since DID_OK == 0, do we really need "DID_OK << 16 |" here?
> 
> Please see my answers to the other patches on this.

No matter what the next step is, you will have to deal with code that looks
very similar to what I proposed. An example from libata:

	else if (qc->flags & ATA_QCFLAG_SENSE_VALID)
		cmd->result = SAM_STAT_CHECK_CONDITION;

BTW, in drivers/scsi/sym53c8xx_2/sym_defs.h there are definitions for synonyms
of standard SCSI constants. I wouldn't mind if these synonyms would be removed:

#define	S_GOOD		SAM_STAT_GOOD
#define	S_CHECK_COND	SAM_STAT_CHECK_CONDITION
#define	S_COND_MET	SAM_STAT_CONDITION_MET
#define	S_BUSY		SAM_STAT_BUSY
#define	S_INT		SAM_STAT_INTERMEDIATE
#define	S_INT_COND_MET	SAM_STAT_INTERMEDIATE_CONDITION_MET
#define	S_CONFLICT	SAM_STAT_RESERVATION_CONFLICT
#define	S_TERMINATED	SAM_STAT_COMMAND_TERMINATED
#define	S_QUEUE_FULL	SAM_STAT_TASK_SET_FULL

Thanks,

Bart.

Re: [PATCH 4/4] scsi: ncr53c8xx: remove ScsiResult macro

[Johannes Thumshirn]

On Fri, Jul 06, 2018 at 05:05:19PM +0000, Bart Van Assche wrote:
> On Fri, 2018-07-06 at 10:09 +0200, Johannes Thumshirn wrote:
> > On Thu, Jul 05, 2018 at 05:04:05PM +0000, Bart Van Assche wrote:
> > > On Thu, 2018-07-05 at 13:01 +0200, Johannes Thumshirn wrote:
> > > > -		cmd->result = ScsiResult(DID_OK, S_CHECK_COND);
> > > > +		cmd->result = DID_OK << 16 | S_CHECK_COND;
> > > 
> > > Since DID_OK == 0, do we really need "DID_OK << 16 |" here?
> > 
> > Please see my answers to the other patches on this.
> 
> No matter what the next step is, you will have to deal with code that looks
> very similar to what I proposed. An example from libata:
> 
> 	else if (qc->flags & ATA_QCFLAG_SENSE_VALID)
> 		cmd->result = SAM_STAT_CHECK_CONDITION;

OK, I'll take remove the "| 0" parts.

> 
> BTW, in drivers/scsi/sym53c8xx_2/sym_defs.h there are definitions for synonyms
> of standard SCSI constants. I wouldn't mind if these synonyms would be removed:
> 
> #define	S_GOOD		SAM_STAT_GOOD
> #define	S_CHECK_COND	SAM_STAT_CHECK_CONDITION
> #define	S_COND_MET	SAM_STAT_CONDITION_MET
> #define	S_BUSY		SAM_STAT_BUSY
> #define	S_INT		SAM_STAT_INTERMEDIATE
> #define	S_INT_COND_MET	SAM_STAT_INTERMEDIATE_CONDITION_MET
> #define	S_CONFLICT	SAM_STAT_RESERVATION_CONFLICT
> #define	S_TERMINATED	SAM_STAT_COMMAND_TERMINATED
> #define	S_QUEUE_FULL	SAM_STAT_TASK_SET_FULL

Yup, saw those as well and already removed them in the next
version. Ditto for the bfa ones.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 0/4] SCSI results rework preparation part 2

[Martin K. Petersen]

Johannes,

> As the first part of the preparation work for my scsi results handling
> rework is done, here's the second batch.
>
> The first patch changes the AAC_STAT_GOOD define into an open-coded
> version in aacraid. The other three patches convert the three
> ScsiResult() macros in bfa, lpfc and ncr53c8xx to their open-coded
> equivalen.
>
> This is helpful for subsequent refactoring, as it's easier to identify
> which code parts need more work and Coccinelle Spatches are easier to
> apply.

Applied to 4.19/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering