diff mbox

[3/3] scsi: don't add scsi command result bytes

Message ID 20180612135311.13216-4-jthumshirn@suse.de (mailing list archive)
State Changes Requested
Headers show

Commit Message

Johannes Thumshirn June 12, 2018, 1:53 p.m. UTC 
Some drivers are ADDing the scsi command's result bytes instead of
ORing them.

While this can produce correct results it has unexpected side effects.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/imm.c                  | 2 +-
 drivers/scsi/mesh.c                 | 4 ++--
 drivers/scsi/sym53c8xx_2/sym_glue.c | 2 +-
 drivers/scsi/sym53c8xx_2/sym_glue.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

kernel test robot June 12, 2018, 3:02 p.m. UTC | #1 
Hi Johannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on v4.17 next-20180612]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/Preparation-patch-set-for-SCSI-results-rework/20180612-221711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-x016-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/scsi/imm.c: In function 'imm_engine':
>> drivers/scsi/imm.c:895:35: warning: suggest parentheses around arithmetic in operand of '|' [-Wparentheses]
       cmd->result = DID_OK << 16 | l & STATUS_MASK;

vim +895 drivers/scsi/imm.c

   775	
   776	static int imm_engine(imm_struct *dev, struct scsi_cmnd *cmd)
   777	{
   778		unsigned short ppb = dev->base;
   779		unsigned char l = 0, h = 0;
   780		int retv, x;
   781	
   782		/* First check for any errors that may have occurred
   783		 * Here we check for internal errors
   784		 */
   785		if (dev->failed)
   786			return 0;
   787	
   788		switch (cmd->SCp.phase) {
   789		case 0:		/* Phase 0 - Waiting for parport */
   790			if (time_after(jiffies, dev->jstart + HZ)) {
   791				/*
   792				 * We waited more than a second
   793				 * for parport to call us
   794				 */
   795				imm_fail(dev, DID_BUS_BUSY);
   796				return 0;
   797			}
   798			return 1;	/* wait until imm_wakeup claims parport */
   799			/* Phase 1 - Connected */
   800		case 1:
   801			imm_connect(dev, CONNECT_EPP_MAYBE);
   802			cmd->SCp.phase++;
   803	
   804			/* Phase 2 - We are now talking to the scsi bus */
   805		case 2:
   806			if (!imm_select(dev, scmd_id(cmd))) {
   807				imm_fail(dev, DID_NO_CONNECT);
   808				return 0;
   809			}
   810			cmd->SCp.phase++;
   811	
   812			/* Phase 3 - Ready to accept a command */
   813		case 3:
   814			w_ctr(ppb, 0x0c);
   815			if (!(r_str(ppb) & 0x80))
   816				return 1;
   817	
   818			if (!imm_send_command(cmd))
   819				return 0;
   820			cmd->SCp.phase++;
   821	
   822			/* Phase 4 - Setup scatter/gather buffers */
   823		case 4:
   824			if (scsi_bufflen(cmd)) {
   825				cmd->SCp.buffer = scsi_sglist(cmd);
   826				cmd->SCp.this_residual = cmd->SCp.buffer->length;
   827				cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
   828			} else {
   829				cmd->SCp.buffer = NULL;
   830				cmd->SCp.this_residual = 0;
   831				cmd->SCp.ptr = NULL;
   832			}
   833			cmd->SCp.buffers_residual = scsi_sg_count(cmd) - 1;
   834			cmd->SCp.phase++;
   835			if (cmd->SCp.this_residual & 0x01)
   836				cmd->SCp.this_residual++;
   837			/* Phase 5 - Pre-Data transfer stage */
   838		case 5:
   839			/* Spin lock for BUSY */
   840			w_ctr(ppb, 0x0c);
   841			if (!(r_str(ppb) & 0x80))
   842				return 1;
   843	
   844			/* Require negotiation for read requests */
   845			x = (r_str(ppb) & 0xb8);
   846			dev->rd = (x & 0x10) ? 1 : 0;
   847			dev->dp = (x & 0x20) ? 0 : 1;
   848	
   849			if ((dev->dp) && (dev->rd))
   850				if (imm_negotiate(dev))
   851					return 0;
   852			cmd->SCp.phase++;
   853	
   854			/* Phase 6 - Data transfer stage */
   855		case 6:
   856			/* Spin lock for BUSY */
   857			w_ctr(ppb, 0x0c);
   858			if (!(r_str(ppb) & 0x80))
   859				return 1;
   860	
   861			if (dev->dp) {
   862				retv = imm_completion(cmd);
   863				if (retv == -1)
   864					return 0;
   865				if (retv == 0)
   866					return 1;
   867			}
   868			cmd->SCp.phase++;
   869	
   870			/* Phase 7 - Post data transfer stage */
   871		case 7:
   872			if ((dev->dp) && (dev->rd)) {
   873				if ((dev->mode == IMM_NIBBLE) || (dev->mode == IMM_PS2)) {
   874					w_ctr(ppb, 0x4);
   875					w_ctr(ppb, 0xc);
   876					w_ctr(ppb, 0xe);
   877					w_ctr(ppb, 0x4);
   878				}
   879			}
   880			cmd->SCp.phase++;
   881	
   882			/* Phase 8 - Read status/message */
   883		case 8:
   884			/* Check for data overrun */
   885			if (imm_wait(dev) != (unsigned char) 0xb8) {
   886				imm_fail(dev, DID_ERROR);
   887				return 0;
   888			}
   889			if (imm_negotiate(dev))
   890				return 0;
   891			if (imm_in(dev, &l, 1)) {	/* read status byte */
   892				/* Check for optional message byte */
   893				if (imm_wait(dev) == (unsigned char) 0xb8)
   894					imm_in(dev, &h, 1);
 > 895				cmd->result = DID_OK << 16 | l & STATUS_MASK;
   896			}
   897			if ((dev->mode == IMM_NIBBLE) || (dev->mode == IMM_PS2)) {
   898				w_ctr(ppb, 0x4);
   899				w_ctr(ppb, 0xc);
   900				w_ctr(ppb, 0xe);
   901				w_ctr(ppb, 0x4);
   902			}
   903			return 0;	/* Finished */
   904			break;
   905	
   906		default:
   907			printk("imm: Invalid scsi phase\n");
   908		}
   909		return 0;
   910	}
   911	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot June 12, 2018, 3:46 p.m. UTC | #2 
