Thread (12 messages) 12 messages, 4 authors, 2012-03-12

RE: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.

From: Rajesh Borundia <hidden>
Date: 2012-03-12 06:20:01
Also in: kernel-janitors, lkml 

-----Original Message-----
From: santosh prasad nayak [mailto:santoshprasadnayak@gmail.com]
Sent: Sunday, March 11, 2012 2:47 PM
To: Rajesh Borundia
Cc: Sony Chacko; netdev; linux-kernel; kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.

Thanks Rajesh for clarification.
Included all your inputs in the following patch.
This is for review not a formal one. Once review is done I will send a
formal patch.

Looks fine to me.

quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
b/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
index 2eeac32..b5de8a7 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
@@ -954,7 +954,7 @@ typedef struct nx_mac_list_s {

 struct nx_vlan_ip_list {
 	struct list_head list;
-	u32 ip_addr;
+	__be32 ip_addr;
 };

 /*
@@ -1780,7 +1780,7 @@ int netxen_process_rcv_ring(struct
nx_host_sds_ring *sds_ring, int max);
 void netxen_p3_free_mac_list(struct netxen_adapter *adapter);
 int netxen_config_intr_coalesce(struct netxen_adapter *adapter);
 int netxen_config_rss(struct netxen_adapter *adapter, int enable);
-int netxen_config_ipaddr(struct netxen_adapter *adapter, u32 ip, int
cmd);
+int netxen_config_ipaddr(struct netxen_adapter *adapter, __be32 ip,
int cmd);
 int netxen_linkevent_request(struct netxen_adapter *adapter, int
enable);
 void netxen_advert_link_change(struct netxen_adapter *adapter, int
linkup);
 void netxen_pci_camqm_read_2M(struct netxen_adapter *, u64, u64 *);
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
index 6f37470..59d5ee7 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
@@ -909,7 +909,7 @@ int netxen_config_rss(struct netxen_adapter
*adapter, int enable)
 	return rv;
 }

-int netxen_config_ipaddr(struct netxen_adapter *adapter, u32 ip, int
cmd)
+int netxen_config_ipaddr(struct netxen_adapter *adapter, __be32 ip,
int cmd)
 {
 	nx_nic_req_t req;
 	u64 word;
@@ -922,7 +922,7 @@ int netxen_config_ipaddr(struct netxen_adapter
*adapter, u32 ip, int cmd)
 	req.req_hdr = cpu_to_le64(word);

 	req.words[0] = cpu_to_le64(cmd);
-	req.words[1] = cpu_to_le64(ip);
+	memcpy(&req.words[1], &ip, sizeof(u32));

 	rv = netxen_send_cmd_descs(adapter, (struct cmd_desc_type0
*)&req, 1);
 	if (rv != 0) {
@@ -1050,7 +1050,7 @@ int netxen_get_flash_mac_addr(struct
netxen_adapter *adapter, u64 *mac)
 	if (netxen_get_flash_block(adapter, offset, sizeof(u64), pmac) ==
-1)
 		return -1;

-	if (*mac == cpu_to_le64(~0ULL)) {
+	if (*(__le64 *)mac == cpu_to_le64(~0ULL)) {

 		offset = NX_OLD_MAC_ADDR_OFFSET +
 			(adapter->portnum * sizeof(u64));
@@ -1059,7 +1059,7 @@ int netxen_get_flash_mac_addr(struct
netxen_adapter *adapter, u64 *mac)
 					offset, sizeof(u64), pmac) == -1)
 			return -1;

-		if (*mac == cpu_to_le64(~0ULL))
+		if (*(__le64 *)mac == cpu_to_le64(~0ULL))
 			return -1;
 	}
 	return 0;
@@ -2155,7 +2155,7 @@ static u32 netxen_md_rd_crb(struct
netxen_adapter *adapter,
 static u32
 netxen_md_rdrom(struct netxen_adapter *adapter,
 			struct netxen_minidump_entry_rdrom
-				*romEntry, u32 *data_buff)
+				*romEntry, __le32 *data_buff)
 {
 	int i, count = 0;
 	u32 size, lck_val;
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 7648995..65a718f 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -805,12 +805,12 @@ netxen_check_options(struct netxen_adapter
*adapter)
 	char brd_name[NETXEN_MAX_SHORT_NAME];
 	char serial_num[32];
 	int i, offset, val, err;
-	int *ptr32;
+	__le32 *ptr32;
 	struct pci_dev *pdev = adapter->pdev;

 	adapter->driver_mismatch = 0;

-	ptr32 = (int *)&serial_num;
+	ptr32 = (__le32 *)&serial_num;
 	offset = NX_FW_SERIAL_NUM_OFFSET;
 	for (i = 0; i < 8; i++) {
 		if (netxen_rom_fast_read(adapter, offset, &val) == -1) {



On Sun, Mar 11, 2012 at 12:31 AM, Rajesh Borundia
[off-list ref] wrote:
quoted
quoted
-----Original Message-----
From: santosh prasad nayak [mailto:santoshprasadnayak@gmail.com]
Sent: Saturday, March 10, 2012 12:20 AM
To: Rajesh Borundia
Cc: Sony Chacko; netdev; linux-kernel; kernel-
janitors@vger.kernel.org
quoted
quoted
Subject: Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.

On Fri, Mar 9, 2012 at 10:04 PM, Rajesh Borundia
[off-list ref] wrote:
quoted
quoted
-----Original Message-----
From: santosh nayak [mailto:santoshprasadnayak@gmail.com]
Sent: Saturday, March 03, 2012 9:18 PM
To: Sony Chacko
Cc: Rajesh Borundia; netdev; linux-kernel; kernel-
janitors@vger.kernel.org; Santosh Nayak
Subject: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.

From: Santosh Nayak <redacted>

Fix endian bug.

Signed-off-by: Santosh Nayak <redacted>
---
 drivers/net/ethernet/qlogic/netxen/netxen_nic.h    |    4 ++--
 drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c |   12
+++++++--
quoted
quoted
---
quoted
quoted
 .../net/ethernet/qlogic/netxen/netxen_nic_main.c   |    2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
b/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
index 2eeac32..b5de8a7 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic.h
@@ -954,7 +954,7 @@ typedef struct nx_mac_list_s {

 struct nx_vlan_ip_list {
      struct list_head list;
-     u32 ip_addr;
+     __be32 ip_addr;
 };

 /*
@@ -1780,7 +1780,7 @@ int netxen_process_rcv_ring(struct
nx_host_sds_ring *sds_ring, int max);
 void netxen_p3_free_mac_list(struct netxen_adapter *adapter);
 int netxen_config_intr_coalesce(struct netxen_adapter *adapter);
 int netxen_config_rss(struct netxen_adapter *adapter, int
enable);
quoted
quoted
quoted
quoted
-int netxen_config_ipaddr(struct netxen_adapter *adapter, u32 ip,
int
quoted
quoted
cmd);
+int netxen_config_ipaddr(struct netxen_adapter *adapter, __be32
ip,
quoted
quoted
quoted
quoted
int cmd);
 int netxen_linkevent_request(struct netxen_adapter *adapter, int
enable);
 void netxen_advert_link_change(struct netxen_adapter *adapter,
int
quoted
quoted
quoted
quoted
linkup);
 void netxen_pci_camqm_read_2M(struct netxen_adapter *, u64, u64
*);
quoted
quoted
quoted
quoted
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
index 6f37470..0f81287 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
@@ -909,10 +909,11 @@ int netxen_config_rss(struct netxen_adapter
*adapter, int enable)
      return rv;
 }

-int netxen_config_ipaddr(struct netxen_adapter *adapter, u32 ip,
int
quoted
quoted
cmd)
+int netxen_config_ipaddr(struct netxen_adapter *adapter, __be32
ip,
quoted
quoted
quoted
quoted
int cmd)
 {
      nx_nic_req_t req;
      u64 word;
+     u64 ip_addr;
      int rv;

      memset(&req, 0, sizeof(nx_nic_req_t));
@@ -922,7 +923,8 @@ int netxen_config_ipaddr(struct
netxen_adapter
quoted
quoted
quoted
quoted
*adapter, u32 ip, int cmd)
      req.req_hdr = cpu_to_le64(word);

      req.words[0] = cpu_to_le64(cmd);
-     req.words[1] = cpu_to_le64(ip);
+     ip_addr = be32_to_cpu(ip);
+     *(__be64 *)&req.words[1] = cpu_to_be64(ip_addr);
quoted
Adapter requires ip value in big endian stored at lower 32 bit
address.
quoted
The cpu_to_be64 will swap the lower 32 bit with higher 32 bit and
adapter
quoted
Will get incorrect ip addr. Instead u can do.

      U32 *ip_addr;
      ip_addr = (u32 *)&req.words[1];
      *ip_addr = ip;

1.  It looks incomplete.
    In the function call "netxen_send_cmd_descs" we have to pass
"&req"
quoted
quoted
as
   2nd argument  not  "ipaddr".
 I should have sent a patch. This piece of code was just to show how
to
quoted
copy ip addr in  req.words[1].

quoted
2. Your above suggestion is with assumption that the data type of
2nd
quoted
quoted
argument "ip"
     in "netxen_config_ipaddr()" is still "u32".  This is not true.

     Some days back you suggested to change the data type to
"__be32".
quoted
quoted
 In the present patch
     the "ip"  is in "__be32" format i.e already in Big endian
format
quoted
quoted
as per requirement.
     We need to only convert this 32 bit to 64 bit.  There are two
ways:
 No I did not assume that ip is u32, ip is still __be32(ip value is
in form of big endian)
quoted
 though I should have mentioned it explicitly. But the ip value
should be copied to lower 32 bit of req.words[1].
quoted
quoted
     a.   *(__be64 *)&req.words[1] = ip;   // auto conversion
In big endian machine MSB is copied into MSB first. So ip will get
copied into higher 32 bit of
quoted
req.words[1] but adapter requires it in lower 32 bit.
quoted
     b.   *(__be64 *)&req.words[1] = cpu_to_be64(be32_to_cpu(ip));
            // explicit conversion.
If you follow second cpu_to_be64 it will swap lower 32 bit of ip with
higher 32 bit in little endian machine.
quoted
But adapter requires it in lower 32 bit.

Simple solution to copy ip in req.words[1] could be
memcpy(&req.words[1], &ip, sizeof(u32));
quoted
quoted
 Please correct me if I am wrong.


regards
Santosh




quoted
quoted
      rv = netxen_send_cmd_descs(adapter, (struct cmd_desc_type0
*)&req, 1);
      if (rv != 0) {
@@ -1050,7 +1052,7 @@ int netxen_get_flash_mac_addr(struct
netxen_adapter *adapter, u64 *mac)
      if (netxen_get_flash_block(adapter, offset, sizeof(u64),
pmac)
quoted
quoted
==
quoted
quoted
-1)
              return -1;

-     if (*mac == cpu_to_le64(~0ULL)) {
+     if (*mac == ~0ULL) {
*mac is in little endian format so compare it with cpu_to_le64.

quoted
              offset = NX_OLD_MAC_ADDR_OFFSET +
                      (adapter->portnum * sizeof(u64));
@@ -1059,7 +1061,7 @@ int netxen_get_flash_mac_addr(struct
netxen_adapter *adapter, u64 *mac)
                                      offset, sizeof(u64), pmac)
==
quoted
quoted
-1)
quoted
quoted
                      return -1;

-             if (*mac == cpu_to_le64(~0ULL))
+             if (*mac == ~0ULL)
*mac here is in little endian format so compare it with
cpu_to_le64.
quoted
quoted
quoted
quoted
                      return -1;
      }
      return 0;
@@ -2178,7 +2180,7 @@ lock_try:
              NX_WR_DUMP_REG(FLASH_ROM_WINDOW, adapter-
ahw.pci_base0,
quoted
waddr);
              raddr = FLASH_ROM_DATA + (fl_addr & 0x0000FFFF);
              NX_RD_DUMP_REG(raddr, adapter->ahw.pci_base0,
&val);
quoted
quoted
quoted
quoted
-             *data_buff++ = cpu_to_le32(val);
+             *data_buff++ = val;
It should be cpu_to_le32 as it is returned to tool which requires
output in little endian.

quoted
              fl_addr += sizeof(val);
      }
      readl((void __iomem *)(adapter->ahw.pci_base0 +
NX_FLASH_SEM2_ULK));
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 8dc4a134..70783b4 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -818,7 +818,7 @@ netxen_check_options(struct netxen_adapter
*adapter)
                      adapter->driver_mismatch = 1;
                      return;
              }
-             ptr32[i] = cpu_to_le32(val);
+             ptr32[i] = val;
Here val should be in little endian (cpu_to_le32) as it will be
referenced by byte array to print serial number.
quoted
quoted
              offset += sizeof(u32);
      }

--
1.7.4.4

Sorry for Late reply.

Rajesh
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help