[PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Deepak R Varma]

When adding two bit-field mask values, an OR operation offers higher
performance over an arithmetic operation. So, convert such additions to
an OR based expressions.
Issue identified using orplus.cocci semantic patch script.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
 drivers/scsi/FlashPoint.c | 92 +++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c
index 3d9c56ac8224..1ecd7bd5a4ab 100644
--- a/drivers/scsi/FlashPoint.c
+++ b/drivers/scsi/FlashPoint.c
@@ -415,8 +415,8 @@ typedef struct SCCBscam_info {
 
 #define  DISABLE_INT       BIT(7)	/*Do not interrupt at end of cmd. */
 
-#define  HOST_WRT_CMD      ((DISABLE_INT + XFER_HOST_DMA + XFER_HOST_AUTO + XFER_DMA_8BIT))
-#define  HOST_RD_CMD       ((DISABLE_INT + XFER_DMA_HOST + XFER_HOST_AUTO + XFER_DMA_8BIT))
+#define  HOST_WRT_CMD      ((DISABLE_INT | XFER_HOST_DMA | XFER_HOST_AUTO | XFER_DMA_8BIT))
+#define  HOST_RD_CMD       ((DISABLE_INT | XFER_DMA_HOST | XFER_HOST_AUTO | XFER_DMA_8BIT))
 
 #define  hp_host_addr_lo      0x1C
 #define  hp_host_addr_hmi     0x1E
@@ -1892,7 +1892,7 @@ static int FlashPoint_HandleInterrupt(void *pcard)
 					   (unsigned char)0x00);
 				WR_HARPOON(ioport + hp_fifowrite, i);
 				WR_HARPOON(ioport + hp_autostart_3,
-					   (AUTO_IMMED + TAG_STRT));
+					   (AUTO_IMMED | TAG_STRT));
 			}
 		}
 
@@ -2218,14 +2218,14 @@ static unsigned char FPT_sfm(u32 port, struct sccb *pCurrSCCB)
 
 	message = RD_HARPOON(port + hp_scsidata_0);
 
-	WR_HARPOON(port + hp_scsisig, SCSI_ACK + S_MSGI_PH);
+	WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_MSGI_PH));
 
 	if (TimeOutLoop > 20000)
 		message = 0x00;	/* force message byte = 0 if Time Out on Req */
 
 	if ((RDW_HARPOON((port + hp_intstat)) & PARITY) &&
 	    (RD_HARPOON(port + hp_addstat) & SCSI_PAR_ERR)) {
-		WR_HARPOON(port + hp_scsisig, (SCSI_ACK + S_ILL_PH));
+		WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_ILL_PH));
 		WR_HARPOON(port + hp_xferstat, 0);
 		WR_HARPOON(port + hp_fiforead, 0);
 		WR_HARPOON(port + hp_fifowrite, 0);
@@ -2252,12 +2252,12 @@ static unsigned char FPT_sfm(u32 port, struct sccb *pCurrSCCB)
 
 			RD_HARPOON(port + hp_scsidata_0);
 
-			WR_HARPOON(port + hp_scsisig, (SCSI_ACK + S_ILL_PH));
+			WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_ILL_PH));
 
 		} while (1);
 
 	}
-	WR_HARPOON(port + hp_scsisig, (SCSI_ACK + S_ILL_PH));
+	WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_ILL_PH));
 	WR_HARPOON(port + hp_xferstat, 0);
 	WR_HARPOON(port + hp_fiforead, 0);
 	WR_HARPOON(port + hp_fifowrite, 0);
@@ -2385,7 +2385,7 @@ static void FPT_ssel(u32 port, unsigned char p_card)
 
 		currSCCB->Sccb_scsimsg = TARGET_RESET;
 
-		WR_HARPOON(port + hp_autostart_3, (SELECT + SELCHK_STRT));
+		WR_HARPOON(port + hp_autostart_3, (SELECT | SELCHK_STRT));
 		auto_loaded = 1;
 		currSCCB->Sccb_scsistat = SELECT_BDR_ST;
 
