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

From: Rajesh Borundia
Date: Mon Mar 12 2012 - 02:19:57 EST




> -----Original Message-----
> From: santosh prasad nayak [mailto:santoshprasadnayak@xxxxxxxxx]
> Sent: Sunday, March 11, 2012 2:47 PM
> To: Rajesh Borundia
> Cc: Sony Chacko; netdev; linux-kernel; kernel-janitors@xxxxxxxxxxxxxxx
> 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@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/