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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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.



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
<rajesh.borundia@xxxxxxxxxx> wrote:
>
>
>> -----Original Message-----
>> From: santosh prasad nayak [mailto:santoshprasadnayak@xxxxxxxxx]
>> Sent: Saturday, March 10, 2012 12:20 AM
>> To: Rajesh Borundia
>> Cc: Sony Chacko; netdev; linux-kernel; kernel-janitors@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.
>>
>> On Fri, Mar 9, 2012 at 10:04 PM, Rajesh Borundia
>> <rajesh.borundia@xxxxxxxxxx> wrote:
>> >
>> >> -----Original Message-----
>> >> From: santosh nayak [mailto:santoshprasadnayak@xxxxxxxxx]
>> >> Sent: Saturday, March 03, 2012 9:18 PM
>> >> To: Sony Chacko
>> >> Cc: Rajesh Borundia; netdev; linux-kernel; kernel-
>> >> janitors@xxxxxxxxxxxxxxx; Santosh Nayak
>> >> Subject: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.
>> >>
>> >> From: Santosh Nayak <santoshprasadnayak@xxxxxxxxx>
>> >>
>> >> Fix endian bug.
>> >>
>> >> Signed-off-by: Santosh Nayak <santoshprasadnayak@xxxxxxxxx>
>> >> ---
>> >>  drivers/net/ethernet/qlogic/netxen/netxen_nic.h    |    4 ++--
>> >>  drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c |   12 +++++++--
>> ---
>> >>  .../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);
>> >> -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..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
>> >> cmd)
>> >> +int netxen_config_ipaddr(struct netxen_adapter *adapter, __be32 ip,
>> >> 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
>> >> *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);
>> >
>>
>>
>> > Adapter requires ip value in big endian stored at lower 32 bit
>> address.
>> > The cpu_to_be64 will swap the lower 32 bit with higher 32 bit and
>> adapter
>> > 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"
>> as
>>    2nd argument  not  "ipaddr".
>
>  I should have sent a patch. This piece of code was just to show how to
> copy ip addr in  req.words[1].
>
>>
>> 2. Your above suggestion is with assumption that the data type of 2nd
>> 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".
>>  In the present patch
>>      the "ip"  is in "__be32" format i.e already in Big endian format
>> 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)
>  though I should have mentioned it explicitly. But the ip value should be copied to lower 32 bit of req.words[1].
>
>
>>      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
> req.words[1] but adapter requires it in lower 32 bit.
>>
>>      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.
> 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));
>
>
>>
>>  Please correct me if I am wrong.
>>
>>
>> regards
>> Santosh
>>
>>
>>
>>
>> >
>> >
>> >>
>> >>       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)
>> ==
>> >> -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.
>> >
>> >>
>> >>               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) ==
>> -1)
>> >>                       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.
>> >
>> >>                       return -1;
>> >>       }
>> >>       return 0;
>> >> @@ -2178,7 +2180,7 @@ lock_try:
>> >>               NX_WR_DUMP_REG(FLASH_ROM_WINDOW, adapter-
>> >ahw.pci_base0,
>> >> waddr);
>> >>               raddr = FLASH_ROM_DATA + (fl_addr & 0x0000FFFF);
>> >>               NX_RD_DUMP_REG(raddr, adapter->ahw.pci_base0, &val);
>> >> -             *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.
>> >
>> >>               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.
>> >
>> >>               offset += sizeof(u32);
>> >>       }
>> >>
>> >> --
>> >> 1.7.4.4
>> >>
>> >
>> >
>> > Sorry for Late reply.
>> >
>> > Rajesh
>> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux Kernel Discussion]     [Ethernet Bridging]     [Linux Wireless Networking]     [Linux Bluetooth Networking]     [Linux Networking Users]     [VLAN]     [Git]     [IETF Annouce]     [Linux Assembly]     [Security]     [Bugtraq]     [Photo]     [Singles Social Networking]     [Yosemite Information]     [MIPS Linux]     [ARM Linux Kernel]     [ARM Linux]     [Linux Virtualization]     [Linux Security]     [Linux IDE]     [Linux RAID]     [Linux SCSI]     [Free Dating]

Add to Google Powered by Linux