@@ -2421,7 +2421,7 @@ static void FPT_ssel(u32 port, unsigned char p_card)
 			    (MPM_OP + AMSG_OUT + currSCCB->Sccb_tag));
 		WRW_HARPOON((port + SYNC_MSGS + 4), (BRH_OP + ALWAYS + NP));
 
-		WR_HARPOON(port + hp_autostart_3, (SELECT + SELCHK_STRT));
+		WR_HARPOON(port + hp_autostart_3, (SELECT | SELCHK_STRT));
 		auto_loaded = 1;
 
 	}
@@ -2457,7 +2457,7 @@ static void FPT_ssel(u32 port, unsigned char p_card)
 					     currSCCB->Sccb_idmsg));
 
 				WR_HARPOON(port + hp_autostart_3,
-					   (SELECT + SELCHK_STRT));
+					   (SELECT | SELCHK_STRT));
 
 				/* Setup our STATE so we know what happened when
 				   the wheels fall off. */
@@ -2506,7 +2506,7 @@ static void FPT_ssel(u32 port, unsigned char p_card)
 				currSCCB->Sccb_scsistat = SELECT_Q_ST;
 
 				WR_HARPOON(port + hp_autostart_3,
-					   (SELECT + SELCHK_STRT));
+					   (SELECT | SELCHK_STRT));
 			}
 		}
 
@@ -2521,7 +2521,7 @@ static void FPT_ssel(u32 port, unsigned char p_card)
 			currSCCB->Sccb_scsistat = SELECT_ST;
 
 			WR_HARPOON(port + hp_autostart_3,
-				   (SELECT + SELCHK_STRT));
+				   (SELECT | SELCHK_STRT));
 		}
 
 		theCCB = (unsigned char *)&currSCCB->Cdb[0];
@@ -2826,7 +2826,7 @@ static void FPT_SendMsg(u32 port, unsigned char message)
 
 		WR_HARPOON(port + hp_scsidata_0, message);
 
-		WR_HARPOON(port + hp_scsisig, (SCSI_ACK + S_ILL_PH));
+		WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_ILL_PH));
 
 		ACCEPT_MSG(port);
 
@@ -2874,7 +2874,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 
 		ACCEPT_MSG(port);
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 	}
 
 	else if (message == COMMAND_COMPLETE) {
@@ -2895,7 +2895,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 
 		ACCEPT_MSG(port);
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 	}
 
 	else if (message == MESSAGE_REJECT) {
@@ -2979,7 +2979,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 				    ~(unsigned char)F_USE_CMD_Q;
 
 				WR_HARPOON(port + hp_autostart_1,
-					   (AUTO_IMMED + DISCONNECT_START));
+					   (AUTO_IMMED | DISCONNECT_START));
 
 			}
 		}
@@ -2994,7 +2994,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 
 			if (!(RDW_HARPOON((port + hp_intstat)) & BUS_FREE)) {
 				WR_HARPOON(port + hp_autostart_1,
-					   (AUTO_IMMED + DISCONNECT_START));
+					   (AUTO_IMMED | DISCONNECT_START));
 			}
 		}
 	}
@@ -3014,7 +3014,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 		if (currSCCB->Sccb_scsimsg != MSG_PARITY_ERROR)
 			ACCEPT_MSG(port);
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 	}
 
 	else {
@@ -3024,7 +3024,7 @@ static void FPT_sdecm(unsigned char message, u32 port, unsigned char p_card)
 
 		ACCEPT_MSG_ATN(port);
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 	}
 }
 
