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

[Larry Finger <Larry.Finger@lwfinger.net>]

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.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>   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..62fc9b5 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[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);
>

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.

Larry