* [patch] net/fec: "u32" is more explicit than "unsigned long"
@ 2013-08-23 9:49 Dan Carpenter
2013-08-23 12:44 ` Ben Hutchings
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2013-08-23 9:49 UTC (permalink / raw)
To: Grant Likely
Cc: Rob Herring, David S. Miller, Fabio Estevam, Frank Li, Jim Baxter,
Fugang Duan, netdev, devicetree, kernel-janitors
tmpaddr[] is a six byte array. We want to set the first four bytes on
the first line and the remaining two on the next line. The code assumes
that "unsigned long" is 32 bits and obviously that's not true on 64 bit
arches. It's better to just use u32 instead.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is a static checker thing and I can't compile this file.
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fdf9307..422b125 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1100,9 +1100,9 @@ static void fec_get_mac(struct net_device *ndev)
* 4) FEC mac registers set by bootloader
*/
if (!is_valid_ether_addr(iap)) {
- *((unsigned long *) &tmpaddr[0]) =
+ *((u32 *) &tmpaddr[0]) =
be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
- *((unsigned short *) &tmpaddr[4]) =
+ *((u16 *) &tmpaddr[4]) =
be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
iap = &tmpaddr[0];
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch] net/fec: "u32" is more explicit than "unsigned long"
2013-08-23 9:49 [patch] net/fec: "u32" is more explicit than "unsigned long" Dan Carpenter
@ 2013-08-23 12:44 ` Ben Hutchings
2013-08-27 18:51 ` David Miller
2013-08-29 8:25 ` [patch v2] net/fec: cleanup types in fec_get_mac() Dan Carpenter
0 siblings, 2 replies; 9+ messages in thread
From: Ben Hutchings @ 2013-08-23 12:44 UTC (permalink / raw)
To: Dan Carpenter
Cc: Grant Likely, Rob Herring, David S. Miller, Fabio Estevam,
Frank Li, Jim Baxter, Fugang Duan, netdev, devicetree,
kernel-janitors
On Fri, 2013-08-23 at 12:49 +0300, Dan Carpenter wrote:
> tmpaddr[] is a six byte array. We want to set the first four bytes on
> the first line and the remaining two on the next line. The code assumes
> that "unsigned long" is 32 bits and obviously that's not true on 64 bit
> arches. It's better to just use u32 instead.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is a static checker thing and I can't compile this file.
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index fdf9307..422b125 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1100,9 +1100,9 @@ static void fec_get_mac(struct net_device *ndev)
> * 4) FEC mac registers set by bootloader
> */
> if (!is_valid_ether_addr(iap)) {
> - *((unsigned long *) &tmpaddr[0]) =
> + *((u32 *) &tmpaddr[0]) =
> be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
> - *((unsigned short *) &tmpaddr[4]) =
> + *((u16 *) &tmpaddr[4]) =
> be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> iap = &tmpaddr[0];
> }
This code also seems to have CPU vs big-endian byte order the wrong way
round. readl() returns bytes in native order whereas we always store
MAC addresses in network (big-endian) order. So I think it should be
doing:
*((__be32 *) &tmpaddr[0]) =
cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
*((__be16 *) &tmpaddr[4]) =
cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] net/fec: "u32" is more explicit than "unsigned long"
2013-08-23 12:44 ` Ben Hutchings
@ 2013-08-27 18:51 ` David Miller
2013-08-27 18:59 ` Dan Carpenter
2013-08-29 8:25 ` [patch v2] net/fec: cleanup types in fec_get_mac() Dan Carpenter
1 sibling, 1 reply; 9+ messages in thread
From: David Miller @ 2013-08-27 18:51 UTC (permalink / raw)
To: bhutchings
Cc: dan.carpenter, grant.likely, rob.herring, fabio.estevam, Frank.Li,
jim_baxter, B38611, netdev, devicetree, kernel-janitors
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 23 Aug 2013 13:44:29 +0100
> On Fri, 2013-08-23 at 12:49 +0300, Dan Carpenter wrote:
>> tmpaddr[] is a six byte array. We want to set the first four bytes on
>> the first line and the remaining two on the next line. The code assumes
>> that "unsigned long" is 32 bits and obviously that's not true on 64 bit
>> arches. It's better to just use u32 instead.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>> This is a static checker thing and I can't compile this file.
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index fdf9307..422b125 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1100,9 +1100,9 @@ static void fec_get_mac(struct net_device *ndev)
>> * 4) FEC mac registers set by bootloader
>> */
>> if (!is_valid_ether_addr(iap)) {
>> - *((unsigned long *) &tmpaddr[0]) =
>> + *((u32 *) &tmpaddr[0]) =
>> be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
>> - *((unsigned short *) &tmpaddr[4]) =
>> + *((u16 *) &tmpaddr[4]) =
>> be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
>> iap = &tmpaddr[0];
>> }
>
> This code also seems to have CPU vs big-endian byte order the wrong way
> round. readl() returns bytes in native order whereas we always store
> MAC addresses in network (big-endian) order. So I think it should be
> doing:
>
> *((__be32 *) &tmpaddr[0]) =
> cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
> *((__be16 *) &tmpaddr[4]) =
> cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
Dan please resubmit with Ben's suggested changes, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] net/fec: "u32" is more explicit than "unsigned long"
2013-08-27 18:51 ` David Miller
@ 2013-08-27 18:59 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2013-08-27 18:59 UTC (permalink / raw)
To: David Miller
Cc: bhutchings, grant.likely, rob.herring, fabio.estevam, Frank.Li,
jim_baxter, B38611, netdev, devicetree, kernel-janitors
On Tue, Aug 27, 2013 at 02:51:44PM -0400, David Miller wrote:
>
> Dan please resubmit with Ben's suggested changes, thanks.
Yes. Of course. Sorry for that, I'll resend tomorrow.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch v2] net/fec: cleanup types in fec_get_mac()
2013-08-23 12:44 ` Ben Hutchings
2013-08-27 18:51 ` David Miller
@ 2013-08-29 8:25 ` Dan Carpenter
2013-08-29 18:48 ` Ben Hutchings
2013-08-30 21:54 ` David Miller
1 sibling, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2013-08-29 8:25 UTC (permalink / raw)
To: Grant Likely
Cc: Rob Herring, David S. Miller, Fabio Estevam, Frank Li, Jim Baxter,
Fugang Duan, netdev, Ben Hutchings, devicetree, kernel-janitors
My static checker complains that on some arches unsigned longs can be 8
characters which is larger than the buffer is only 6 chars.
Additionally, Ben Hutchings points out that the buffer actually holds
big endian data and the buffer we are reading from is CPU endian.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
they say cpu_to_beXX().
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fdf9307..0b12866 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1100,10 +1100,10 @@ static void fec_get_mac(struct net_device *ndev)
* 4) FEC mac registers set by bootloader
*/
if (!is_valid_ether_addr(iap)) {
- *((unsigned long *) &tmpaddr[0]) =
- be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
- *((unsigned short *) &tmpaddr[4]) =
- be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
+ *((__be32 *) &tmpaddr[0]) =
+ cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
+ *((__be16 *) &tmpaddr[4]) =
+ cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
iap = &tmpaddr[0];
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch v2] net/fec: cleanup types in fec_get_mac()
2013-08-29 8:25 ` [patch v2] net/fec: cleanup types in fec_get_mac() Dan Carpenter
@ 2013-08-29 18:48 ` Ben Hutchings
2013-08-30 2:02 ` Duan Fugang-B38611
2013-08-30 21:54 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2013-08-29 18:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Grant Likely, Rob Herring, David S. Miller, Fabio Estevam,
Frank Li, Jim Baxter, Fugang Duan, netdev, devicetree,
kernel-janitors
On Thu, 2013-08-29 at 11:25 +0300, Dan Carpenter wrote:
> My static checker complains that on some arches unsigned longs can be 8
> characters which is larger than the buffer is only 6 chars.
> Additionally, Ben Hutchings points out that the buffer actually holds
> big endian data and the buffer we are reading from is CPU endian.
It's not really as clear-cut as that. :-) But I think it's slightly
more logical this way.
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
> they say cpu_to_beXX().
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index fdf9307..0b12866 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1100,10 +1100,10 @@ static void fec_get_mac(struct net_device *ndev)
> * 4) FEC mac registers set by bootloader
> */
> if (!is_valid_ether_addr(iap)) {
> - *((unsigned long *) &tmpaddr[0]) =
> - be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
> - *((unsigned short *) &tmpaddr[4]) =
> - be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> + *((__be32 *) &tmpaddr[0]) =
> + cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
> + *((__be16 *) &tmpaddr[4]) =
> + cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> iap = &tmpaddr[0];
> }
>
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [patch v2] net/fec: cleanup types in fec_get_mac()
2013-08-29 18:48 ` Ben Hutchings
@ 2013-08-30 2:02 ` Duan Fugang-B38611
2013-08-30 9:00 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Duan Fugang-B38611 @ 2013-08-30 2:02 UTC (permalink / raw)
To: Ben Hutchings, Dan Carpenter
Cc: Grant Likely, Rob Herring, David S. Miller, Estevam Fabio-R49496,
Li Frank-B20596, Jim Baxter, netdev@vger.kernel.org,
devicetree@vger.kernel.org, kernel-janitors@vger.kernel.org
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Data: Friday, August 30, 2013 2:49 AM
> To: Dan Carpenter
> Cc: Grant Likely; Rob Herring; David S. Miller; Estevam Fabio-R49496; Li
> Frank-B20596; Jim Baxter; Duan Fugang-B38611; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [patch v2] net/fec: cleanup types in fec_get_mac()
>
> On Thu, 2013-08-29 at 11:25 +0300, Dan Carpenter wrote:
> > My static checker complains that on some arches unsigned longs can be
> > 8 characters which is larger than the buffer is only 6 chars.
> > Additionally, Ben Hutchings points out that the buffer actually holds
> > big endian data and the buffer we are reading from is CPU endian.
>
> It's not really as clear-cut as that. :-) But I think it's slightly more
> logical this way.
>
Yes, it is not clear, pls remove somebody's name from the commit log.
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
>
> > ---
> > v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
> > they say cpu_to_beXX().
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index fdf9307..0b12866 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1100,10 +1100,10 @@ static void fec_get_mac(struct net_device *ndev)
> > * 4) FEC mac registers set by bootloader
> > */
> > if (!is_valid_ether_addr(iap)) {
> > - *((unsigned long *) &tmpaddr[0]) =
> > - be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
> > - *((unsigned short *) &tmpaddr[4]) =
> > - be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> > + *((__be32 *) &tmpaddr[0]) =
> > + cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
> > + *((__be16 *) &tmpaddr[4]) =
> > + cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> > iap = &tmpaddr[0];
> > }
> >
>
> --
> Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer;
> that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch v2] net/fec: cleanup types in fec_get_mac()
2013-08-30 2:02 ` Duan Fugang-B38611
@ 2013-08-30 9:00 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2013-08-30 9:00 UTC (permalink / raw)
To: Duan Fugang-B38611
Cc: Ben Hutchings, Grant Likely, Rob Herring, David S. Miller,
Estevam Fabio-R49496, Li Frank-B20596, Jim Baxter,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
kernel-janitors@vger.kernel.org
On Fri, Aug 30, 2013 at 02:02:00AM +0000, Duan Fugang-B38611 wrote:
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Data: Friday, August 30, 2013 2:49 AM
>
> > To: Dan Carpenter
> > Cc: Grant Likely; Rob Herring; David S. Miller; Estevam Fabio-R49496; Li
> > Frank-B20596; Jim Baxter; Duan Fugang-B38611; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [patch v2] net/fec: cleanup types in fec_get_mac()
> >
> > On Thu, 2013-08-29 at 11:25 +0300, Dan Carpenter wrote:
> > > My static checker complains that on some arches unsigned longs can be
> > > 8 characters which is larger than the buffer is only 6 chars.
> > > Additionally, Ben Hutchings points out that the buffer actually holds
> > > big endian data and the buffer we are reading from is CPU endian.
> >
> > It's not really as clear-cut as that. :-) But I think it's slightly more
> > logical this way.
> >
> Yes, it is not clear, pls remove somebody's name from the commit log.
Huh? No, I'm going to leave Ben's name as is. It's weird that you
would ask for that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2] net/fec: cleanup types in fec_get_mac()
2013-08-29 8:25 ` [patch v2] net/fec: cleanup types in fec_get_mac() Dan Carpenter
2013-08-29 18:48 ` Ben Hutchings
@ 2013-08-30 21:54 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2013-08-30 21:54 UTC (permalink / raw)
To: dan.carpenter
Cc: grant.likely, rob.herring, fabio.estevam, Frank.Li, jim_baxter,
B38611, netdev, bhutchings, devicetree, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 29 Aug 2013 11:25:14 +0300
> My static checker complains that on some arches unsigned longs can be 8
> characters which is larger than the buffer is only 6 chars.
> Additionally, Ben Hutchings points out that the buffer actually holds
> big endian data and the buffer we are reading from is CPU endian.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
> they say cpu_to_beXX().
Applied, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-30 21:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-23 9:49 [patch] net/fec: "u32" is more explicit than "unsigned long" Dan Carpenter
2013-08-23 12:44 ` Ben Hutchings
2013-08-27 18:51 ` David Miller
2013-08-27 18:59 ` Dan Carpenter
2013-08-29 8:25 ` [patch v2] net/fec: cleanup types in fec_get_mac() Dan Carpenter
2013-08-29 18:48 ` Ben Hutchings
2013-08-30 2:02 ` Duan Fugang-B38611
2013-08-30 9:00 ` Dan Carpenter
2013-08-30 21:54 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).