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

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

On 03/09/2013 05:02 PM, Rafał Miłecki wrote:
> 2013/2/21 Larry Finger <Larry.Finger@lwfinger.net>:
>> On 02/20/2013 02:11 PM, Joe Perches wrote:
>>>
>>> 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
>>
>>
>> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
>
> I didn't confirm my issue to be related to this patch, but with recent
> kernels my MAC has changed from
> 00:11:22:33:44:55
> to
> 11:00:33:22:55:44
>
> Larry: did you verify MAC didn't change for you?

I just checked on x86_64, and the bytes are swapped here too. Do you want to 
push the patch, or should I do it?

Larry