Message ID | 20130823094920.GP31293@elgon.mountain |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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.
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. -- 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
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 -- 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/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]; }
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. -- 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