| Message ID | CAOD=uF4Vx581zvpAT0dHKjUkrdsiZo-DTNR352V8tALM_P2roA@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> -----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. > > 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@qlogic.com> wrote: > > > > > >> -----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 > >> Subject: Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug. > >> > >> On Fri, Mar 9, 2012 at 10:04 PM, Rajesh Borundia > >> <rajesh.borundia@qlogic.com> wrote: > >> > > >> >> -----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 <santoshprasadnayak@gmail.com> > >> >> > >> >> Fix endian bug. > >> >> > >> >> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> > >> >> --- > >> >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: netdev-owner@vger.kernel.org > [mailto:netdev-owner@vger.kernel.org] On Behalf Of santosh > prasad nayak > Sent: 11 March 2012 09:17 > 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. I'm not sure of the exact nature of the issues here, but whenever I see code that casts between the addresses of integer types large bells start ringing - such code tends to have unwanted dependencies against the sizes and endiannesses of the relevant fields. This code might be ok, but lines like: > + if (*(__le64 *)mac == cpu_to_le64(~0ULL)) { rather give me the willies. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
here "mac" is in "u64" and I have casted it to "__le64". Because its required there. If you have any better suggestion, please let me know. regards santosh On Mon, Mar 12, 2012 at 3:07 PM, David Laight <David.Laight@aculab.com> wrote: > > >> -----Original Message----- >> From: netdev-owner@vger.kernel.org >> [mailto:netdev-owner@vger.kernel.org] On Behalf Of santosh >> prasad nayak >> Sent: 11 March 2012 09:17 >> 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. > > I'm not sure of the exact nature of the issues here, > but whenever I see code that casts between the addresses > of integer types large bells start ringing - such code > tends to have unwanted dependencies against the sizes > and endiannesses of the relevant fields. > This code might be ok, but lines like: >> + if (*(__le64 *)mac == cpu_to_le64(~0ULL)) { > rather give me the willies. > > David > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {