[PATCH v4 1/7] scsi: hpsa: Eliminate duplicate barriers on weakly-ordered archs

[PATCH v4 1/7] scsi: hpsa: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes wmb() followed by writel(). writel() already has a
barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing
the register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/hpsa.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 018f980..c7d7e6a 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -599,7 +599,7 @@ static unsigned long SA5_ioaccel_mode1_completed(struct ctlr_info *h, u8 q)
 		 * but with current driver design this is easiest.
 		 */
 		wmb();
-		writel((q << 24) | rq->current_entry, h->vaddr +
+		writel_relaxed((q << 24) | rq->current_entry, h->vaddr +
 				IOACCEL_MODE1_CONSUMER_INDEX);
 		atomic_dec(&h->commands_outstanding);
 	}
-- 
2.7.4

Re: [PATCH v4 1/7] scsi: hpsa: Eliminate duplicate barriers on weakly-ordered archs

[Laurence Oberman]

On Mon, 2018-03-19 at 22:50 -0400, Sinan Kaya wrote:
> Code includes wmb() followed by writel(). writel() already has a
> barrier on some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing
> the register write.
> 
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/scsi/hpsa.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
> index 018f980..c7d7e6a 100644
> --- a/drivers/scsi/hpsa.h
> +++ b/drivers/scsi/hpsa.h
> @@ -599,7 +599,7 @@ static unsigned long
> SA5_ioaccel_mode1_completed(struct ctlr_info *h, u8 q)
>  		 * but with current driver design this is easiest.
>  		 */
>  		wmb();
> -		writel((q << 24) | rq->current_entry, h->vaddr +
> +		writel_relaxed((q << 24) | rq->current_entry, h-
> >vaddr +
>  				IOACCEL_MODE1_CONSUMER_INDEX);
>  		atomic_dec(&h->commands_outstanding);
>  	}

This looks like it would work for the x86_64 and arm because of how its
defined architecture specific for the x86_64 and the arm64

I guess its up to Don and the driver folks and if its worth the change.
I am generally not a fan of messing with these barrier things though.

Reviewed-by: Laurence Oberman <loberman@redhat.com>

[PATCH v4 2/7] scsi: megaraid: eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Create a new wrapper function with relaxed write operator. Use the new
wrapper when a write is following a barrier().

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/megaraid/megaraid_mbox.c       | 8 ++++----
 drivers/scsi/megaraid/megaraid_mbox.h       | 2 ++
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
index 530358c..8c4fc6e 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -1439,7 +1439,7 @@ mbox_post_cmd(adapter_t *adapter, scb_t *scb)
 	mbox->ack	= 0;
 	wmb();
 
-	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
+	WRINDOOR_RELAXED(raid_dev, raid_dev->mbox_dma | 0x1);
 
 	spin_unlock_irqrestore(MAILBOX_LOCK(raid_dev), flags);
 
@@ -2752,7 +2752,7 @@ mbox_post_sync_cmd(adapter_t *adapter, uint8_t raw_mbox[])
 	mbox->status		= 0xFF;
 
 	wmb();
-	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
+	WRINDOOR_RELAXED(raid_dev, raid_dev->mbox_dma | 0x1);
 
 	// wait for maximum 1 second for status to post. If the status is not
 	// available within 1 second, assume FW is initializing and wait
@@ -2877,7 +2877,7 @@ mbox_post_sync_cmd_fast(adapter_t *adapter, uint8_t raw_mbox[])
 	mbox->status		= 0xFF;
 
 	wmb();
