From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-ob0-f170.google.com ([209.85.214.170]:54850 "EHLO mail-ob0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751259Ab3CIX4T (ORCPT ); Sat, 9 Mar 2013 18:56:19 -0500 Received: by mail-ob0-f170.google.com with SMTP id wc20so2357164obb.29 for ; Sat, 09 Mar 2013 15:56:18 -0800 (PST) Message-ID: <513BCC20.70900@lwfinger.net> (sfid-20130310_005623_420575_B0F69549) Date: Sat, 09 Mar 2013 17:56:16 -0600 From: Larry Finger MIME-Version: 1.0 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= CC: Joe Perches , Hauke Mehrtens , linville@tuxdriver.com, linux-wireless@vger.kernel.org Subject: Re: [PATCH] ssb: fix unaligned access to mac address 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> <1361391065.2223.5.camel@joe-AO722> <512595BC.5030607@lwfinger.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 03/09/2013 05:02 PM, Rafał Miłecki wrote: > 2013/2/21 Larry Finger : >> 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 : >>>>>>>>> >>>>>>>>> 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 >> >> >> Tested-by: Larry Finger > > 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