Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug.
From: santosh prasad nayak
Date: Sun Mar 11 2012 - 05:17:32 EST
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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/