Skip to contentSkip to conversation

net/fec: "u32" is more explicit than "unsigned long"

PATCH In review Dan Carpenter 4 participants 9 messages Aug 23, 2013 – Aug 30, 2013

Subsystems (4)
Networking Network drivers Kernel janitors Freescale i.MX / MXC FEC driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 net/fec: "u32" is more explicit than "unsigned long"

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 <[email protected]>

---
This is a static checker thing and I can't compile this file.

drivers/net/ethernet/freescale/fec_main.c +0 -2
index fdf9307..422b125 100644
@@ -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];
 	}
#1 lore raw
Quoted 14 lines · Dan Carpenter

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 <[email protected]>

---
This is a static checker thing and I can't compile this file.

drivers/net/ethernet/freescale/fec_main.c +0 -0
index fdf9307..422b125 100644

drivers/net/ethernet/freescale/fec_main.c static void fec_get_mac(struct net_device *ndev) · line 1104

Quoted 7 lines
drivers/net/ethernet/freescale/fec_main.c +0 -2
@@ -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.

Signature · 3 lines
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.
maintainer
#2 lore raw

From: Ben Hutchings <[email protected]>
Date: Fri, 23 Aug 2013 13:44:29 +0100

Quoted 28 lines · Dan Carpenter
Quoted 14 lines

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 <[email protected]>

---
This is a static checker thing and I can't compile this file.

drivers/net/ethernet/freescale/fec_main.c +0 -0
index fdf9307..422b125 100644

drivers/net/ethernet/freescale/fec_main.c static void fec_get_mac(struct net_device *ndev) · line 1104

Quoted 7 lines
drivers/net/ethernet/freescale/fec_main.c +0 -2
@@ -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:

Quoted 2 lines
		*((__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.

author
#3 lore raw
Quoted 2 lines · David Miller

Dan please resubmit with Ben's suggested changes, thanks.

Yes. Of course. Sorry for that, I'll resend tomorrow.

regards,
dan carpenter

author
#4 lore raw

PATCH v2 1/1 net/fec: cleanup types in fec_get_mac()

In reply to Ben Hutchings

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 <[email protected]>

---
v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
they say cpu_to_beXX().

drivers/net/ethernet/freescale/fec_main.c +2 -2
index fdf9307..0b12866 100644
@@ -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];
 	}
Reviewed-by on v2 1/1
#5 lore raw

Re: [patch v2] net/fec: cleanup types in fec_get_mac()

Quoted 4 lines · Dan Carpenter

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.

Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Ben Hutchings <[email protected]>
Quoted 19 lines

---
v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
they say cpu_to_beXX().

drivers/net/ethernet/freescale/fec_main.c +2 -2
index fdf9307..0b12866 100644
@@ -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];
 	}
Signature · 3 lines
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.
#6 lore raw

RE: [patch v2] net/fec: cleanup types in fec_get_mac()

