Re: [PATCH] ssb: fix unaligned access to mac address

[Joe Perches <joe@perches.com>]

On Wed, 2013-02-20 at 20:17 +0100, Hauke Mehrtens wrote:
> On 02/20/2013 07:29 PM, Joe Perches wrote:
> > On Wed, 2013-02-20 at 11:56 -0600, Larry Finger wrote:
> >> On 02/20/2013 11:31 AM, Joe Perches wrote:
> >>> On Mon, 2013-02-18 at 17:09 +0100, Rafał Miłecki wrote:
> >>>> 2013/2/16 Hauke Mehrtens <hauke@hauke-m.de>:
> >>>>> The mac address should be aligned to u16 to prevent an unaligned access
> >>>>> in drivers/ssb/pci.c where it is casted to __be16.
> >>> []
> >>>>> diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
> >>> []
> >>>>> @@ -26,6 +26,7 @@ struct ssb_sprom_core_pwr_info {
> >>>>>
> >>>>>   struct ssb_sprom {
> >>>>>          u8 revision;
> >>>>> +       u8 country_code;        /* Country Code */
> >>>>>          u8 il0mac[6];           /* MAC address for 802.11b/g */
> >>>>
> >>>> It looks a little hacky to me too, it's easy to forget about that
> >>>> requirement and break that again in the future.
> >>>>
> >>>> What about not casting il0mac to u16 at all? Maybe we should just fill
> >>>> it as u8 (which it is)?
> >>>>
> >>>
> >>> Perhaps this?
> >>>
> >>> From: Joe Perches <joe@perches.com>
> >>> Subject: [PATCH] ssb: pci: Standardize a function to get mac address
> >>>
> >>> Don't require alignment of mac addresses to u16.
> 
> ...
> 
> >> I like the looks of sprom_get_mac() over that ugly *(((__be16 *)out->il0mac) 
> >> construct, but this patch breaks ssb. The resulting MAC address is all ones. I 
> >> have not yet figured out the problem.
> > 
> > Dunno, I must have done something stupid.
> > 
> > I don't have one of these but the transform
> > looked correct when I did  it.
> > 
> > I'm not sure it's the best solution anyway
> > because some of the other ether address
> > functions like compare_ether_addr also
> > require 2 byte alignment and cast to u16.
> 
> Your patch looks good (haven't tested it), but as you said we should
> still align the mac addresses to u16 so functions like
> compare_ether_addr work with them.
> 
> There is also a v2 of my patch:
> http://www.spinics.net/lists/linux-wireless/msg103628.html

Seems OK to me, though I suggest using ETH_ALEN
instead of 6.

Maybe using both patches would be better still.

And as long as I'm futzing with ssb, there's another
logging cleanup to go with it.

Here's a V2 with the missing SPOFF fixed.

Subject: [PATCH 1/2] ssb: pci: Standardize a function to get mac address
From: Joe Perches <joe@perches.com>

Don't require alignment of mac addresses to u16.

Signed-off-by: Joe Perches <joe@perches.com>
---

V2: Add missing SPOFF

 drivers/ssb/pci.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index e9d9496..4ec0bdb 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -231,6 +231,15 @@ static inline u8 ssb_crc8(u8 crc, u8 data)
 	return t[crc ^ data];
 }
 
+static void sprom_get_mac(char *mac, const u16 *in)
+{
+	int i;
+	for (i = 0; i < 3; i++) {
+		*mac++ = in[i];
+		*mac++ = in[i] >> 8;
+	}
+}
+
 static u8 ssb_sprom_crc(const u16 *sprom, u16 size)
 {
 	int word;
@@ -341,8 +350,6 @@ static s8 r123_extract_antgain(u8 sprom_revision, const u16 *in,
 
 static void sprom_extract_r123(struct ssb_sprom *out, const u16 *in)
 {
-	int i;
-	u16 v;
 	u16 loc[3];
 
 	if (out->revision == 3)			/* rev 3 moved MAC */
@@ -352,19 +359,10 @@ static void sprom_extract_r123(struct ssb_sprom *out, const u16 *in)
 		loc[1] = SSB_SPROM1_ET0MAC;
 		loc[2] = SSB_SPROM1_ET1MAC;
 	}
-	for (i = 0; i < 3; i++) {
-		v = in[SPOFF(loc[0]) + i];
-		*(((__be16 *)out->il0mac) + i) = cpu_to_be16(v);
-	}
+	sprom_get_mac(out->il0mac, &in[SPOFF(loc[0])]);
 	if (out->revision < 3) { 	/* only rev 1-2 have et0, et1 */
-		for (i = 0; i < 3; i++) {
-			v = in[SPOFF(loc[1]) + i];
-			*(((__be16 *)out->et0mac) + i) = cpu_to_be16(v);
-		}
-		for (i = 0; i < 3; i++) {
-			v = in[SPOFF(loc[2]) + i];
-			*(((__be16 *)out->et1mac) + i) = cpu_to_be16(v);
-		}
+		sprom_get_mac(out->et0mac, &in[SPOFF(loc[1])]);
+		sprom_get_mac(out->et1mac, &in[SPOFF(loc[2])]);
 	}
 	SPEX(et0phyaddr, SSB_SPROM1_ETHPHY, SSB_SPROM1_ETHPHY_ET0A, 0);
 	SPEX(et1phyaddr, SSB_SPROM1_ETHPHY, SSB_SPROM1_ETHPHY_ET1A,
@@ -454,19 +452,15 @@ static void sprom_extract_r458(struct ssb_sprom *out, const u16 *in)
 
 static void sprom_extract_r45(struct ssb_sprom *out, const u16 *in)
 {
-	int i;
-	u16 v;
 	u16 il0mac_offset;
 
 	if (out->revision == 4)
 		il0mac_offset = SSB_SPROM4_IL0MAC;
 	else
 		il0mac_offset = SSB_SPROM5_IL0MAC;
-	/* extract the MAC address */
-	for (i = 0; i < 3; i++) {
-		v = in[SPOFF(il0mac_offset) + i];
-		*(((__be16 *)out->il0mac) + i) = cpu_to_be16(v);
-	}
+
+	sprom_get_mac(out->il0mac, &in[SPOFF(il0mac_offset)]);
+
 	SPEX(et0phyaddr, SSB_SPROM4_ETHPHY, SSB_SPROM4_ETHPHY_ET0A, 0);
 	SPEX(et1phyaddr, SSB_SPROM4_ETHPHY, SSB_SPROM4_ETHPHY_ET1A,
 	     SSB_SPROM4_ETHPHY_ET1A_SHIFT);
@@ -530,7 +524,7 @@ static void sprom_extract_r45(struct ssb_sprom *out, const u16 *in)
 static void sprom_extract_r8(struct ssb_sprom *out, const u16 *in)
 {
 	int i;
-	u16 v, o;
+	u16 o;
 	u16 pwr_info_offset[] = {
 		SSB_SROM8_PWR_INFO_CORE0, SSB_SROM8_PWR_INFO_CORE1,
 		SSB_SROM8_PWR_INFO_CORE2, SSB_SROM8_PWR_INFO_CORE3
@@ -539,10 +533,8 @@ static void sprom_extract_r8(struct ssb_sprom *out, const u16 *in)
 			ARRAY_SIZE(out->core_pwr_info));
 
 	/* extract the MAC address */
-	for (i = 0; i < 3; i++) {
-		v = in[SPOFF(SSB_SPROM8_IL0MAC) + i];
-		*(((__be16 *)out->il0mac) + i) = cpu_to_be16(v);
-	}
+	sprom_get_mac(out->il0mac, &in[SPOFF(SSB_SPROM8_IL0MAC)]);
+
 	SPEX(board_rev, SSB_SPROM8_BOARDREV, 0xFFFF, 0);
 	SPEX(alpha2[0], SSB_SPROM8_CCODE, 0xff00, 8);
 	SPEX(alpha2[1], SSB_SPROM8_CCODE, 0x00ff, 0);
-- 
1.8.1.2.459.gbcd45b4.dirty