@@ -3069,27 +3069,25 @@ static void FPT_shandem(u32 port, unsigned char p_card, struct sccb *pCurrSCCB)
 					ACCEPT_MSG_ATN(port);
 
 					WR_HARPOON(port + hp_autostart_1,
-						   (AUTO_IMMED +
-						    DISCONNECT_START));
+						   (AUTO_IMMED | DISCONNECT_START));
 				}
 			} else {
 
 				pCurrSCCB->Sccb_scsimsg = MESSAGE_REJECT;
 				ACCEPT_MSG_ATN(port);
 
-				WR_HARPOON(port + hp_autostart_1,
-					   (AUTO_IMMED + DISCONNECT_START));
+				WR_HARPOON(port + hp_autostart_1, (AUTO_IMMED | DISCONNECT_START));
 			}
 		} else {
 			if (pCurrSCCB->Sccb_scsimsg != MSG_PARITY_ERROR)
 				ACCEPT_MSG(port);
 			WR_HARPOON(port + hp_autostart_1,
-				   (AUTO_IMMED + DISCONNECT_START));
+				   (AUTO_IMMED | DISCONNECT_START));
 		}
 	} else {
 		if (pCurrSCCB->Sccb_scsimsg == MSG_PARITY_ERROR)
 			WR_HARPOON(port + hp_autostart_1,
-				   (AUTO_IMMED + DISCONNECT_START));
+				   (AUTO_IMMED | DISCONNECT_START));
 	}
 }
 
@@ -3154,14 +3152,14 @@ static unsigned char FPT_sisyncn(u32 port, unsigned char p_card,
 
 		if (syncFlag == 0) {
 			WR_HARPOON(port + hp_autostart_3,
-				   (SELECT + SELCHK_STRT));
+				   (SELECT | SELCHK_STRT));
 			currTar_Info->TarStatus =
 			    ((currTar_Info->
 			      TarStatus & ~(unsigned char)TAR_SYNC_MASK) |
 			     (unsigned char)SYNC_TRYING);
 		} else {
 			WR_HARPOON(port + hp_autostart_3,
-				   (AUTO_IMMED + CMD_ONLY_STRT));
+				   (AUTO_IMMED | CMD_ONLY_STRT));
 		}
 
 		return 1;
@@ -3196,7 +3194,7 @@ static void FPT_stsyncn(u32 port, unsigned char p_card)
 
 	if ((sync_msg == 0x00) && (currSCCB->Sccb_scsimsg == MSG_PARITY_ERROR)) {
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 		return;
 	}
 
@@ -3206,7 +3204,7 @@ static void FPT_stsyncn(u32 port, unsigned char p_card)
 
 	if ((offset == 0x00) && (currSCCB->Sccb_scsimsg == MSG_PARITY_ERROR)) {
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 		return;
 	}
 
@@ -3290,7 +3288,7 @@ static void FPT_stsyncn(u32 port, unsigned char p_card)
 					   (unsigned char)SYNC_SUPPORTED);
 
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 	}
 
 	else {
@@ -3330,7 +3328,7 @@ static void FPT_sisyncr(u32 port, unsigned char sync_pulse,
 	WR_HARPOON(port + hp_portctrl_0, SCSI_PORT);
 	WRW_HARPOON((port + hp_intstat), CLR_ALL_INT_1);
 
-	WR_HARPOON(port + hp_autostart_3, (AUTO_IMMED + CMD_ONLY_STRT));
+	WR_HARPOON(port + hp_autostart_3, (AUTO_IMMED | CMD_ONLY_STRT));
 
 	while (!(RDW_HARPOON((port + hp_intstat)) & (BUS_FREE | AUTO_INT))) {
 	}
@@ -3372,7 +3370,7 @@ static unsigned char FPT_siwidn(u32 port, unsigned char p_card)
 			    (MPM_OP + AMSG_OUT + SM16BIT));
 		WRW_HARPOON((port + SYNC_MSGS + 10), (BRH_OP + ALWAYS + NP));
 
-		WR_HARPOON(port + hp_autostart_3, (SELECT + SELCHK_STRT));
+		WR_HARPOON(port + hp_autostart_3, (SELECT | SELCHK_STRT));
 
 		currTar_Info->TarStatus = ((currTar_Info->TarStatus &
 					    ~(unsigned char)TAR_WIDE_MASK) |
@@ -3413,7 +3411,7 @@ static void FPT_stwidn(u32 port, unsigned char p_card)
 
 	if ((width == 0x00) && (currSCCB->Sccb_scsimsg == MSG_PARITY_ERROR)) {
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 		return;
 	}
 
@@ -3445,7 +3443,7 @@ static void FPT_stwidn(u32 port, unsigned char p_card)
 		} else {
 			ACCEPT_MSG(port);
 			WR_HARPOON(port + hp_autostart_1,
-				   (AUTO_IMMED + DISCONNECT_START));
+				   (AUTO_IMMED | DISCONNECT_START));
 		}
 	}
 
