Re: mvsas regression since 3.5

2012-12-22 Thread Xi Wang 
On 12/22/12 10:33 AM, Drunkard Zhang wrote:
 I'm using Asus PIKE 6480 SAS card, whose chipset is RAID bus
 controller: Marvell Technology Group Ltd. MV64460/64461/64462 System
 Controller, Revision B, with latest stable branch 3.7.1 only 1 of 8
 ports works, to get others works I got to pull  plug back. While with
 3.5.7 it's all good, so it must be a regression.

Can you try to revert commit beecadea1b8d67f591b13f7099559f32f3fd601d?

Particularly, can you first change

  #define bit(n) ((u64)1  n)

in drivers/scsi/mvsas/mv_sas.h back to

  #define bit(n) ((u32)1  n)

and see if it works for you?  Thanks.

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

[PATCH v2] [SCSI] mvsas: fix undefined bit shift

2012-11-16 Thread Xi Wang 
The macro bit(n) is defined as ((u32)1  n), and thus it doesn't work
with n = 32, such as in mvs_94xx_assign_reg_set():

if (i = 32) {
mvi-sata_reg_set |= bit(i);
...
}

The shift ((u32)1  n) with n = 32 also leads to undefined behavior.
The result varies depending on the architecture.

This patch changes bit(n) to do a 64-bit shift.  It also simplifies
mv_ffc64() using __ffs64(), since invoking ffz() with ~0 is undefined.

