diff mbox

[16/16] wl1251: Add sysfs file address for setting permanent mac address

Message ID 1382819655-30430-17-git-send-email-pali.rohar@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Pali Rohár Oct. 26, 2013, 8:34 p.m. UTC
Driver wl1251 generating mac address randomly at startup and there is no way to
set permanent mac address via SET_IEEE80211_PERM_ADDR. This patch export sysfs
file which can set permanent mac address by userspace helper program. Patch is
needed for devices which do not store mac address in internal wl1251 eeprom.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |   52 +++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Johannes Berg Oct. 28, 2013, 1:45 p.m. UTC | #1
On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> Driver wl1251 generating mac address randomly at startup and there is no way to
> set permanent mac address via SET_IEEE80211_PERM_ADDR. This patch export sysfs
> file which can set permanent mac address by userspace helper program. Patch is
> needed for devices which do not store mac address in internal wl1251 eeprom.

This doesn't really seem like a good idea since you can also just use
'ip' or whatever to set the MAC address.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 28, 2013, 1:49 p.m. UTC | #2
On Monday 28 October 2013 14:45:05 Johannes Berg wrote:
> On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> > Driver wl1251 generating mac address randomly at startup and
> > there is no way to set permanent mac address via
> > SET_IEEE80211_PERM_ADDR. This patch export sysfs file which
> > can set permanent mac address by userspace helper program.
> > Patch is needed for devices which do not store mac address
> > in internal wl1251 eeprom.
> 
> This doesn't really seem like a good idea since you can also
> just use 'ip' or whatever to set the MAC address.
> 
> johannes

AFAIK you cannot set permanent address (show by ethtool -P wlan0) 
via ip/ifconfig.
Johannes Berg Oct. 28, 2013, 1:55 p.m. UTC | #3
On Mon, 2013-10-28 at 14:49 +0100, Pali Rohár wrote:
> On Monday 28 October 2013 14:45:05 Johannes Berg wrote:
> > On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> > > Driver wl1251 generating mac address randomly at startup and
> > > there is no way to set permanent mac address via
> > > SET_IEEE80211_PERM_ADDR. This patch export sysfs file which
> > > can set permanent mac address by userspace helper program.
> > > Patch is needed for devices which do not store mac address
> > > in internal wl1251 eeprom.
> > 
> > This doesn't really seem like a good idea since you can also
> > just use 'ip' or whatever to set the MAC address.
> > 
> > johannes
> 
> AFAIK you cannot set permanent address (show by ethtool -P wlan0) 
> via ip/ifconfig.

You probably can't, but that address also doesn't matter at all and
isn't really used anywhere.

johannes


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 28, 2013, 2 p.m. UTC | #4
On Monday 28 October 2013 14:55:22 Johannes Berg wrote:
> On Mon, 2013-10-28 at 14:49 +0100, Pali Rohár wrote:
> > On Monday 28 October 2013 14:45:05 Johannes Berg wrote:
> > > On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> > > > Driver wl1251 generating mac address randomly at startup
> > > > and there is no way to set permanent mac address via
> > > > SET_IEEE80211_PERM_ADDR. This patch export sysfs file
> > > > which can set permanent mac address by userspace helper
> > > > program. Patch is needed for devices which do not store
> > > > mac address in internal wl1251 eeprom.
> > > 
> > > This doesn't really seem like a good idea since you can
> > > also just use 'ip' or whatever to set the MAC address.
> > > 
> > > johannes
> > 
> > AFAIK you cannot set permanent address (show by ethtool -P
> > wlan0) via ip/ifconfig.
> 
> You probably can't, but that address also doesn't matter at
> all and isn't really used anywhere.
> 
> johannes

There are some (proprietary) applications which using permanent 
address for something...

And also more important: some network managing tools using it. 
NetworkManager resetting current MAC address to permanent one 
before starting configuring interface.

So this will lead to never use correct MAC address (but random 
permanent one) assigned for that wl1251 wireless card...
Dan Williams Oct. 28, 2013, 2:46 p.m. UTC | #5
On Mon, 2013-10-28 at 15:00 +0100, Pali Rohár wrote:
> On Monday 28 October 2013 14:55:22 Johannes Berg wrote:
> > On Mon, 2013-10-28 at 14:49 +0100, Pali Rohár wrote:
> > > On Monday 28 October 2013 14:45:05 Johannes Berg wrote:
> > > > On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> > > > > Driver wl1251 generating mac address randomly at startup
> > > > > and there is no way to set permanent mac address via
> > > > > SET_IEEE80211_PERM_ADDR. This patch export sysfs file
> > > > > which can set permanent mac address by userspace helper
> > > > > program. Patch is needed for devices which do not store
> > > > > mac address in internal wl1251 eeprom.
> > > > 
> > > > This doesn't really seem like a good idea since you can
> > > > also just use 'ip' or whatever to set the MAC address.
> > > > 
> > > > johannes
> > > 
> > > AFAIK you cannot set permanent address (show by ethtool -P
> > > wlan0) via ip/ifconfig.
> > 
> > You probably can't, but that address also doesn't matter at
> > all and isn't really used anywhere.
> > 
> > johannes
> 
> There are some (proprietary) applications which using permanent 
> address for something...
> 
> And also more important: some network managing tools using it. 
> NetworkManager resetting current MAC address to permanent one 
> before starting configuring interface.
> 
> So this will lead to never use correct MAC address (but random 
> permanent one) assigned for that wl1251 wireless card...