@@ -3487,7 +3485,7 @@ static void FPT_siwidr(u32 port, unsigned char width)
 	WR_HARPOON(port + hp_portctrl_0, SCSI_PORT);
 	WRW_HARPOON((port + hp_intstat), CLR_ALL_INT_1);
 
-	WR_HARPOON(port + hp_autostart_3, (AUTO_IMMED + CMD_ONLY_STRT));
+	WR_HARPOON(port + hp_autostart_3, (AUTO_IMMED | CMD_ONLY_STRT));
 
 	while (!(RDW_HARPOON((port + hp_intstat)) & (BUS_FREE | AUTO_INT))) {
 	}
@@ -3751,7 +3749,7 @@ static void FPT_sxfrp(u32 p_port, unsigned char p_card)
 
 	if (!(RDW_HARPOON((p_port + hp_intstat)) & (BUS_FREE | RESET))) {
 		WR_HARPOON(p_port + hp_autostart_0,
-			   (AUTO_IMMED + DISCONNECT_START));
+			   (AUTO_IMMED | DISCONNECT_START));
 		while (!(RDW_HARPOON((p_port + hp_intstat)) & AUTO_INT)) {
 		}
 
@@ -3987,7 +3985,7 @@ static void FPT_phaseDataOut(u32 port, unsigned char p_card)
 
 	WRW_HARPOON((port + hp_intstat), XFER_CNT_0);
 
-	WR_HARPOON(port + hp_autostart_0, (END_DATA + END_DATA_START));
+	WR_HARPOON(port + hp_autostart_0, (END_DATA | END_DATA_START));
 
 	FPT_dataXferProcessor(port, &FPT_BL_Card[p_card]);
 
@@ -4030,7 +4028,7 @@ static void FPT_phaseDataIn(u32 port, unsigned char p_card)
 
 	WRW_HARPOON((port + hp_intstat), XFER_CNT_0);
 
-	WR_HARPOON(port + hp_autostart_0, (END_DATA + END_DATA_START));
+	WR_HARPOON(port + hp_autostart_0, (END_DATA | END_DATA_START));
 
 	FPT_dataXferProcessor(port, &FPT_BL_Card[p_card]);
 
@@ -4115,7 +4113,7 @@ static void FPT_phaseStatus(u32 port, unsigned char p_card)
 
 	WR_HARPOON(port + hp_scsisig, 0x00);
 
