| Message ID | 20110324212737.14936.21228.stgit@otae.warmcat.com |
|---|---|
| State | New |
| Headers | show |
Hi, one Minor comment. >-----Original Message----- >From: linux-omap-owner@vger.kernel.org >[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Andy Green >Sent: Friday, March 25, 2011 2:58 AM >To: linux-arm-kernel@lists.infradead.org; linux-omap@vger.kernel.org >Cc: patches@linaro.org; nicolas.pitre@linaro.org; >arnd@arndb.de; x0132446@ti.com; s-jan@ti.com; >tony@atomide.com; Alan Cox; Andy Green >Subject: [RFC PATCH 2/2] OMAP2+: PANDA: Fix up random or >missing MAC addresses for eth0 and wlan0 > >This patch registers a network device notifier callback to set the mac >addresses for the onboard network assets of Panda correctly, >despite the >drivers involved have used a random or all-zeros MAC address. > >The technique was suggested by Alan Cox on lkml. > >It works by device path so it corrects the MAC addresses even if the >drivers are in modules loaded in an order that changes their interface >name from usual (eg, the onboard module might be "wlan1" if there is a >USB wireless stick plugged in and its module is inserted first.) > >Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> >Signed-off-by: Andy Green <andy.green@linaro.org> >--- > > arch/arm/mach-omap2/board-omap4panda.c | 91 >++++++++++++++++++++++++++++++++ > 1 files changed, 91 insertions(+), 0 deletions(-) > >diff --git a/arch/arm/mach-omap2/board-omap4panda.c >b/arch/arm/mach-omap2/board-omap4panda.c >index 80b8860..0b92873 100644 >--- a/arch/arm/mach-omap2/board-omap4panda.c >+++ b/arch/arm/mach-omap2/board-omap4panda.c >@@ -28,9 +28,12 @@ > #include <linux/regulator/machine.h> > #include <linux/regulator/fixed.h> > #include <linux/wl12xx.h> >+#include <linux/netdevice.h> >+#include <linux/if_ether.h> > > #include <mach/hardware.h> > #include <mach/omap4-common.h> >+#include <mach/id.h> > #include <asm/mach-types.h> > #include <asm/mach/arch.h> > #include <asm/mach/map.h> >@@ -506,6 +509,92 @@ static inline void board_serial_init(void) > } > #endif > >+/* >+ * These device paths represent the onboard USB <-> Ethernet >bridge, and >+ * the WLAN module on Panda, both of which need their random >or all-zeros >+ * mac address replacing with a per-cpu stable generated one >+ */ >+ >+static const char * const panda_fixup_mac_device_paths[] = { >+ "usb1/1-1/1-1.1/1-1.1:1.0", >+ "mmc1:0001:2", >+}; >+ >+static int panda_device_path_need_mac(struct device *dev) >+{ >+ const char **try = panda_fixup_mac_device_paths; >+ const char *path; >+ int count = ARRAY_SIZE(panda_fixup_mac_device_paths); >+ const char *p; >+ int len; >+ struct device *devn; >+ >+ while (count--) { >+ >+ p = *try + strlen(*try); >+ devn = dev; >+ >+ while (devn) { >+ >+ path = dev_name(devn); >+ len = strlen(path); >+ >+ if ((p - *try) < len) { >+ devn = NULL; >+ continue; >+ } >+ >+ p -= len; >+ >+ if (strncmp(path, p, len)) { >+ devn = NULL; >+ continue; >+ } >+ >+ devn = devn->parent; >+ if (p == *try) >+ return count; >+ >+ if (devn != NULL && (p - *try) < 2) >+ devn = NULL; >+ >+ p--; >+ if (devn != NULL && *p != '/') >+ devn = NULL; >+ } >+ >+ try++; >+ } >+ >+ return -ENOENT; >+} >+ >+static int omap_panda_netdev_event(struct notifier_block *this, >+ unsigned long >event, void *ptr) >+{ >+ struct net_device *dev = ptr; >+ struct sockaddr sa; >+ int n; >+ >+ if (event != NETDEV_REGISTER) >+ return NOTIFY_DONE; >+ >+ n = panda_device_path_need_mac(dev->dev.parent); >+ if (n >= 0) { >+ sa.sa_family = dev->type; >+ omap2_die_id_to_ethernet_mac(sa.sa_data, n); >+ dev->netdev_ops->ndo_set_mac_address(dev, &sa); >+ } >+ >+ return NOTIFY_DONE; >+} >+ >+static struct notifier_block omap_panda_netdev_notifier = { >+ .notifier_call = omap_panda_netdev_event, >+ .priority = 1, >+}; >+ >+ One extra blank line not needed. Regards, Hema > static void __init omap4_panda_init(void) > { > int package = OMAP_PACKAGE_CBS; >@@ -517,6 +606,8 @@ static void __init omap4_panda_init(void) > if (wl12xx_set_platform_data(&omap_panda_wlan_data)) > pr_err("error setting wl12xx data\n"); > >+ register_netdevice_notifier(&omap_panda_netdev_notifier); >+ > omap4_panda_i2c_init(); > platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices)); > platform_device_register(&omap_vwlan_device); > >-- >To unsubscribe from this list: send the line "unsubscribe >linux-omap" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thursday 24 March 2011, Andy Green wrote: > This patch registers a network device notifier callback to set the mac > addresses for the onboard network assets of Panda correctly, despite the > drivers involved have used a random or all-zeros MAC address. > > The technique was suggested by Alan Cox on lkml. > > It works by device path so it corrects the MAC addresses even if the > drivers are in modules loaded in an order that changes their interface > name from usual (eg, the onboard module might be "wlan1" if there is a > USB wireless stick plugged in and its module is inserted first.) > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > Signed-off-by: Andy Green <andy.green@linaro.org> It looks like this is the best solution so far. Acked-by: Arnd Bergmann <arnd@arndb.de>
On Fri, Mar 25, 2011 at 3:39 AM, Hema Kalliguddi <hemahk@ti.com> wrote: > Hi, > > one Minor comment. > >>-----Original Message----- >>From: linux-omap-owner@vger.kernel.org >>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Andy Green >>Sent: Friday, March 25, 2011 2:58 AM >>To: linux-arm-kernel@lists.infradead.org; linux-omap@vger.kernel.org >>Cc: patches@linaro.org; nicolas.pitre@linaro.org; >>arnd@arndb.de; x0132446@ti.com; s-jan@ti.com; >>tony@atomide.com; Alan Cox; Andy Green >>Subject: [RFC PATCH 2/2] OMAP2+: PANDA: Fix up random or >>missing MAC addresses for eth0 and wlan0 >> >>This patch registers a network device notifier callback to set the mac >>addresses for the onboard network assets of Panda correctly, >>despite the >>drivers involved have used a random or all-zeros MAC address. >> >>The technique was suggested by Alan Cox on lkml. >> >>It works by device path so it corrects the MAC addresses even if the >>drivers are in modules loaded in an order that changes their interface >>name from usual (eg, the onboard module might be "wlan1" if there is a >>USB wireless stick plugged in and its module is inserted first.) >> >>Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> >>Signed-off-by: Andy Green <andy.green@linaro.org> I very much like this approach. I believed the ability to use the die ID to get a unique code was reasonable approach and that is why I didn't get an EEPROM put onto the BeagleBoard, though Gerald is looking at adding one on a future revision because the lack of one wasn't well received. Minor questions below. >>--- >> >> arch/arm/mach-omap2/board-omap4panda.c | 91 >>++++++++++++++++++++++++++++++++ >> 1 files changed, 91 insertions(+), 0 deletions(-) >> >>diff --git a/arch/arm/mach-omap2/board-omap4panda.c >>b/arch/arm/mach-omap2/board-omap4panda.c >>index 80b8860..0b92873 100644 >>--- a/arch/arm/mach-omap2/board-omap4panda.c >>+++ b/arch/arm/mach-omap2/board-omap4panda.c >>@@ -28,9 +28,12 @@ >> #include <linux/regulator/machine.h> >> #include <linux/regulator/fixed.h> >> #include <linux/wl12xx.h> >>+#include <linux/netdevice.h> >>+#include <linux/if_ether.h> >> >> #include <mach/hardware.h> >> #include <mach/omap4-common.h> >>+#include <mach/id.h> >> #include <asm/mach-types.h> >> #include <asm/mach/arch.h> >> #include <asm/mach/map.h> >>@@ -506,6 +509,92 @@ static inline void board_serial_init(void) >> } >> #endif >> >>+/* >>+ * These device paths represent the onboard USB <-> Ethernet >>bridge, and >>+ * the WLAN module on Panda, both of which need their random >>or all-zeros >>+ * mac address replacing with a per-cpu stable generated one >>+ */ >>+ >>+static const char * const panda_fixup_mac_device_paths[] = { >>+ "usb1/1-1/1-1.1/1-1.1:1.0", >>+ "mmc1:0001:2", >>+}; >>+ >>+static int panda_device_path_need_mac(struct device *dev) >>+{ >>+ const char **try = panda_fixup_mac_device_paths; >>+ const char *path; >>+ int count = ARRAY_SIZE(panda_fixup_mac_device_paths); >>+ const char *p; >>+ int len; >>+ struct device *devn; >>+ >>+ while (count--) { >>+ >>+ p = *try + strlen(*try); >>+ devn = dev; >>+ >>+ while (devn) { >>+ >>+ path = dev_name(devn); >>+ len = strlen(path); >>+ >>+ if ((p - *try) < len) { >>+ devn = NULL; >>+ continue; >>+ } >>+ >>+ p -= len; >>+ >>+ if (strncmp(path, p, len)) { >>+ devn = NULL; >>+ continue; >>+ } >>+ >>+ devn = devn->parent; >>+ if (p == *try) >>+ return count; >>+ >>+ if (devn != NULL && (p - *try) < 2) >>+ devn = NULL; >>+ >>+ p--; >>+ if (devn != NULL && *p != '/') >>+ devn = NULL; >>+ } >>+ >>+ try++; >>+ } >>+ >>+ return -ENOENT; >>+} >>+ >>+static int omap_panda_netdev_event(struct notifier_block *this, >>+ unsigned long The use of the OMAP die id below makes this OMAP specific and the list referenced below of the devices to be referenced makes it Panda specific. Is there a way to make the list board specific, but to make these functions that will be used across many OMAP platforms reusable? I believe that this current code will result in a lot of cut-and-paste. My preference is that this is accepted and that we make this more general when we add this to other OMAP platforms, but it'd be great to capture your suggestions on how to do so before those cut-and-paste patch sets start coming in. >>event, void *ptr) >>+{ >>+ struct net_device *dev = ptr; >>+ struct sockaddr sa; >>+ int n; >>+ >>+ if (event != NETDEV_REGISTER) >>+ return NOTIFY_DONE; >>+ >>+ n = panda_device_path_need_mac(dev->dev.parent); >>+ if (n >= 0) { >>+ sa.sa_family = dev->type; >>+ omap2_die_id_to_ethernet_mac(sa.sa_data, n); >>+ dev->netdev_ops->ndo_set_mac_address(dev, &sa); >>+ } >>+ >>+ return NOTIFY_DONE; >>+} >>+ >>+static struct notifier_block omap_panda_netdev_notifier = { >>+ .notifier_call = omap_panda_netdev_event, >>+ .priority = 1, >>+}; >>+ >>+ > > One extra blank line not needed. > > Regards, > Hema > >> static void __init omap4_panda_init(void) >> { >> int package = OMAP_PACKAGE_CBS; >>@@ -517,6 +606,8 @@ static void __init omap4_panda_init(void) >> if (wl12xx_set_platform_data(&omap_panda_wlan_data)) >> pr_err("error setting wl12xx data\n"); >> >>+ register_netdevice_notifier(&omap_panda_netdev_notifier); >>+ I just want to make sure I understand how this works. When a new network device is added, if the device name matches one of the above listed device paths, then the die id based MAC id is applied. This must be done via a device registration notifier as the registration is triggered when the device is detected. >> omap4_panda_i2c_init(); >> platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices)); >> platform_device_register(&omap_vwlan_device); >> >>-- >>To unsubscribe from this list: send the line "unsubscribe >>linux-omap" in >>the body of a message to majordomo@vger.kernel.org >>More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Friday 25 March 2011 21:13:05 Jason Kridner wrote: > The use of the OMAP die id below makes this OMAP specific and the list > referenced below of the devices to be referenced makes it Panda > specific. Is there a way to make the list board specific, but to make > these functions that will be used across many OMAP platforms reusable? > I believe that this current code will result in a lot of > cut-and-paste. My preference is that this is accepted and that we > make this more general when we add this to other OMAP platforms, but > it'd be great to capture your suggestions on how to do so before those > cut-and-paste patch sets start coming in. Do you know of other existing boards without the EEPROM? If we need the code to be more general, it will simply have to move out of the panda specific board file into one file that can be selected for compilation by other boards. > >> static void __init omap4_panda_init(void) > >> { > >> int package = OMAP_PACKAGE_CBS; > >>@@ -517,6 +606,8 @@ static void __init omap4_panda_init(void) > >> if (wl12xx_set_platform_data(&omap_panda_wlan_data)) > >> pr_err("error setting wl12xx data\n"); > >> > >>+ register_netdevice_notifier(&omap_panda_netdev_notifier); > >>+ > > I just want to make sure I understand how this works. When a new > network device is added, if the device name matches one of the above > listed device paths, then the die id based MAC id is applied. This > must be done via a device registration notifier as the registration is > triggered when the device is detected. Correct. Arnd
On Fri, 25 Mar 2011, Jason Kridner wrote: > I very much like this approach. I believed the ability to use the die > ID to get a unique code was reasonable approach and that is why I > didn't get an EEPROM put onto the BeagleBoard, though Gerald is > looking at adding one on a future revision because the lack of one > wasn't well received. Minor questions below. If this code had been available and/or the procedure well documented before then I believe the reception would have been better. > The use of the OMAP die id below makes this OMAP specific and the list > referenced below of the devices to be referenced makes it Panda > specific. Is there a way to make the list board specific, but to make > these functions that will be used across many OMAP platforms reusable? > I believe that this current code will result in a lot of > cut-and-paste. My preference is that this is accepted and that we > make this more general when we add this to other OMAP platforms, but > it'd be great to capture your suggestions on how to do so before those > cut-and-paste patch sets start coming in. It is true that this might get copied. But as I suggested to Andy, it is best to wait and see how often this happens before generalizing the approach. Consolidation is easier when you can see what is actually common and what is board specific. Otherwise it is easy to fall into the over-engineering trap. Nicolas
On 03/25/2011 08:13 PM, Somebody in the thread at some point said: Hi - > I very much like this approach. I believed the ability to use the die > ID to get a unique code was reasonable approach and that is why I > didn't get an EEPROM put onto the BeagleBoard, though Gerald is > looking at adding one on a future revision because the lack of one > wasn't well received. Minor questions below. Great. FWIW I think it'd be a lost opportunity to wire the EEPROM direct to the network device. It's more flexible and powerful to regard the EEPROM as general "board identity storage", a way to bind information to the physical board. Then you can stick any kind of information that you need to bind to the board in the same 25c device and in-kernel code can take care of discovering that data when needed on any subsystem that takes an interest. > The use of the OMAP die id below makes this OMAP specific and the list > referenced below of the devices to be referenced makes it Panda > specific. Is there a way to make the list board specific, but to make > these functions that will be used across many OMAP platforms reusable? > I believe that this current code will result in a lot of > cut-and-paste. My preference is that this is accepted and that we > make this more general when we add this to other OMAP platforms, but > it'd be great to capture your suggestions on how to do so before those > cut-and-paste patch sets start coming in. Sure, I would be happy to put this stuff at OMAP platform layer for example if it makes sense to OMAP guys more generally. > I just want to make sure I understand how this works. When a new > network device is added, if the device name matches one of the above > listed device paths, then the die id based MAC id is applied. This > must be done via a device registration notifier as the registration is > triggered when the device is detected. That's right. Arguably it would be better if there was a core API to register your board-specific uniqueness / entropy, and the drivers were able to use that instead of "random" ethernet address all in network layer. But after wasting two weeks getting pointlessly beaten up on lkml largely on the question of how generic this issue is, I would rather restart somewhere specific where everyone can see the obvious benefit and if it's seen as more useful migrate it. -Andy
On Fri, Mar 25, 2011 at 4:23 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Fri, 25 Mar 2011, Jason Kridner wrote: > >> I very much like this approach. I believed the ability to use the die >> ID to get a unique code was reasonable approach and that is why I >> didn't get an EEPROM put onto the BeagleBoard, though Gerald is >> looking at adding one on a future revision because the lack of one >> wasn't well received. Minor questions below. > > If this code had been available and/or the procedure well documented > before then I believe the reception would have been better. Understood. Live and learn and try not to repeat the same mistakes. Hopefully others pick up on this one. > >> The use of the OMAP die id below makes this OMAP specific and the list >> referenced below of the devices to be referenced makes it Panda >> specific. Is there a way to make the list board specific, but to make >> these functions that will be used across many OMAP platforms reusable? >> I believe that this current code will result in a lot of >> cut-and-paste. My preference is that this is accepted and that we >> make this more general when we add this to other OMAP platforms, but >> it'd be great to capture your suggestions on how to do so before those >> cut-and-paste patch sets start coming in. > > It is true that this might get copied. But as I suggested to Andy, it > is best to wait and see how often this happens before generalizing the > approach. Consolidation is easier when you can see what is actually > common and what is board specific. Otherwise it is easy to > fall into the over-engineering trap. Makes sense. I hope to see this patch accepted for Panda quickly and we can follow Andy's advice on how to make it more general in the future as we wee how people use/need it. > > > Nicolas >
diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c index 80b8860..0b92873 100644 --- a/arch/arm/mach-omap2/board-omap4panda.c +++ b/arch/arm/mach-omap2/board-omap4panda.c @@ -28,9 +28,12 @@ #include <linux/regulator/machine.h> #include <linux/regulator/fixed.h> #include <linux/wl12xx.h> +#include <linux/netdevice.h> +#include <linux/if_ether.h> #include <mach/hardware.h> #include <mach/omap4-common.h> +#include <mach/id.h> #include <asm/mach-types.h> #include <asm/mach/arch.h> #include <asm/mach/map.h> @@ -506,6 +509,92 @@ static inline void board_serial_init(void) } #endif +/* + * These device paths represent the onboard USB <-> Ethernet bridge, and + * the WLAN module on Panda, both of which need their random or all-zeros + * mac address replacing with a per-cpu stable generated one + */ + +static const char * const panda_fixup_mac_device_paths[] = { + "usb1/1-1/1-1.1/1-1.1:1.0", + "mmc1:0001:2", +}; + +static int panda_device_path_need_mac(struct device *dev) +{ + const char **try = panda_fixup_mac_device_paths; + const char *path; + int count = ARRAY_SIZE(panda_fixup_mac_device_paths); + const char *p; + int len; + struct device *devn; + + while (count--) { + + p = *try + strlen(*try); + devn = dev; + + while (devn) { + + path = dev_name(devn); + len = strlen(path); + + if ((p - *try) < len) { + devn = NULL; + continue; + } + + p -= len; + + if (strncmp(path, p, len)) { + devn = NULL; + continue; + } + + devn = devn->parent; + if (p == *try) + return count; + + if (devn != NULL && (p - *try) < 2) + devn = NULL; + + p--; + if (devn != NULL && *p != '/') + devn = NULL; + } + + try++; + } + + return -ENOENT; +} + +static int omap_panda_netdev_event(struct notifier_block *this, + unsigned long event, void *ptr) +{ + struct net_device *dev = ptr; + struct sockaddr sa; + int n; + + if (event != NETDEV_REGISTER) + return NOTIFY_DONE; + + n = panda_device_path_need_mac(dev->dev.parent); + if (n >= 0) { + sa.sa_family = dev->type; + omap2_die_id_to_ethernet_mac(sa.sa_data, n); + dev->netdev_ops->ndo_set_mac_address(dev, &sa); + } + + return NOTIFY_DONE; +} + +static struct notifier_block omap_panda_netdev_notifier = { + .notifier_call = omap_panda_netdev_event, + .priority = 1, +}; + + static void __init omap4_panda_init(void) { int package = OMAP_PACKAGE_CBS; @@ -517,6 +606,8 @@ static void __init omap4_panda_init(void) if (wl12xx_set_platform_data(&omap_panda_wlan_data)) pr_err("error setting wl12xx data\n"); + register_netdevice_notifier(&omap_panda_netdev_notifier); + omap4_panda_i2c_init(); platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices)); platform_device_register(&omap_vwlan_device);
This patch registers a network device notifier callback to set the mac addresses for the onboard network assets of Panda correctly, despite the drivers involved have used a random or all-zeros MAC address. The technique was suggested by Alan Cox on lkml. It works by device path so it corrects the MAC addresses even if the drivers are in modules loaded in an order that changes their interface name from usual (eg, the onboard module might be "wlan1" if there is a USB wireless stick plugged in and its module is inserted first.) Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> Signed-off-by: Andy Green <andy.green@linaro.org> --- arch/arm/mach-omap2/board-omap4panda.c | 91 ++++++++++++++++++++++++++++++++ 1 files changed, 91 insertions(+), 0 deletions(-)