If the device doesn't actually *have* a permanent MAC address, then it
shouldn't be returning one via ethtool, and should return an error for
ETHTOOL_GPERMADDR.  Setting the permanent MAC address shouldn't ever be
allowed except by the driver inspecting EEPROM, and certainly not via
sysfs.

mac80211 does have to do some special stuff due to virtual interfaces
and such, but in general, if the MAC address is randomly generated,
neither the driver nor mac80211 should be reporting that address as the
permanent address, only as the current one.

Dan


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Oct. 28, 2013, 2:56 p.m. UTC | #6
On Mon, 2013-10-28 at 09:46 -0500, Dan Williams wrote:

> If the device doesn't actually *have* a permanent MAC address, then it
> shouldn't be returning one via ethtool, and should return an error for
> ETHTOOL_GPERMADDR.

There's currently no provision for that, but I agree it would be better
to do so.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 28, 2013, 3:04 p.m. UTC | #7
On Monday 28 October 2013 15:56:55 Johannes Berg wrote:
> On Mon, 2013-10-28 at 09:46 -0500, Dan Williams wrote:
> > If the device doesn't actually *have* a permanent MAC
> > address, then it shouldn't be returning one via ethtool,
> > and should return an error for ETHTOOL_GPERMADDR.
> 
> There's currently no provision for that, but I agree it would
> be better to do so.
> 
> johannes

So what to do with devices which has MAC address stored in some 
obscure place and there is userspace binary which can read it?

I think that there should be some way to tell kernel that *this* 
is the permanent address and it should use it.

This is reason why I proposed patch which adding sysfs entry for 
setting permanent address from userspace in wl1251 driver.
Dan Williams Oct. 28, 2013, 3:29 p.m. UTC | #8
On Mon, 2013-10-28 at 16:04 +0100, Pali Rohár wrote:
> On Monday 28 October 2013 15:56:55 Johannes Berg wrote:
> > On Mon, 2013-10-28 at 09:46 -0500, Dan Williams wrote:
> > > If the device doesn't actually *have* a permanent MAC
> > > address, then it shouldn't be returning one via ethtool,
> > > and should return an error for ETHTOOL_GPERMADDR.
> > 
> > There's currently no provision for that, but I agree it would
> > be better to do so.
> > 
> > johannes
> 
> So what to do with devices which has MAC address stored in some 
> obscure place and there is userspace binary which can read it?
> 
> I think that there should be some way to tell kernel that *this* 
> is the permanent address and it should use it.
> 
> This is reason why I proposed patch which adding sysfs entry for 
> setting permanent address from userspace in wl1251 driver.