-	WR_HARPOON(port + hp_autostart_0, (AUTO_IMMED + END_DATA_START));
+	WR_HARPOON(port + hp_autostart_0, (AUTO_IMMED | END_DATA_START));
 }
 
 /*---------------------------------------------------------------------
@@ -4199,7 +4197,7 @@ static void FPT_phaseMsgOut(u32 port, unsigned char p_card)
 
 	WR_HARPOON(port + hp_scsidata_0, message);
 
-	WR_HARPOON(port + hp_scsisig, (SCSI_ACK + S_ILL_PH));
+	WR_HARPOON(port + hp_scsisig, (SCSI_ACK | S_ILL_PH));
 
 	ACCEPT_MSG(port);
 
@@ -4251,7 +4249,7 @@ static void FPT_phaseMsgOut(u32 port, unsigned char p_card)
 		if (message == MSG_PARITY_ERROR) {
 			currSCCB->Sccb_scsimsg = NOP;
 			WR_HARPOON(port + hp_autostart_1,
-				   (AUTO_IMMED + DISCONNECT_START));
+				   (AUTO_IMMED | DISCONNECT_START));
 		} else {
 			FPT_sxfrp(port, p_card);
 		}
@@ -4282,7 +4280,7 @@ static void FPT_phaseMsgIn(u32 port, unsigned char p_card)
 	if ((message == DISCONNECT) || (message == SAVE_POINTERS)) {
 
 		WR_HARPOON(port + hp_autostart_1,
-			   (AUTO_IMMED + END_DATA_START));
+			   (AUTO_IMMED | END_DATA_START));
 
 	}
 
@@ -4297,7 +4295,7 @@ static void FPT_phaseMsgIn(u32 port, unsigned char p_card)
 			if (currSCCB->Sccb_scsimsg != MSG_PARITY_ERROR)
 				ACCEPT_MSG(port);
 			WR_HARPOON(port + hp_autostart_1,
-				   (AUTO_IMMED + DISCONNECT_START));
+				   (AUTO_IMMED | DISCONNECT_START));
 		}
 	}
 
@@ -6134,7 +6132,7 @@ static unsigned char FPT_scsell(u32 p_port, unsigned char targ_id)
 		while (!(RDW_HARPOON((p_port + hp_intstat)) & BUS_FREE)) {
 			if (RD_HARPOON(p_port + hp_scsisig) & SCSI_REQ) {
 				WR_HARPOON(p_port + hp_scsisig,
-					   (SCSI_ACK + S_ILL_PH));
+					   (SCSI_ACK | S_ILL_PH));
 				ACCEPT_MSG(p_port);
 			}
 		}
@@ -7284,7 +7282,7 @@ static void FPT_utilEEWrite(u32 p_port, unsigned short ee_data,
 
 	FPT_utilEESendCmdAddr(p_port, EE_WRITE, ee_addr);
 
-	ee_value |= (SEE_MS + SEE_CS);
+	ee_value |= (SEE_MS | SEE_CS);
 
 	for (i = 0x8000; i != 0; i >>= 1) {
 
@@ -7364,7 +7362,7 @@ static unsigned short FPT_utilEEReadOrg(u32 p_port, unsigned short ee_addr)
 
 	FPT_utilEESendCmdAddr(p_port, EE_READ, ee_addr);
 
-	ee_value |= (SEE_MS + SEE_CS);
+	ee_value |= (SEE_MS | SEE_CS);
 	ee_data = 0;
 
 	for (i = 1; i <= 16; i++) {
@@ -7382,7 +7380,7 @@ static unsigned short FPT_utilEEReadOrg(u32 p_port, unsigned short ee_addr)
 			ee_data |= 1;
 	}
 
-	ee_value &= ~(SEE_MS + SEE_CS);
+	ee_value &= ~(SEE_MS | SEE_CS);
 	WR_HARPOON(p_port + hp_ee_ctrl, (ee_value | SEE_MS));	/*Turn off CS */
 	WR_HARPOON(p_port + hp_ee_ctrl, ee_value);	/*Turn off Master Select */
 
-- 
2.34.1

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[James Bottomley]

On Tue, 2023-02-07 at 16:51 +0530, Deepak R Varma wrote:
> When adding two bit-field mask values, an OR operation offers higher
> performance over an arithmetic operation. So, convert such additions
> to an OR based expressions. Issue identified using orplus.cocci
> semantic patch script.

No it doesn't, at least not for constants, which is the entirety of
this patch: the compiler can find the value at compile time, so the
whole lot becomes a load immediate of a single value.  Whether the
compiler sees OR or + is immaterial to the compile time computation. 
Perhaps Coccinelle should be fixed not to complain about this case?

James

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Khalid Aziz]

