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

login
register
mail settings
Submitter santosh nayak
Date March 11, 2012, 9:16 a.m.
Message ID <CAOD=uF4Vx581zvpAT0dHKjUkrdsiZo-DTNR352V8tALM_P2roA@mail.gmail.com>
Download mbox | patch
Permalink /patch/145968/
State RFC
Delegated to: David Miller
Headers show

Comments

santosh nayak - March 11, 2012, 9:16 a.m.
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
<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
Rajesh Borundia - March 12, 2012, 6:19 a.m.
> -----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
David Laight - March 12, 2012, 9:37 a.m.
> -----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
santosh nayak - March 12, 2012, 9:47 a.m.
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

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) {