Signed-off-by: Xi Wang xi.w...@gmail.com
Cc: Xiangliang Yu yuxia...@marvell.com
Cc: James Bottomley jbottom...@parallels.com
Cc: sta...@vger.kernel.org
---
As suggested by James, v2 is a single patch with a stable tag.
---
 drivers/scsi/mvsas/mv_94xx.h |   14 ++
 drivers/scsi/mvsas/mv_sas.h  |2 +-
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.h b/drivers/scsi/mvsas/mv_94xx.h
index 8f7eb4f..487aa6f 100644
--- a/drivers/scsi/mvsas/mv_94xx.h
+++ b/drivers/scsi/mvsas/mv_94xx.h
@@ -258,21 +258,11 @@ enum sas_sata_phy_regs {
 #define SPI_ADDR_VLD_94XX  (1U  1)
 #define SPI_CTRL_SpiStart_94XX (1U  0)
 
-#define mv_ffc(x)   ffz(x)
-
 static inline int
 mv_ffc64(u64 v)
 {
-   int i;
-   i = mv_ffc((u32)v);
-   if (i = 0)
-   return i;
-   i = mv_ffc((u32)(v32));
-
-   if (i != 0)
-   return 32 + i;
-
-   return -1;
+   u64 x = ~v;
+   return x ? __ffs64(x) : -1;
 }
 
 #define r_reg_set_enable(i) \
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index c04a4f5..da24955 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -69,7 +69,7 @@ extern struct kmem_cache *mvs_task_list_cache;
 #define DEV_IS_EXPANDER(type)  \
((type == EDGE_DEV) || (type == FANOUT_DEV))
 
-#define bit(n) ((u32)1  n)
+#define bit(n) ((u64)1  n)
 
 #define for_each_phy(__lseq_mask, __mc, __lseq)\
for ((__mc) = (__lseq_mask), (__lseq) = 0;  \
-- 
1.7.10.4

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

Re: [PATCH 2/3] [SCSI] mvsas: fix shift in mvs_94xx_free_reg_set()

2012-11-09 Thread Xi Wang 

On 11/9/12 2:30 AM, Xiangliang Yu wrote:

Agree with James, and just need to do NOT operation one time


Thanks for reviewing the patches.

Okay I'll remove patch 2 in v2 then.


About patch 3, I check the ffz code and found it will check ~0 conditions.


Can you point me to the ~0 check in ffz code?  I also feel like using
__ffs64 makes the code simpler.

Does patch 1 look good to you?  Thanks.

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

Re: [PATCH 2/3] [SCSI] mvsas: fix shift in mvs_94xx_free_reg_set()

2012-11-06 Thread Xi Wang 

On 11/6/12 7:06 AM, James Bottomley wrote:


Why is this necessary?  As I read the reg set assignment code, it finds
a free bit in the 64 bit register and uses that ... which can never be
greater than 64 so there's no need for the check.


This patch just tries to be more defensive for bit(reg_set) with a
broken reg_set value.  I agree with you that it's not that necessary.


The other two look OK (probably redone as a single patch with a stable
tag), but I'd like the input of the mvs people since it seems with the
current code, we only use 32 bit regsets and probably hang if we go over
that.  The bug fix is either to enable the full 64 if it works, or
possibly cap at 32 ... what works with all released devices?


Thanks for reviewing.  Yeah we'd better to wait for the input from
the mvs people.

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

[PATCH 0/3] [SCSI] mvsas: fix multiple shift issues

2012-11-05 Thread Xi Wang 
The main issue is that bit(n) is defined as:

(u32)1  n

Thus bit(n) with n = 32 will produce 0 or 1, depending on the
architecture.  This is also undefined behavior in C.

The OR with sata_reg_set (u64) then doesn't work because bit()
does a 32-bit shift, which should have been a 64-bit shift:

if (i = 32) {
mvi-sata_reg_set |= bit(i);
...
}

The other two patches fix similar oversized shift issues.

Xi Wang (3):
  [SCSI] mvsas: fix shift in mvs_94xx_assign_reg_set()
  [SCSI] mvsas: fix shift in mvs_94xx_free_reg_set()
  [SCSI] mvsas: fix shift in mv_ffc64()

 drivers/scsi/mvsas/mv_94xx.c |8 +---
 drivers/scsi/mvsas/mv_94xx.h |   14 ++
 drivers/scsi/mvsas/mv_sas.h  |2 +-
 3 files changed, 8 insertions(+), 16 deletions(-)

-- 
1.7.10.4

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

[PATCH 1/3] [SCSI] mvsas: fix shift in mvs_94xx_assign_reg_set()

2012-11-05 Thread Xi Wang 
The macro bit(n) is defined as ((u32)1  n), and thus it doesn't
work with n = 32, such as in mvs_94xx_assign_reg_set():

if (i = 32) {
mvi-sata_reg_set |= bit(i);
...
}

The shift ((u32)1  n) with n = 32 also leads to undefined behavior.
The result varies depending on the architecture.

This patch changes bit(n) to do a 64-bit shift.

Signed-off-by: Xi Wang xi.w...@gmail.com
---
 drivers/scsi/mvsas/mv_sas.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index c04a4f5..da24955 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -69,7 +69,7 @@ extern struct kmem_cache *mvs_task_list_cache;
 #define DEV_IS_EXPANDER(type)  \
((type == EDGE_DEV) || (type == FANOUT_DEV))
 
-#define bit(n) ((u32)1  n)
+#define bit(n) ((u64)1  n)
 
 #define for_each_phy(__lseq_mask, __mc, __lseq)\
for ((__mc) = (__lseq_mask), (__lseq) = 0;  \
-- 
1.7.10.4

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

[PATCH 2/3] [SCSI] mvsas: fix shift in mvs_94xx_free_reg_set()

2012-11-05 Thread Xi Wang 
Invoking bit(n) with n = 64 is undefined behavior, since bit(n) does
a 64-bit shift.  This patch adds a check on the shifting amount.

Signed-off-by: Xi Wang xi.w...@gmail.com
---
 drivers/scsi/mvsas/mv_94xx.c |8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.c b/drivers/scsi/mvsas/mv_94xx.c
index 7e423e5..e1f35d4 100644
--- a/drivers/scsi/mvsas/mv_94xx.c
+++ b/drivers/scsi/mvsas/mv_94xx.c
@@ -715,11 +715,13 @@ static void mvs_94xx_free_reg_set(struct mvs_info *mvi, 
u8 *tfs)
if (*tfs == MVS_ID_NOT_MAPPED)
return;
 
-   mvi-sata_reg_set = ~bit(reg_set);
-   if (reg_set  32)
+   if (reg_set  32) {
+   mvi-sata_reg_set = ~bit(reg_set);
w_reg_set_enable(reg_set, (u32)mvi-sata_reg_set);
-   else
+   } else if (reg_set  64) {
+   mvi-sata_reg_set = ~bit(reg_set);
w_reg_set_enable(reg_set, (u32)(mvi-sata_reg_set  32));
+   }
 
*tfs = MVS_ID_NOT_MAPPED;
 
-- 
1.7.10.4

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

[PATCH 3/3] [SCSI] mvsas: fix shift in mv_ffc64()

2012-11-05 Thread Xi Wang 
Invoking ffz(x) with x = ~0 is undefined.  This patch returns -1
for this case, and invokes __ffs64() instead.

Signed-off-by: Xi Wang xi.w...@gmail.com
---
 drivers/scsi/mvsas/mv_94xx.h |   14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.h b/drivers/scsi/mvsas/mv_94xx.h
index 8f7eb4f..487aa6f 100644
--- a/drivers/scsi/mvsas/mv_94xx.h
+++ b/drivers/scsi/mvsas/mv_94xx.h
@@ -258,21 +258,11 @@ enum sas_sata_phy_regs {
 #define SPI_ADDR_VLD_94XX  (1U  1)
 #define SPI_CTRL_SpiStart_94XX (1U  0)
 
-#define mv_ffc(x)   ffz(x)
-
 static inline int
 mv_ffc64(u64 v)
 {
-   int i;
-   i = mv_ffc((u32)v);
-   if (i = 0)
-   return i;
-   i = mv_ffc((u32)(v32));
-
-   if (i != 0)
-   return 32 + i;
-
-   return -1;
+   u64 x = ~v;
+   return x ? __ffs64(x) : -1;
 }
 
 #define r_reg_set_enable(i) \
-- 
1.7.10.4

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

[PATCH] [SCSI] bnx2fc: fix NULL checking in bnx2fc_initiate_tmf()

2012-11-04 Thread Xi Wang 
The dereference rport-data should come after the NULL check of rport.

Signed-off-by: Xi Wang xi.w...@gmail.com
---
 drivers/scsi/bnx2fc/bnx2fc_io.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 8d4626c..eebe93c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -686,7 +686,7 @@ static int bnx2fc_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 
tm_flags)
 {
struct fc_lport *lport;
struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd-device));
-   struct fc_rport_libfc_priv *rp = rport-dd_data;
+   struct fc_rport_libfc_priv *rp;
struct fcoe_port *port;
struct bnx2fc_interface *interface;
struct bnx2fc_rport *tgt;
@@ -712,6 +712,7 @@ static int bnx2fc_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 
tm_flags)
rc = FAILED;
goto tmf_err;
}
+   rp = rport-dd_data;
 
rc = fc_block_scsi_eh(sc_cmd);
if (rc)
-- 
1.7.10.4

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