On 2/7/23 05:26, James Bottomley wrote:
> On Tue, 2023-02-07 at 16:51 +0530, Deepak R Varma wrote:
>> When adding two bit-field mask values, an OR operation offers higher
>> performance over an arithmetic operation. So, convert such additions
>> to an OR based expressions. Issue identified using orplus.cocci
>> semantic patch script.
> 
> No it doesn't, at least not for constants, which is the entirety of
> this patch: the compiler can find the value at compile time, so the
> whole lot becomes a load immediate of a single value.  Whether the
> compiler sees OR or + is immaterial to the compile time computation.
> Perhaps Coccinelle should be fixed not to complain about this case?
> 
> James
> 

Agreed. This would be lot of code changes for no benefit.

--
Khalid

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Deepak R Varma]

On Tue, Feb 07, 2023 at 03:26:48PM +0300, James Bottomley wrote:
> On Tue, 2023-02-07 at 16:51 +0530, Deepak R Varma wrote:
> > When adding two bit-field mask values, an OR operation offers higher
> > performance over an arithmetic operation. So, convert such additions
> > to an OR based expressions. Issue identified using orplus.cocci
> > semantic patch script.
> 
> No it doesn't, at least not for constants, which is the entirety of
> this patch: the compiler can find the value at compile time, so the
> whole lot becomes a load immediate of a single value.  Whether the
> compiler sees OR or + is immaterial to the compile time computation. 

Hello James,
Thank you for the feedback. Your comments are useful. I was not aware of the
compile time computation of constant expressions before. Thanks.

> Perhaps Coccinelle should be fixed not to complain about this case?
Yes, sure. I will review the semantic patch and deterrmine if and how to fine
tune the same to avoid false positives.

> 
> James

James, there are a few other patch submissions for the scsi subsystem that I
submitted in last few weeks. I sent couple of reminder request for comments on
those submission, but still waiting. Could you please also review those and
share your feedback?

Best Regards,
deepak.

> 

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Deepak R Varma]

On Tue, Feb 07, 2023 at 08:28:22AM -0700, Khalid Aziz wrote:
> On 2/7/23 05:26, James Bottomley wrote:
> > On Tue, 2023-02-07 at 16:51 +0530, Deepak R Varma wrote:
> > > When adding two bit-field mask values, an OR operation offers higher
> > > performance over an arithmetic operation. So, convert such additions
> > > to an OR based expressions. Issue identified using orplus.cocci
> > > semantic patch script.
> > 
> > No it doesn't, at least not for constants, which is the entirety of
> > this patch: the compiler can find the value at compile time, so the
> > whole lot becomes a load immediate of a single value.  Whether the
> > compiler sees OR or + is immaterial to the compile time computation.
> > Perhaps Coccinelle should be fixed not to complain about this case?
> > 
> > James
> > 
> 
> Agreed. This would be lot of code changes for no benefit.

Hi Khalid,
I understand. Please disregard/drop the patch proposal.

Regards,
deepak.

> 
> --
> Khalid

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[James Bottomley]

On Tue, 2023-02-07 at 23:16 +0530, Deepak R Varma wrote:
[...]
> James, there are a few other patch submissions for the scsi subsystem
> that I submitted in last few weeks. I sent couple of reminder request
> for comments on those submission, but still waiting. Could you please
> also review those and share your feedback?

The problem is basically that they don't fix a bug or introduce an
enhancement.  Review bandwidth in SCSI is the main limiting factor for
any new patch, so we tend to concentrate on these two types.  The main
problem with code replacement patches is that they do tend to introduce
inadvertent bugs in old drivers, which is why people are afraid to
review them.  You can reduce the overhead of review by demonstrating
that the binary produced is unchanged (obviously this works for some
but not all of your patches), so a maintainer need only decide if they
like the change rather than worrying about it introducing a regression.

Regards,

James

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Martin K. Petersen]

Hi Deepak!

> James, there are a few other patch submissions for the scsi subsystem
> that I submitted in last few weeks. I sent couple of reminder request
> for comments on those submission, but still waiting. Could you please
> also review those and share your feedback?

