diff mbox

scsi: hisi_sas: shift vs compare typos

Message ID 20161129104725.GA7623@mwanda (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Dan Carpenter Nov. 29, 2016, 10:47 a.m. UTC 
There are some typos where we intended "<<" but have "<".  Seems likely
to cause a bunch of problems.

Fixes: d3b688d3c69d ("scsi: hisi_sas: add v2 hw support for ECC and AXI bus fatal error")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
There is another static checker warning to fix perhaps:

	drivers/scsi/hisi_sas/hisi_sas_v2_hw.c:2015 phy_up_v2_hw()
	warn: was expecting a 64 bit value instead of '(2 | 1)'

The code is doing:

	phy->phy_type &= ~(PORT_TYPE_SAS | PORT_TYPE_SATA);

phy->phy_type is a u64 and PORT_TYPE_SAS is "1U << 1".  So this code
is clearing out the high 32 bits of ->phy_type uninitentionally.  It's
simple enough to make it 1ULL << 1 but the question is, do we really
need ->phy_type to be a 64 bit variable?  I'm pretty sure that a u32
will suffice.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

John Garry Nov. 29, 2016, 11:20 a.m. UTC | #1 
On 29/11/2016 10:47, Dan Carpenter wrote:
> There are some typos where we intended "<<" but have "<".  Seems likely
> to cause a bunch of problems.
>
> Fixes: d3b688d3c69d ("scsi: hisi_sas: add v2 hw support for ECC and AXI bus fatal error")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> There is another static checker warning to fix perhaps:
>
> 	drivers/scsi/hisi_sas/hisi_sas_v2_hw.c:2015 phy_up_v2_hw()
> 	warn: was expecting a 64 bit value instead of '(2 | 1)'
>
> The code is doing:
>
> 	phy->phy_type &= ~(PORT_TYPE_SAS | PORT_TYPE_SATA);

Right, I think that a u32 would suffice. We can generate a separate 
patch for this.

>
> phy->phy_type is a u64 and PORT_TYPE_SAS is "1U << 1".  So this code
> is clearing out the high 32 bits of ->phy_type uninitentionally.  It's
> simple enough to make it 1ULL << 1 but the question is, do we really
> need ->phy_type to be a 64 bit variable?  I'm pretty sure that a u32
> will suffice.
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> index 15487f2..93876c0 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> @@ -64,19 +64,19 @@
>  					 HGC_LM_DFX_STATUS2_ITCTLIST_OFF)
>  #define HGC_CQE_ECC_ADDR		0x13c
>  #define HGC_CQE_ECC_1B_ADDR_OFF	0
> -#define HGC_CQE_ECC_1B_ADDR_MSK	(0x3f < HGC_CQE_ECC_1B_ADDR_OFF)
> +#define HGC_CQE_ECC_1B_ADDR_MSK	(0x3f << HGC_CQE_ECC_1B_ADDR_OFF)
>  #define HGC_CQE_ECC_MB_ADDR_OFF	8
> -#define HGC_CQE_ECC_MB_ADDR_MSK (0x3f < HGC_CQE_ECC_MB_ADDR_OFF)
> +#define HGC_CQE_ECC_MB_ADDR_MSK (0x3f << HGC_CQE_ECC_MB_ADDR_OFF)
>  #define HGC_IOST_ECC_ADDR		0x140
>  #define HGC_IOST_ECC_1B_ADDR_OFF	0
> -#define HGC_IOST_ECC_1B_ADDR_MSK	(0x3ff < HGC_IOST_ECC_1B_ADDR_OFF)
> +#define HGC_IOST_ECC_1B_ADDR_MSK	(0x3ff << HGC_IOST_ECC_1B_ADDR_OFF)
>  #define HGC_IOST_ECC_MB_ADDR_OFF	16
> -#define HGC_IOST_ECC_MB_ADDR_MSK	(0x3ff < HGC_IOST_ECC_MB_ADDR_OFF)
> +#define HGC_IOST_ECC_MB_ADDR_MSK	(0x3ff << HGC_IOST_ECC_MB_ADDR_OFF)
>  #define HGC_DQE_ECC_ADDR		0x144
>  #define HGC_DQE_ECC_1B_ADDR_OFF	0
> -#define HGC_DQE_ECC_1B_ADDR_MSK	(0xfff < HGC_DQE_ECC_1B_ADDR_OFF)
> +#define HGC_DQE_ECC_1B_ADDR_MSK	(0xfff << HGC_DQE_ECC_1B_ADDR_OFF)
>  #define HGC_DQE_ECC_MB_ADDR_OFF	16
> -#define HGC_DQE_ECC_MB_ADDR_MSK (0xfff < HGC_DQE_ECC_MB_ADDR_OFF)
> +#define HGC_DQE_ECC_MB_ADDR_MSK (0xfff << HGC_DQE_ECC_MB_ADDR_OFF)
>  #define HGC_INVLD_DQE_INFO		0x148
>  #define HGC_INVLD_DQE_INFO_FB_CH0_OFF	9
>  #define HGC_INVLD_DQE_INFO_FB_CH0_MSK	(0x1 << HGC_INVLD_DQE_INFO_FB_CH0_OFF)
>
> .
>

