From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perches-mx.perches.com ([206.117.179.246]:59538 "EHLO labridge.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S934523Ab3BTULH (ORCPT ); Wed, 20 Feb 2013 15:11:07 -0500 Message-ID: <1361391065.2223.5.camel@joe-AO722> (sfid-20130220_211113_631309_79BC6DEA) Subject: Re: [PATCH] ssb: fix unaligned access to mac address From: Joe Perches To: Hauke Mehrtens Cc: Larry Finger , =?UTF-8?Q?Rafa=C5=82_Mi=C5=82ecki?= , linville@tuxdriver.com, linux-wireless@vger.kernel.org Date: Wed, 20 Feb 2013 12:11:05 -0800 In-Reply-To: <51252132.3050603@hauke-m.de> References: <1361021140-19871-1-git-send-email-hauke@hauke-m.de> <1361381489.3065.1.camel@joe-AO722> <51250E6B.3030308@lwfinger.net> <1361384969.2219.4.camel@joe-AO722> <51252132.3050603@hauke-m.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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 : > >>>>> 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 > >>> 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 Don't require alignment of mac addresses to u16. Signed-off-by: Joe Perches --- 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