Hi Jing,
A few more potential issues I noticed after looking through this patch.
These are not necessarily problems with this patch, but are highlighted
by the patch. Perhaps these could be addressed in a subsequent patch or
patch set?
Rob
diff --git a/drivers/scsi/bfa/bfa_ioc.h b/drivers/scsi/bfa/bfa_ioc.h
index 71c0b5a..d60e472 100644
--- a/drivers/scsi/bfa/bfa_ioc.h
+++ b/drivers/scsi/bfa/bfa_ioc.h
@@ -62,9 +62,9 @@ struct bfa_sge_s {
};
#define bfa_sge_word_swap(__sge) do { \
- ((u32 *)(__sge))[0] = bfa_os_swap32(((u32 *)(__sge))[0]); \
- ((u32 *)(__sge))[1] = bfa_os_swap32(((u32 *)(__sge))[1]); \
- ((u32 *)(__sge))[2] = bfa_os_swap32(((u32 *)(__sge))[2]); \
+ ((u32 *)(__sge))[0] = swab32(((u32 *)(__sge))[0]); \
+ ((u32 *)(__sge))[1] = swab32(((u32 *)(__sge))[1]); \
+ ((u32 *)(__sge))[2] = swab32(((u32 *)(__sge))[2]); \
} while (0)
Is bfa_sge_word_swap used anywhere? I didn't see it.
snip
+#define bfa_mem_read(_raddr, _off) swab32(readl(((_raddr) + (_off))))
#define bfa_mem_write(_raddr, _off, _val) \
- bfa_os_mem_write(_raddr, _off, _val)
+ writel(swab32((_val)), ((_raddr) + (_off)))
Do bfa_mem_read/write() belong here after your cleanup? Could you
explain these a bit if they are required?
snip
diff --git a/drivers/scsi/bfa/bfa_os_inc.h b/drivers/scsi/bfa/bfa_os_inc.h
index d928dca..f9edc75 100644
--- a/drivers/scsi/bfa/bfa_os_inc.h
+++ b/drivers/scsi/bfa/bfa_os_inc.h
@@ -65,12 +65,6 @@ do { \
((_x) & 0x00ff00) | \
(((_x) & 0xff0000) >> 16))
-#define bfa_os_swap32(_x) \
- ((((_x) & 0xff) << 24) | \
- (((_x) & 0x0000ff00) << 8) | \
- (((_x) & 0x00ff0000) >> 8) | \
- (((_x) & 0xff000000) >> 24))
-
#define bfa_os_swap_sgaddr(_x) ((u64)( \
(((u64)(_x) & (u64)0x00000000000000ffull) << 32) | \
(((u64)(_x) & (u64)0x000000000000ff00ull) << 32) | \
@@ -82,7 +76,7 @@ do { \
(((u64)(_x) & (u64)0xff00000000000000ull) >> 32)))
The sg byte swapping code above lead me to look at the sg code in the
bfa driver and I saw that bfa doesn't use scsi_for_each_sg() from what I
can tell.
Is scsi_for_each_sg() use pretty standard for mass storage drivers these
days?
Is the byte swapping required if the more traditional sg implementation
is used?
#ifndef __BIGENDIAN
-#define bfa_os_hton3b(_x) bfa_swap_3b(_x)
+#define bfa_os_hton3b(_x) bfa_swap_3b(_x)
Should bfa_os_hton3b and related functionality be relocated
appropriately and implemented in common form to the other common byte
swapping code?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