Ok, so the N900's MAC is stored in the "cal partition", which is a
region of flash exposed to the OS as /dev/mtd1.  That also stores the
regulatory domain.  Typically userspace binaries are used to pull out
this and other data (see
https://dev.openwrt.org/browser/packages/utils/calvaria/files/src/calvaria.c ) which is then used to initialize the device.

Any idea what kernel driver is used to expose the CAL partition
as /dev/mtd1?  If it's nothing special maybe a small EEPROM-style driver
could be written to read the relevant areas (and since they don't ever
change, don't need to worry about something else writing at the same
time).

Dan


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger Oct. 28, 2013, 3:33 p.m. UTC | #9
On Mon, 28 Oct 2013 15:56:55 +0100
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Mon, 2013-10-28 at 09:46 -0500, Dan Williams wrote:
> 
> > If the device doesn't actually *have* a permanent MAC address, then it
> > shouldn't be returning one via ethtool, and should return an error for
> > ETHTOOL_GPERMADDR.
> 
> There's currently no provision for that, but I agree it would be better
> to do so.
> 
> johannes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

The current netdevice API handle the case of a device without a permanent
MAC address, slightly differently.

If device does not have a permanent address,
then:
  1. dev->addr_assign_type should not be NET_ADDR_PERM
  2. when device is registered dev->perm_addr will not be set
     and retain all zeros value
  4. when ethtool gets address it will return all zeros which
     is not a valid address.

This case doesn't seem to be handled threo mac80211 API's



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 28, 2013, 4:21 p.m. UTC | #10
On Monday 28 October 2013 16:29:21 Dan Williams wrote:
> On Mon, 2013-10-28 at 16:04 +0100, Pali Rohár wrote:
> > On Monday 28 October 2013 15:56:55 Johannes Berg wrote:
> > > On Mon, 2013-10-28 at 09:46 -0500, Dan Williams wrote:
> > > > If the device doesn't actually *have* a permanent MAC
> > > > address, then it shouldn't be returning one via ethtool,
> > > > and should return an error for ETHTOOL_GPERMADDR.
> > > 
> > > There's currently no provision for that, but I agree it
> > > would be better to do so.
> > > 
> > > johannes
> > 
> > So what to do with devices which has MAC address stored in
> > some obscure place and there is userspace binary which can
> > read it?
> > 
> > I think that there should be some way to tell kernel that
> > *this* is the permanent address and it should use it.
> > 
> > This is reason why I proposed patch which adding sysfs entry
> > for setting permanent address from userspace in wl1251
> > driver.
> 
> Ok, so the N900's MAC is stored in the "cal partition", which
> is a region of flash exposed to the OS as /dev/mtd1.  That
> also stores the regulatory domain.  Typically userspace
> binaries are used to pull out this and other data (see
> https://dev.openwrt.org/browser/packages/utils/calvaria/files/
> src/calvaria.c ) which is then used to initialize the device.
> 
> Any idea what kernel driver is used to expose the CAL
> partition as /dev/mtd1?  If it's nothing special maybe a
> small EEPROM-style driver could be written to read the
> relevant areas (and since they don't ever change, don't need
> to worry about something else writing at the same time).
> 
> Dan

/dev/mtd1 is second (0 is first) OneNand partition. Nothing 
special. Kernel driver for OnaNand is already in mainline kernel.

That partition also has NVS device data for wl1251 chip.

And you are right calvaria has GPL v2 code for parsing data in 
that partition.
Ben Hutchings Oct. 28, 2013, 11:50 p.m. UTC | #11
On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> Driver wl1251 generating mac address randomly at startup and there is no way to
> set permanent mac address via SET_IEEE80211_PERM_ADDR. This patch export sysfs
> file which can set permanent mac address by userspace helper program. Patch is
> needed for devices which do not store mac address in internal wl1251 eeprom.
[...]

This belongs in struct wl12xx_platform_data or (better) Device Tree
properties.  It definitely should not be settable once the device is
registered.

Ben.
diff mbox

Patch

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 29625c2..91a009c 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1375,6 +1375,46 @@  static const struct ieee80211_ops wl1251_ops = {
 	.get_survey = wl1251_op_get_survey,
 };
 
+static ssize_t wl1251_sysfs_show_address(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct wl1251 *wl = dev_get_drvdata(dev);
+	ssize_t len;
+
+	/* FIXME: what's the maximum length of buf? page size?*/
+	len = 500;
+
+	len = snprintf(buf, len, "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x\n",
+		       wl->mac_addr[0], wl->mac_addr[1], wl->mac_addr[2],
+		       wl->mac_addr[3], wl->mac_addr[4], wl->mac_addr[5]);
+
+	return len;
+}
+
+static ssize_t wl1251_sysfs_store_address(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct wl1251 *wl = dev_get_drvdata(dev);
+	unsigned int addr[6];
+	int ret, i;
+
+	ret = sscanf(buf, "%2x:%2x:%2x:%2x:%2x:%2x\n",
+			&addr[0], &addr[1], &addr[2],
+			&addr[3], &addr[4], &addr[5]);
+
+	if (ret != 6)
+		return -EINVAL;
+
+	for (i = 0; i < 6; i++)
+		wl->mac_addr[i] = addr[i] & 0xff;
+
+	SET_IEEE80211_PERM_ADDR(wl->hw, wl->mac_addr);
+
+	return count;
+}
+
 static ssize_t wl1251_sysfs_show_tx_mgmt_frm_rate(struct device *dev,
 						  struct device_attribute *attr,
 						  char *buf)
@@ -1584,6 +1624,10 @@  out:
 	return count;
 }
 
+static DEVICE_ATTR(address, S_IRUGO | S_IWUSR,
+		   wl1251_sysfs_show_address,
+		   wl1251_sysfs_store_address);
+
 static DEVICE_ATTR(tx_mgmt_frm_rate, S_IRUGO | S_IWUSR,
 		   wl1251_sysfs_show_tx_mgmt_frm_rate,
 		   wl1251_sysfs_store_tx_mgmt_frm_rate);
@@ -1728,6 +1772,14 @@  int wl1251_init_ieee80211(struct wl1251 *wl)
 	}
 	dev_set_drvdata(&wl1251_device.dev, wl);
 
+	/* Create sysfs file address */
+	ret = device_create_file(&wl1251_device.dev,
+				 &dev_attr_address);
+	if (ret < 0) {
+		wl1251_error("failed to create sysfs file address");
+		goto out;
+	}
+
 	/* Create sysfs file tx_mgmt_frm_rate */
 	ret = device_create_file(&wl1251_device.dev,
 				 &dev_attr_tx_mgmt_frm_rate);