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@xxxxxxxxxx>:
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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Don't require alignment of mac addresses to u16.
Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
---
V2: Add missing SPOFF
Tested-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
Larry
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