-	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
+	WRINDOOR_RELAXED(raid_dev, raid_dev->mbox_dma | 0x1);
 
 	for (i = 0; i < MBOX_SYNC_WAIT_CNT; i++) {
 		if (mbox->numstatus != 0xFF) break;
@@ -3329,7 +3329,7 @@ megaraid_mbox_fire_sync_cmd(adapter_t *adapter)
 	mbox->status		= 0;
 
 	wmb();
-	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
+	WRINDOOR_RELAXED(raid_dev, raid_dev->mbox_dma | 0x1);
 
 	/* Wait for maximum 1 min for status to post.
 	 * If the Firmware SUPPORTS the ABOVE COMMAND,
diff --git a/drivers/scsi/megaraid/megaraid_mbox.h b/drivers/scsi/megaraid/megaraid_mbox.h
index c1d86d9..641cbd4 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.h
+++ b/drivers/scsi/megaraid/megaraid_mbox.h
@@ -230,6 +230,8 @@ typedef struct {
 
 #define RDINDOOR(rdev)		readl((rdev)->baseaddr + 0x20)
 #define RDOUTDOOR(rdev)		readl((rdev)->baseaddr + 0x2C)
+#define WRINDOOR_RELAXED(rdev, value)	\
+	writel_relaxed(value, (rdev)->baseaddr + 0x20)
 #define WRINDOOR(rdev, value)	writel(value, (rdev)->baseaddr + 0x20)
 #define WROUTDOOR(rdev, value)	writel(value, (rdev)->baseaddr + 0x2C)
 
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 073ced0..f560496 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -3479,11 +3479,11 @@ complete_cmd_fusion(struct megasas_instance *instance, u32 MSIxIndex)
 
 	wmb();
 	if (instance->msix_combined)
-		writel(((MSIxIndex & 0x7) << 24) |
+		writel_relaxed(((MSIxIndex & 0x7) << 24) |
 			fusion->last_reply_idx[MSIxIndex],
 			instance->reply_post_host_index_addr[MSIxIndex/8]);
 	else
-		writel((MSIxIndex << 24) |
+		writel_relaxed((MSIxIndex << 24) |
 			fusion->last_reply_idx[MSIxIndex],
 			instance->reply_post_host_index_addr[0]);
 	megasas_check_and_restore_queue_depth(instance);
-- 
2.7.4

[PATCH v4 3/7] scsi: dpt_i2o: eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/dpt_i2o.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index fd172b0..3c1e64a 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -1300,7 +1300,7 @@ static s32 adpt_i2o_post_this(adpt_hba* pHba, u32* data, int len)
 	wmb();
 
 	//post message
-	writel(m, pHba->post_port);
+	writel_relaxed(m, pHba->post_port);
 	wmb();
 
 	return 0;
@@ -1390,7 +1390,7 @@ static s32 adpt_i2o_reset_hba(adpt_hba* pHba)
 
 	memcpy_toio(pHba->msg_addr_virt+m, msg, sizeof(msg));
 	wmb();
-	writel(m, pHba->post_port);
+	writel_relaxed(m, pHba->post_port);
 	wmb();
 
 	while(*status == 0){
@@ -2797,7 +2797,7 @@ static s32 adpt_send_nop(adpt_hba*pHba,u32 m)
 	writel( 0,&msg[2]);
 	wmb();
 
-	writel(m, pHba->post_port);
+	writel_relaxed(m, pHba->post_port);
 	wmb();
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v4 3/7] scsi: dpt_i2o: eliminate duplicate barriers on weakly-ordered archs

[Christoph Hellwig]

This is a basically unmaintained driver for historic hardware, and not the
right place for micro-optimizations.

Re: [PATCH v4 3/7] scsi: dpt_i2o: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-20 06:23, Christoph Hellwig wrote:
> This is a basically unmaintained driver for historic hardware, and not 
> the
> right place for micro-optimizations.

Sure, I can drop this on the next version.

[PATCH v4 4/7] scsi: lpfc: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/lpfc/lpfc_sli.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 5f5528a..7dae7d3 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -1647,7 +1647,7 @@ lpfc_sli_update_full_ring(struct lpfc_hba *phba, struct lpfc_sli_ring *pring)
 	 * Set ring 'ringno' to SET R0CE_REQ in Chip Att register.
 	 * The HBA will tell us when an IOCB entry is available.
 	 */
-	writel((CA_R0ATT|CA_R0CE_REQ) << (ringno*4), phba->CAregaddr);
+	writel_relaxed((CA_R0ATT|CA_R0CE_REQ) << (ringno*4), phba->CAregaddr);
 	readl(phba->CAregaddr); /* flush */
 
 	pring->stats.iocb_cmd_full++;
@@ -1672,7 +1672,7 @@ lpfc_sli_update_ring(struct lpfc_hba *phba, struct lpfc_sli_ring *pring)
 	 */
 	if (!(phba->sli3_options & LPFC_SLI3_CRP_ENABLED)) {
 		wmb();
-		writel(CA_R0ATT << (ringno * 4), phba->CAregaddr);
+		writel_relaxed(CA_R0ATT << (ringno * 4), phba->CAregaddr);
 		readl(phba->CAregaddr); /* flush */
 	}
 }
-- 
2.7.4

