diff mbox

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

Message ID 20180705110140.19545-5-jthumshirn@suse.de (mailing list archive)
State Accepted
Headers show

Commit Message

Johannes Thumshirn July 5, 2018, 11:01 a.m. UTC 
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(-)

Comments

Bart Van Assche July 5, 2018, 5:04 p.m. UTC | #1 
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.
Johannes Thumshirn July 6, 2018, 8:09 a.m. UTC | #2 
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
Bart Van Assche July 6, 2018, 5:05 p.m. UTC | #3 
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.
Johannes Thumshirn July 9, 2018, 7:15 a.m. UTC | #4 
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.
diff mbox

Patch

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);
 		}
 	}