RnJvbTogQmVuIEh1dGNoaW5ncyBbbWFpbHRvOmJodXRjaGluZ3NAc29sYXJmbGFyZS5jb21dDQpE YXRhOiBGcmlkYXksIEF1Z3VzdCAzMCwgMjAxMyAyOjQ5IEFNDQoNCj4gVG86IERhbiBDYXJwZW50 ZXINCj4gQ2M6IEdyYW50IExpa2VseTsgUm9iIEhlcnJpbmc7IERhdmlkIFMuIE1pbGxlcjsgRXN0 ZXZhbSBGYWJpby1SNDk0OTY7IExpDQo+IEZyYW5rLUIyMDU5NjsgSmltIEJheHRlcjsgRHVhbiBG dWdhbmctQjM4NjExOyBuZXRkZXZAdmdlci5rZXJuZWwub3JnOw0KPiBkZXZpY2V0cmVlQHZnZXIu a2VybmVsLm9yZzsga2VybmVsLWphbml0b3JzQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0OiBS ZTogW3BhdGNoIHYyXSBuZXQvZmVjOiBjbGVhbnVwIHR5cGVzIGluIGZlY19nZXRfbWFjKCkNCj4g DQo+IE9uIFRodSwgMjAxMy0wOC0yOSBhdCAxMToyNSArMDMwMCwgRGFuIENhcnBlbnRlciB3cm90 ZToNCj4gPiBNeSBzdGF0aWMgY2hlY2tlciBjb21wbGFpbnMgdGhhdCBvbiBzb21lIGFyY2hlcyB1 bnNpZ25lZCBsb25ncyBjYW4gYmUNCj4gPiA4IGNoYXJhY3RlcnMgd2hpY2ggaXMgbGFyZ2VyIHRo YW4gdGhlIGJ1ZmZlciBpcyBvbmx5IDYgY2hhcnMuDQo+ID4gQWRkaXRpb25hbGx5LCBCZW4gSHV0 Y2hpbmdzIHBvaW50cyBvdXQgdGhhdCB0aGUgYnVmZmVyIGFjdHVhbGx5IGhvbGRzDQo+ID4gYmln IGVuZGlhbiBkYXRhIGFuZCB0aGUgYnVmZmVyIHdlIGFyZSByZWFkaW5nIGZyb20gaXMgQ1BVIGVu ZGlhbi4NCj4gDQo+IEl0J3Mgbm90IHJlYWxseSBhcyBjbGVhci1jdXQgYXMgdGhhdC4gOi0pICBC dXQgSSB0aGluayBpdCdzIHNsaWdodGx5IG1vcmUNCj4gbG9naWNhbCB0aGlzIHdheS4NCj4gDQpZ ZXMsIGl0IGlzIG5vdCBjbGVhciwgcGxzIHJlbW92ZSBzb21lYm9keSdzIG5hbWUgZnJvbSB0aGUg Y29tbWl0IGxvZy4NCg0KPiA+IFNpZ25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRhbi5jYXJw ZW50ZXJAb3JhY2xlLmNvbT4NCj4gDQo+IFJldmlld2VkLWJ5OiBCZW4gSHV0Y2hpbmdzIDxiaHV0 Y2hpbmdzQHNvbGFyZmxhcmUuY29tPg0KPiANCj4gPiAtLS0NCj4gPiB2MjogZml4IGVuZGlhbiBh bm5vdGF0aW9ucyBhbmQgcmV2ZXJzZSB0aGUgYmVYWF90b19jcHUoKSBjYWxscyBzbyB0aGF0DQo+ ID4gICAgIHRoZXkgc2F5IGNwdV90b19iZVhYKCkuDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvZHJp dmVycy9uZXQvZXRoZXJuZXQvZnJlZXNjYWxlL2ZlY19tYWluLmMNCj4gPiBiL2RyaXZlcnMvbmV0 L2V0aGVybmV0L2ZyZWVzY2FsZS9mZWNfbWFpbi5jDQo+ID4gaW5kZXggZmRmOTMwNy4uMGIxMjg2 NiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL25ldC9ldGhlcm5ldC9mcmVlc2NhbGUvZmVjX21h aW4uYw0KPiA+ICsrKyBiL2RyaXZlcnMvbmV0L2V0aGVybmV0L2ZyZWVzY2FsZS9mZWNfbWFpbi5j DQo+ID4gQEAgLTExMDAsMTAgKzExMDAsMTAgQEAgc3RhdGljIHZvaWQgZmVjX2dldF9tYWMoc3Ry dWN0IG5ldF9kZXZpY2UgKm5kZXYpDQo+ID4gIAkgKiA0KSBGRUMgbWFjIHJlZ2lzdGVycyBzZXQg YnkgYm9vdGxvYWRlcg0KPiA+ICAJICovDQo+ID4gIAlpZiAoIWlzX3ZhbGlkX2V0aGVyX2FkZHIo aWFwKSkgew0KPiA+IC0JCSooKHVuc2lnbmVkIGxvbmcgKikgJnRtcGFkZHJbMF0pID0NCj4gPiAt CQkJYmUzMl90b19jcHUocmVhZGwoZmVwLT5od3AgKyBGRUNfQUREUl9MT1cpKTsNCj4gPiAtCQkq KCh1bnNpZ25lZCBzaG9ydCAqKSAmdG1wYWRkcls0XSkgPQ0KPiA+IC0JCQliZTE2X3RvX2NwdShy ZWFkbChmZXAtPmh3cCArIEZFQ19BRERSX0hJR0gpID4+IDE2KTsNCj4gPiArCQkqKChfX2JlMzIg KikgJnRtcGFkZHJbMF0pID0NCj4gPiArCQkJY3B1X3RvX2JlMzIocmVhZGwoZmVwLT5od3AgKyBG RUNfQUREUl9MT1cpKTsNCj4gPiArCQkqKChfX2JlMTYgKikgJnRtcGFkZHJbNF0pID0NCj4gPiAr CQkJY3B1X3RvX2JlMTYocmVhZGwoZmVwLT5od3AgKyBGRUNfQUREUl9ISUdIKSA+PiAxNik7DQo+ ID4gIAkJaWFwID0gJnRtcGFkZHJbMF07DQo+ID4gIAl9DQo+ID4NCj4gDQo+IC0tDQo+IEJlbiBI dXRjaGluZ3MsIFN0YWZmIEVuZ2luZWVyLCBTb2xhcmZsYXJlIE5vdCBzcGVha2luZyBmb3IgbXkg ZW1wbG95ZXI7DQo+IHRoYXQncyB0aGUgbWFya2V0aW5nIGRlcGFydG1lbnQncyBqb2IuDQo+IFRo ZXkgYXNrZWQgdXMgdG8gbm90ZSB0aGF0IFNvbGFyZmxhcmUgcHJvZHVjdCBuYW1lcyBhcmUgdHJh ZGVtYXJrZWQuDQo+IA0KDQoNCg0K

author
#7 lore raw

Re: [patch v2] net/fec: cleanup types in fec_get_mac()

Quoted 15 lines · On Fri, Aug 30, 2013 at 02:02:00AM +0000, Duan Fugang-B38611 wrote:

From: Ben Hutchings [mailto:[email protected]]
Data: Friday, August 30, 2013 2:49 AM

Quoted 12 lines

To: Dan Carpenter
Cc: Grant Likely; Rob Herring; David S. Miller; Estevam Fabio-R49496; Li Frank-B20596; Jim Baxter; Duan Fugang-B38611; [email protected]; [email protected]; [email protected]
Subject: Re: [patch v2] net/fec: cleanup types in fec_get_mac()

Quoted 4 lines · Dan Carpenter

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.

Quoted 4 lines
Quoted 3 lines

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

maintainer Applied
#8 lore raw

Re: [patch v2] net/fec: cleanup types in fec_get_mac()

In reply to Dan Carpenter

From: Dan Carpenter <[email protected]>
Date: Thu, 29 Aug 2013 11:25:14 +0300

Quoted 9 lines

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 <[email protected]>

---
v2: fix endian annotations and reverse the beXX_to_cpu() calls so that
they say cpu_to_beXX().

Applied, thanks.