[PATCH v4 5/7] scsi: ipr: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Since code already has an explicit barrier call, changing writeX() to
writeX_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/ipr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index e07dd99..209adac 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -762,9 +762,9 @@ static void ipr_mask_and_clear_interrupts(struct ipr_ioa_cfg *ioa_cfg,
 
 	/* Set interrupt mask to stop all new interrupts */
 	if (ioa_cfg->sis64)
-		writeq(~0, ioa_cfg->regs.set_interrupt_mask_reg);
+		writeq_relaxed(~0, ioa_cfg->regs.set_interrupt_mask_reg);
 	else
-		writel(~0, ioa_cfg->regs.set_interrupt_mask_reg);
+		writel_relaxed(~0, ioa_cfg->regs.set_interrupt_mask_reg);
 
 	/* Clear any pending interrupts */
 	if (ioa_cfg->sis64)
@@ -8435,7 +8435,8 @@ static int ipr_reset_enable_ioa(struct ipr_cmnd *ipr_cmd)
 	wmb();
 	if (ioa_cfg->sis64) {
 		/* Set the adapter to the correct endian mode. */
-		writel(IPR_ENDIAN_SWAP_KEY, ioa_cfg->regs.endian_swap_reg);
+		writel_relaxed(IPR_ENDIAN_SWAP_KEY,
+			       ioa_cfg->regs.endian_swap_reg);
 		int_reg = readl(ioa_cfg->regs.endian_swap_reg);
 	}
 
-- 
2.7.4

Re: [PATCH v4 5/7] scsi: ipr: Eliminate duplicate barriers on weakly-ordered archs

[Brian King]

On 03/19/2018 09:50 PM, Sinan Kaya wrote:
> Code includes barrier() followed by writel(). writel() already has a
> barrier
> on some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing the
> register write.
> 
> Since code already has an explicit barrier call, changing writeX() to
> writeX_relaxed().
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/scsi/ipr.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index e07dd99..209adac 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -762,9 +762,9 @@ static void ipr_mask_and_clear_interrupts(struct ipr_ioa_cfg *ioa_cfg,
> 
>  	/* Set interrupt mask to stop all new interrupts */
>  	if (ioa_cfg->sis64)
> -		writeq(~0, ioa_cfg->regs.set_interrupt_mask_reg);
> +		writeq_relaxed(~0, ioa_cfg->regs.set_interrupt_mask_reg);
>  	else
> -		writel(~0, ioa_cfg->regs.set_interrupt_mask_reg);
> +		writel_relaxed(~0, ioa_cfg->regs.set_interrupt_mask_reg);
> 
>  	/* Clear any pending interrupts */
>  	if (ioa_cfg->sis64)
> @@ -8435,7 +8435,8 @@ static int ipr_reset_enable_ioa(struct ipr_cmnd *ipr_cmd)
>  	wmb();
>  	if (ioa_cfg->sis64) {
>  		/* Set the adapter to the correct endian mode. */
> -		writel(IPR_ENDIAN_SWAP_KEY, ioa_cfg->regs.endian_swap_reg);
> +		writel_relaxed(IPR_ENDIAN_SWAP_KEY,
> +			       ioa_cfg->regs.endian_swap_reg);
>  		int_reg = readl(ioa_cfg->regs.endian_swap_reg);
>  	}
> 

Looks fine to me. Thanks.

Acked-by: Brian King <brking@linux.vnet.ibm.com>

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v4 6/7] scsi: bnx2i: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/bnx2i/bnx2i_hwi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 8f03a86..075735b 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -278,7 +278,7 @@ static void bnx2i_ring_sq_dbell(struct bnx2i_conn *bnx2i_conn, int count)
 		sq_db->prod_idx = ep->qp.sq_prod_idx;
 		bnx2i_ring_577xx_doorbell(bnx2i_conn);
 	} else
-		writew(count, ep->qp.ctx_base + CNIC_SEND_DOORBELL);
+		writew_relaxed(count, ep->qp.ctx_base + CNIC_SEND_DOORBELL);
 
 	mmiowb(); /* flush posted PCI writes */
 }
-- 
2.7.4

[PATCH v4 7/7] scsi: csiostor: Eliminate duplicate barriers on weakly-ordered archs

[Sinan Kaya]

Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Create a new wrapper function with relaxed write operator. Use the new
wrapper when a write is following a barrier().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/csiostor/csio_hw.h | 4 ++++
 drivers/scsi/csiostor/csio_wr.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/csiostor/csio_hw.h b/drivers/scsi/csiostor/csio_hw.h
index 30f5f52..9fd8b00 100644
--- a/drivers/scsi/csiostor/csio_hw.h
+++ b/drivers/scsi/csiostor/csio_hw.h
@@ -512,6 +512,10 @@ struct csio_hw {
 						csio_reg((_h)->regstart, (_r)))
 #define	csio_wr_reg16(_h, _v, _r)	writew((_v), \
 						csio_reg((_h)->regstart, (_r)))
+
+#define	csio_wr_reg32_relaxed(_h, _v, _r) \
+	writel_relaxed((_v), csio_reg((_h)->regstart, (_r)))
+
 #define	csio_wr_reg32(_h, _v, _r)	writel((_v), \
 						csio_reg((_h)->regstart, (_r)))
 #define	csio_wr_reg64(_h, _v, _r)	writeq((_v), \
diff --git a/drivers/scsi/csiostor/csio_wr.c b/drivers/scsi/csiostor/csio_wr.c
index c0a1778..db26222 100644
--- a/drivers/scsi/csiostor/csio_wr.c
+++ b/drivers/scsi/csiostor/csio_wr.c
@@ -984,7 +984,7 @@ csio_wr_issue(struct csio_hw *hw, int qidx, bool prio)
 
 	wmb();
 	/* Ring SGE Doorbell writing q->pidx into it */
-	csio_wr_reg32(hw, DBPRIO_V(prio) | QID_V(q->un.eq.physeqid) |
+	csio_wr_reg32_relaxed(hw, DBPRIO_V(prio) | QID_V(q->un.eq.physeqid) |
 			  PIDX_T5_V(q->inc_idx) | DBTYPE_F,
 			  MYPF_REG(SGE_PF_KDOORBELL_A));
 	q->inc_idx = 0;
-- 
2.7.4