Cheers

These were introduced in recent patch applied to 4.10 queue, and would 
only affect rare occurance of AXI/ECC error.

Signed-off-by: John Garry <john.garry@huawei.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin K. Petersen Nov. 29, 2016, 4:28 p.m. UTC | #2 
>>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes:

Dan> There are some typos where we intended "<<" but have "<".  Seems
Dan> likely to cause a bunch of problems.

Applied to 4.10/scsi-queue.
diff mbox

Patch

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 15487f2..93876c0 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -64,19 +64,19 @@ 
 					 HGC_LM_DFX_STATUS2_ITCTLIST_OFF)
 #define HGC_CQE_ECC_ADDR		0x13c
 #define HGC_CQE_ECC_1B_ADDR_OFF	0
-#define HGC_CQE_ECC_1B_ADDR_MSK	(0x3f < HGC_CQE_ECC_1B_ADDR_OFF)
+#define HGC_CQE_ECC_1B_ADDR_MSK	(0x3f << HGC_CQE_ECC_1B_ADDR_OFF)
 #define HGC_CQE_ECC_MB_ADDR_OFF	8
-#define HGC_CQE_ECC_MB_ADDR_MSK (0x3f < HGC_CQE_ECC_MB_ADDR_OFF)
+#define HGC_CQE_ECC_MB_ADDR_MSK (0x3f << HGC_CQE_ECC_MB_ADDR_OFF)
 #define HGC_IOST_ECC_ADDR		0x140
 #define HGC_IOST_ECC_1B_ADDR_OFF	0
-#define HGC_IOST_ECC_1B_ADDR_MSK	(0x3ff < HGC_IOST_ECC_1B_ADDR_OFF)
+#define HGC_IOST_ECC_1B_ADDR_MSK	(0x3ff << HGC_IOST_ECC_1B_ADDR_OFF)
 #define HGC_IOST_ECC_MB_ADDR_OFF	16
-#define HGC_IOST_ECC_MB_ADDR_MSK	(0x3ff < HGC_IOST_ECC_MB_ADDR_OFF)
+#define HGC_IOST_ECC_MB_ADDR_MSK	(0x3ff << HGC_IOST_ECC_MB_ADDR_OFF)
 #define HGC_DQE_ECC_ADDR		0x144
 #define HGC_DQE_ECC_1B_ADDR_OFF	0
-#define HGC_DQE_ECC_1B_ADDR_MSK	(0xfff < HGC_DQE_ECC_1B_ADDR_OFF)
+#define HGC_DQE_ECC_1B_ADDR_MSK	(0xfff << HGC_DQE_ECC_1B_ADDR_OFF)
 #define HGC_DQE_ECC_MB_ADDR_OFF	16
-#define HGC_DQE_ECC_MB_ADDR_MSK (0xfff < HGC_DQE_ECC_MB_ADDR_OFF)
+#define HGC_DQE_ECC_MB_ADDR_MSK (0xfff << HGC_DQE_ECC_MB_ADDR_OFF)
 #define HGC_INVLD_DQE_INFO		0x148
 #define HGC_INVLD_DQE_INFO_FB_CH0_OFF	9
 #define HGC_INVLD_DQE_INFO_FB_CH0_MSK	(0x1 << HGC_INVLD_DQE_INFO_FB_CH0_OFF)