| Message ID | 1331534303-4093-1-git-send-email-santoshprasadnayak@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: santosh nayak <santoshprasadnayak@gmail.com> Date: Mon, 12 Mar 2012 12:08:23 +0530 > From: Santosh Nayak <santoshprasadnayak@gmail.com> > > Fix endian bug. > > Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> This is a very non-trivial patch, therefore the terse commit message and the lack of any comments in this code is unacceptable. This commit message is not only terse, it's wrong. You're not fixing an endian bug, you're fixing several of them. Explain everything in your commit message. > req.words[0] = cpu_to_le64(cmd); > - req.words[1] = cpu_to_le64(ip); > + memcpy(&req.words[1], &ip, sizeof(u32)); Nobody is going to read that and have any idea why it's correct unless they happen to be able to discover the long thread you guys had this past week working out how to do this change correctly. Putting a comment here for the rest of us mere mortals is therefore absolutely required. -- 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
Sure David. I will resend it with proper comment. Regards Santosh On Mon, Mar 12, 2012 at 12:18 PM, David Miller <davem@davemloft.net> wrote: > From: santosh nayak <santoshprasadnayak@gmail.com> > Date: Mon, 12 Mar 2012 12:08:23 +0530 > >> From: Santosh Nayak <santoshprasadnayak@gmail.com> >> >> Fix endian bug. >> >> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> > > This is a very non-trivial patch, therefore the terse commit > message and the lack of any comments in this code is unacceptable. > > This commit message is not only terse, it's wrong. You're not > fixing an endian bug, you're fixing several of them. > > Explain everything in your commit message. > >> req.words[0] = cpu_to_le64(cmd); >> - req.words[1] = cpu_to_le64(ip); >> + memcpy(&req.words[1], &ip, sizeof(u32)); > > Nobody is going to read that and have any idea why it's correct unless > they happen to be able to discover the long thread you guys had this > past week working out how to do this change correctly. > > Putting a comment here for the rest of us mere mortals is therefore > absolutely required. -- 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
Subject and comments are provided below for review. This is for review not a formal one. Please let me know if any more changes are required so that I can include it in a formal patch. -------------------------------------------------------------------------------------------------------- Subject: [PATCH 3/3 v2] netxen: qlogic ethernet : Fix endian bug. Change the datatype of "ip_addr" to __be32 as 'ip' should be in big endian format. Adapter needs "ip address" in big endian format stored at lower 32bit of req.word[1]. netxen_config_ipaddr() now receives 'ip' in big endian format. To satisfy adapter's need, use memcpy() to copy byte by byte of 'ip' into lower 32bit of req.word[1]. Mac address and serial number of adapter need to be in little endian format. Change the data type of the related variables to __le32 / __le64 or cast it explicitly to __le32 / __le64 depending upon the requirement. --------------------------------------------------------------------------------------------------------------- Regards Santosh On Mon, Mar 12, 2012 at 12:22 PM, santosh prasad nayak <santoshprasadnayak@gmail.com> wrote: > Sure David. > I will resend it with proper comment. > > Regards > Santosh > > On Mon, Mar 12, 2012 at 12:18 PM, David Miller <davem@davemloft.net> wrote: >> From: santosh nayak <santoshprasadnayak@gmail.com> >> Date: Mon, 12 Mar 2012 12:08:23 +0530 >> >>> From: Santosh Nayak <santoshprasadnayak@gmail.com> >>> >>> Fix endian bug. >>> >>> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> >> >> This is a very non-trivial patch, therefore the terse commit >> message and the lack of any comments in this code is unacceptable. >> >> This commit message is not only terse, it's wrong. You're not >> fixing an endian bug, you're fixing several of them. >> >> Explain everything in your commit message. >> >>> req.words[0] = cpu_to_le64(cmd); >>> - req.words[1] = cpu_to_le64(ip); >>> + memcpy(&req.words[1], &ip, sizeof(u32)); >> >> Nobody is going to read that and have any idea why it's correct unless >> they happen to be able to discover the long thread you guys had this >> past week working out how to do this change correctly. >> >> Putting a comment here for the rest of us mere mortals is therefore >> absolutely required. -- 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
From: santosh prasad nayak <santoshprasadnayak@gmail.com> Date: Mon, 12 Mar 2012 14:45:38 +0530 > Subject: [PATCH 3/3 v2] netxen: qlogic ethernet : Fix endian bug. > > Change the datatype of "ip_addr" to __be32 as 'ip' should be in > big endian format. > > Adapter needs "ip address" in big endian format stored at lower 32bit > of req.word[1]. netxen_config_ipaddr() now receives 'ip' in big endian > format. To satisfy adapter's need, use memcpy() to copy byte by byte > of 'ip' into lower 32bit of req.word[1]. > > Mac address and serial number of adapter need to be in little endian format. > Change the data type of the related variables to __le32 / __le64 or cast it > explicitly to __le32 / __le64 depending upon the requirement. This looks a lot better. -- 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) {