From patchwork Sun Mar 11 09:16:48 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: santosh nayak X-Patchwork-Id: 145968 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3F745B6FA3 for ; Sun, 11 Mar 2012 20:17:58 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752001Ab2CKJQv (ORCPT ); Sun, 11 Mar 2012 05:16:51 -0400 Received: from mail-pz0-f46.google.com ([209.85.210.46]:38294 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751172Ab2CKJQt convert rfc822-to-8bit (ORCPT ); Sun, 11 Mar 2012 05:16:49 -0400 Received: by dajr28 with SMTP id r28so3621230daj.19 for ; Sun, 11 Mar 2012 01:16:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=6SuyZ8CaFC/M1x7o7rcIq2bblwekZD9mmBMPElHaKoY=; b=Wxt6TYEyBlKebMk6/ME3z+P8DW+z4K/OoQ36xcSWAFq9dwBobl+6NsgvuNYhWLeWk8 WFR2KJktCIEws2pEE0T31NEurEuArWI3XIvZUqBih2H4zWAM8iwMjhn6LSxn0voSXShX KDUEhT6eBed3adgCzULw+dcV3JLKsSXReVG9fVaI3VL2ef2OG+c/BrcofPB48IbCvQwj wF1ZCRn2fXqPbDQe30ZXuM64WhzUVAS1fuq+xzNK/GvC8kn896cY2hd3Dwlwehp3luPP 6mVbewydRTXeWxsglrWKBBPfqUrxqaUJKdzz4wvGgHG1QIMVof6GwGPhbe58+/DNFHW5 QShQ== MIME-Version: 1.0 Received: by 10.68.202.135 with SMTP id ki7mr12715168pbc.158.1331457408681; Sun, 11 Mar 2012 01:16:48 -0800 (PST) Received: by 10.68.217.35 with HTTP; Sun, 11 Mar 2012 01:16:48 -0800 (PST) In-Reply-To: <13A253B3F9BEFE43B93C09CF75F63CAA81A8AB3C07@MNEXMB1.qlogic.org> References: <1330789663-30481-1-git-send-email-santoshprasadnayak@gmail.com> <13A253B3F9BEFE43B93C09CF75F63CAA81A8AB3B75@MNEXMB1.qlogic.org> <13A253B3F9BEFE43B93C09CF75F63CAA81A8AB3C07@MNEXMB1.qlogic.org> Date: Sun, 11 Mar 2012 14:46:48 +0530 Message-ID: Subject: Re: [PATCH 3/3] netxen: qlogic ethernet : Fix Endian Bug. From: santosh prasad nayak To: Rajesh Borundia Cc: Sony Chacko , netdev , linux-kernel , "kernel-janitors@vger.kernel.org" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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. On Sun, Mar 11, 2012 at 12:31 AM, Rajesh Borundia 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 >> 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 >> >> >> >> Fix endian bug. >> >> >> >> Signed-off-by: Santosh Nayak >> >> --- >> >>  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 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) {