I only merge cleanup patches if the relevant driver maintainer reviews
and acks them. And there needs to be sufficient justification, of
course.

As an example I will observe that your sysfs patches say:

"According to Documentation/filesystems/sysfs.rst, the show() callback
function of kobject attributes should strictly use sysfs_emit() instead
of sprintf() family functions."

That's a "should", not a "shall". IOW, a recommendation. Also, there is
no "strictly" requirement to be found in sysfs.rst. So the patch
justification is lacking.

We definitely use sysfs_emit() for new code. But it is not clear that
there is any value in changing old code predating sysfs_emit() to comply
with a mere recommendation. It's just additional work and comes with a
substantial risk of introducing regressions since virtually no cleanup
patches have actually been tested on the relevant hardware.

Nobody has access to all the devices required to validate the many, many
drivers we have in SCSI. So unless the driver maintainer (who presumably
has hardware) is willing to test, it is impossible for me to qualify
whether a patch introduces a regression.

One option is demonstrating that the patch does not change the generated
object code as James suggested. But even if the binary is unchanged,
cleanups often involve stylistic changes or switching to a more "modern"
approach. And for those changes I still want the driver maintainer to
ack. It's their driver.

To pick another example from your posted patches, it not immediately
obvious that using min() is superior to an if-else statement. Sometimes
it is clearer to express things as a conditional even though it takes up
more vertical space (or maybe because it does?). In any case that's a
personal preference and the driver maintainer's choice.

Hope that helps!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

[Deepak R Varma]

On Tue, Feb 07, 2023 at 03:19:52PM -0500, Martin K. Petersen wrote:
> 
> Hi Deepak!
> 
> > James, there are a few other patch submissions for the scsi subsystem
> > that I submitted in last few weeks. I sent couple of reminder request
> > for comments on those submission, but still waiting. Could you please
> > also review those and share your feedback?
> 
> I only merge cleanup patches if the relevant driver maintainer reviews
> and acks them. And there needs to be sufficient justification, of
> course.
> 
> As an example I will observe that your sysfs patches say:
> 
> "According to Documentation/filesystems/sysfs.rst, the show() callback
> function of kobject attributes should strictly use sysfs_emit() instead
> of sprintf() family functions."
> 
> That's a "should", not a "shall". IOW, a recommendation. Also, there is
> no "strictly" requirement to be found in sysfs.rst. So the patch
> justification is lacking.
> 
> We definitely use sysfs_emit() for new code. But it is not clear that
> there is any value in changing old code predating sysfs_emit() to comply
> with a mere recommendation. It's just additional work and comes with a
> substantial risk of introducing regressions since virtually no cleanup
> patches have actually been tested on the relevant hardware.
> 
> Nobody has access to all the devices required to validate the many, many
> drivers we have in SCSI. So unless the driver maintainer (who presumably
> has hardware) is willing to test, it is impossible for me to qualify
> whether a patch introduces a regression.
> 
> One option is demonstrating that the patch does not change the generated
> object code as James suggested. But even if the binary is unchanged,
> cleanups often involve stylistic changes or switching to a more "modern"
> approach. And for those changes I still want the driver maintainer to
> ack. It's their driver.
> 
> To pick another example from your posted patches, it not immediately
> obvious that using min() is superior to an if-else statement. Sometimes
> it is clearer to express things as a conditional even though it takes up
> more vertical space (or maybe because it does?). In any case that's a
> personal preference and the driver maintainer's choice.
> 
> Hope that helps!

Hello James and Martin,
I appreciate your comments and the detailed response. Thank you very much. I
understand the proposals are mostly clean up and not a significant benefit to
the code in terms of bug-fix or improvement. I will leave the patches at the
current status if any driver maintainer wants to take those forward.

Thank you again and apologies for any inconvenience.
./drv

> 
> -- 
> Martin K. Petersen	Oracle Linux Engineering