Hi Johannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on v4.17 next-20180612]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/Preparation-patch-set-for-SCSI-results-rework/20180612-221711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-x010-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from drivers/scsi/sym53c8xx_2/sym_fw.c:40:0:
   drivers/scsi/sym53c8xx_2/sym_glue.h: In function 'sym_set_cam_result_ok':
>> drivers/scsi/sym53c8xx_2/sym_glue.h:259:47: warning: suggest parentheses around arithmetic in operand of '|' [-Wparentheses]
     cmd->result = DID_OK << 16 | cp->ssss_status & 0x7f;
                                  ~~~~~~~~~~~~~~~~^~~~~~

vim +259 drivers/scsi/sym53c8xx_2/sym_glue.h

   252	
   253	/*
   254	 *  Build CAM result for a successful IO and for a failed IO.
   255	 */
   256	static inline void sym_set_cam_result_ok(struct sym_ccb *cp, struct scsi_cmnd *cmd, int resid)
   257	{
   258		scsi_set_resid(cmd, resid);
 > 259		cmd->result = DID_OK << 16 | cp->ssss_status & 0x7f;
   260	}
   261	void sym_set_cam_result_error(struct sym_hcb *np, struct sym_ccb *cp, int resid);
   262	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/scsi/imm.c b/drivers/scsi/imm.c
index 87c94191033b..e67033c8011f 100644
--- a/drivers/scsi/imm.c
+++ b/drivers/scsi/imm.c
@@ -892,7 +892,7 @@  static int imm_engine(imm_struct *dev, struct scsi_cmnd *cmd)
 			/* Check for optional message byte */
 			if (imm_wait(dev) == (unsigned char) 0xb8)
 				imm_in(dev, &h, 1);
-			cmd->result = (DID_OK << 16) + (l & STATUS_MASK);
+			cmd->result = DID_OK << 16 | l & STATUS_MASK;
 		}
 		if ((dev->mode == IMM_NIBBLE) || (dev->mode == IMM_PS2)) {
 			w_ctr(ppb, 0x4);
diff --git a/drivers/scsi/mesh.c b/drivers/scsi/mesh.c
index 1753e42826dd..3473a860e690 100644
--- a/drivers/scsi/mesh.c
+++ b/drivers/scsi/mesh.c
@@ -594,9 +594,9 @@  static void mesh_done(struct mesh_state *ms, int start_next)
 	ms->current_req = NULL;
 	tp->current_req = NULL;
 	if (cmd) {
-		cmd->result = (ms->stat << 16) + cmd->SCp.Status;
+		cmd->result = ms->stat << 16 | cmd->SCp.Status;
 		if (ms->stat == DID_OK)
-			cmd->result += (cmd->SCp.Message << 8);
+			cmd->result |= cmd->SCp.Message << 8;
 		if (DEBUG_TARGET(cmd)) {
 			printk(KERN_DEBUG "mesh_done: result = %x, data_ptr=%d, buflen=%d\n",
 			       cmd->result, ms->data_ptr, scsi_bufflen(cmd));
diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
index 7320d5fe4cbc..afdbd5a66083 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -252,7 +252,7 @@  void sym_set_cam_result_error(struct sym_hcb *np, struct sym_ccb *cp, int resid)
 		cam_status = sym_xerr_cam_status(DID_ERROR, cp->xerr_status);
 	}
 	scsi_set_resid(cmd, resid);
-	cmd->result = (drv_status << 24) + (cam_status << 16) + scsi_status;
+	cmd->result = drv_status << 24 | cam_status << 16 | scsi_status;
 }
 
 static int sym_scatter(struct sym_hcb *np, struct sym_ccb *cp, struct scsi_cmnd *cmd)
diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.h b/drivers/scsi/sym53c8xx_2/sym_glue.h
index 805369521df8..25c7478d8810 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.h
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.h
@@ -256,7 +256,7 @@  sym_get_cam_status(struct scsi_cmnd *cmd)
 static inline void sym_set_cam_result_ok(struct sym_ccb *cp, struct scsi_cmnd *cmd, int resid)
 {
 	scsi_set_resid(cmd, resid);
-	cmd->result = (((DID_OK) << 16) + ((cp->ssss_status) & 0x7f));
+	cmd->result = DID_OK << 16 | cp->ssss_status & 0x7f;
 }
 void sym_set_cam_result_error(struct sym_hcb *np, struct sym_ccb *cp, int resid);