| Message ID | 20120702064054.26782.79849.stgit@build.warmcat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 02 July 2012, Andy Green wrote: > From: Andy Green <andy@warmcat.com> > > This introduces a small helper in net/ethernet, which registers a > network notifier on init, and accepts registrations of expected asynchronously- > probed network device paths (like, "usb1/1-1/1-1.1/1-1.1:1.0") and the MAC > that is needed to be assigned to the device when it appears. > > This allows platform code to enforce valid, consistent MAC addresses on to > devices that have not been probed at boot-time, but due to being wired on the > board are always present at the same interface. It has been tested with USB > and SDIO probed devices. > > To make use of this safely you also need to make sure that any drivers that > may compete for the bus ordinal you are using (eg, mUSB and ehci in Panda > case) are loaded in a deterministic order. > > At registration it makes a copy of the incoming data, so the data may be > __initdata or otherwise transitory. Registration can be called multiple times > so registrations from Device Tree and platform may be mixed. > > Since it needs to be called quite early in boot and there is no lifecycle for > what it does, it could not be modular and is not a driver. > > Via suggestions from Arnd Bergmann and Tony Lindgren. > > Signed-off-by: Andy Green <andy.green@linaro.org> Yes, looks good to me in principle. It needs to get sent to the linux-kernel and netdev mailing lists for review though. A few small comments. > include/net/mac-la-ap.h | 28 ++++++++ > net/Kconfig | 14 ++++ > net/ethernet/Makefile | 2 + > net/ethernet/mac-la-ap.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 209 insertions(+) > create mode 100644 include/net/mac-la-ap.h > create mode 100644 net/ethernet/mac-la-ap.c > diff --git a/include/net/mac-la-ap.h b/include/net/mac-la-ap.h > new file mode 100644 > index 0000000..d5189b5 > --- /dev/null > +++ b/include/net/mac-la-ap.h > @@ -0,0 +1,28 @@ > +/* > + * mac-la-ap.h: Locally Administered MAC address for Async probed devices > + */ I find the name a bit non-obvious, not sure if we can come up with the perfect one. How about mac-platform? > +/** > + * mac_la_ap_register_device_macs - add an array of device path to monitor > + * and MAC to apply when the network device > + * at the device path appears > + * @macs: array of struct mac_la_ap terminated by entry with NULL device_path > + */ > +int mac_la_ap_register_device_macs(const struct mac_la_ap *macs); > + > diff --git a/net/Kconfig b/net/Kconfig > index 245831b..76ba70e 100644 > --- a/net/Kconfig > +++ b/net/Kconfig > @@ -335,6 +335,20 @@ source "net/caif/Kconfig" > source "net/ceph/Kconfig" > source "net/nfc/Kconfig" > > +config MAC_LA_AP > + bool "Locally Administered MAC for Aysnchronously Probed devices" > + ---help--- > + This helper allows platforms to mandate a locally-administered > + sythesized MAC address for network devices that are on asynchronously- > + probed buses like USB or SDIO. This is necessary when the board has > + these network assets but no arrangements for storing or setting the > + MAC address of the network asset (nor any official MAC address > + reserved for the device). In that case, seen in Panda and other > + boards, the platform can synthesize a constant locally-administered > + MAC address from SoC UID bits that has a good probability of differing > + between boards but will be constant on any give board. This helper > + will take care of watching for the network devices to appear and > + force the MAC to the synthesized one when they do. > > endif # if NET This one needs to be either a silent option (only selected by the platforms that want it) or the header file has to provide a static-inline fallback that does nothing, so we don't get a build failure on platforms that need it if the option gets disabled. I'd prefer making it a silent option because then we don't bloat the kernel if someone accidentally enables it on a platform that does not use it. > +static struct mac_la_ap mac_la_ap_list; The normal way to do this is to have just a list head that the entries get added to: static LIST_HEAD(mac_la_ap_list); That also takes care of the initialization. > +DEFINE_MUTEX(mac_la_ap_mutex); > +bool mac_la_ap_started; These should all be static. > +static int mac_la_ap_init(void) > +{ > + int ret; > + > + INIT_LIST_HEAD(&mac_la_ap_list.list); > + mutex_init(&mac_la_ap_mutex); > + ret = register_netdevice_notifier(&mac_la_ap_netdev_notifier); > + if (!ret) > + mac_la_ap_started = 1; > + else > + pr_err("mac_la_ap_init: Notifier registration failed\n"); > + > + return ret; > +} The mutex is already initialized, and the list head too if you do the change above. I think it would be simpler to register the notifier from an initcall and drop the mac_la_ap_started variable. > +int mac_la_ap_register_device_macs(const struct mac_la_ap *macs) > +{ > +} > +EXPORT_SYMBOL_GPL(mac_la_ap_register_device_macs); I'm not sure if there is any point in exporting this function, my feeling is that it would only ever be called from built-in code. If we can call it from a module, we should probably also allow this file to be a loadable module as well. Arnd
On Mon, 2012-07-02 at 16:12 +0000, Arnd Bergmann wrote: > > include/net/mac-la-ap.h | 28 ++++++++ > > net/Kconfig | 14 ++++ > > net/ethernet/Makefile | 2 + > > net/ethernet/mac-la-ap.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 209 insertions(+) > > create mode 100644 include/net/mac-la-ap.h > > create mode 100644 net/ethernet/mac-la-ap.c > > diff --git a/include/net/mac-la-ap.h b/include/net/mac-la-ap.h > > new file mode 100644 > > index 0000000..d5189b5 > > --- /dev/null > > +++ b/include/net/mac-la-ap.h > > @@ -0,0 +1,28 @@ > > +/* > > + * mac-la-ap.h: Locally Administered MAC address for Async probed devices > > + */ > > I find the name a bit non-obvious, not sure if we can come up with the > perfect one. > > How about mac-platform? I really find it unfortunate that 'mac' for ethernet has the same name as the Apple box as well. 'mac' in other contexts can usually differentiate these two, but saying 'mac-platform' just screams of forbidden fruit. mac-probe.h ? -- Steve
On 07/03/12 00:12, the mail apparently from Arnd Bergmann included: > On Monday 02 July 2012, Andy Green wrote: >> From: Andy Green <andy@warmcat.com> >> >> This introduces a small helper in net/ethernet, which registers a >> network notifier on init, and accepts registrations of expected asynchronously- > Yes, looks good to me in principle. It needs to get sent to the linux-kernel > and netdev mailing lists for review though. A few small comments. I wanted to hear that it had actually converged with what was being talked about first. I just sent out a v3 with the relevant patch also cc-d to those lists but stg mail didn't seem to send it to them. I'll wait for any further comments here and resend the whole thing including those lists. > I find the name a bit non-obvious, not sure if we can come up with the > perfect one. > > How about mac-platform? Done. >> endif # if NET > > This one needs to be either a silent option (only selected by the > platforms that want it) or the header file has to provide a > static-inline fallback that does nothing, so we don't get > a build failure on platforms that need it if the option gets > disabled. > > I'd prefer making it a silent option because then we don't bloat > the kernel if someone accidentally enables it on a platform that > does not use it. Done. It's already added as a select on the Panda board config. I also moved it out of the "if NET" clause and took care about the case that CONFIG_NET is off but we still have mac-platform references. >> +static struct mac_la_ap mac_la_ap_list; > > The normal way to do this is to have just a list head that the > entries get added to: > > static LIST_HEAD(mac_la_ap_list); > > That also takes care of the initialization. Thanks for normalizing it, done. >> +DEFINE_MUTEX(mac_la_ap_mutex); >> +bool mac_la_ap_started; > > These should all be static. Right, done. > I think it would be simpler to register the notifier from an > initcall and drop the mac_la_ap_started variable. That was my first approach, to structure it as a real driver. I had tried a few likely-looking initcall levels but the init of the helper was coming after the init of the network code. I found this time that core_initcall works, so I used that, dropped the flag. But isn't it a bit tricky with these to know if the prerequisite you're trying to ensure is already initialized might not actually be at the same initcall level and you're working by accident? >> +int mac_la_ap_register_device_macs(const struct mac_la_ap *macs) >> +{ > >> +} >> +EXPORT_SYMBOL_GPL(mac_la_ap_register_device_macs); > > > I'm not sure if there is any point in exporting this function, my feeling > is that it would only ever be called from built-in code. If we can call it > from a module, we should probably also allow this file to be a loadable > module as well. Being a modular API for platform code to call doesn't make sense, I got rid of the export. -Andy
On Tuesday 03 July 2012, Andy Green wrote: > > I think it would be simpler to register the notifier from an > > initcall and drop the mac_la_ap_started variable. > > That was my first approach, to structure it as a real driver. I had > tried a few likely-looking initcall levels but the init of the helper > was coming after the init of the network code. > > I found this time that core_initcall works, so I used that, dropped the > flag. But isn't it a bit tricky with these to know if the prerequisite > you're trying to ensure is already initialized might not actually be at > the same initcall level and you're working by accident? Well, you would only need to ensure that it's called before any of the network devices are added, which should all happen very late in the boot cycle compared to the platform init code. I think core_initcall is fine here. Arnd
On Mon, 2012-07-02 at 12:35 -0400, Steven Rostedt wrote: > On Mon, 2012-07-02 at 16:12 +0000, Arnd Bergmann wrote: > > > > include/net/mac-la-ap.h | 28 ++++++++ > > > net/Kconfig | 14 ++++ > > > net/ethernet/Makefile | 2 + > > > net/ethernet/mac-la-ap.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 209 insertions(+) > > > create mode 100644 include/net/mac-la-ap.h > > > create mode 100644 net/ethernet/mac-la-ap.c > > > diff --git a/include/net/mac-la-ap.h b/include/net/mac-la-ap.h > > > new file mode 100644 > > > index 0000000..d5189b5 > > > --- /dev/null > > > +++ b/include/net/mac-la-ap.h > > > @@ -0,0 +1,28 @@ > > > +/* > > > + * mac-la-ap.h: Locally Administered MAC address for Async probed devices > > > + */ > > > > I find the name a bit non-obvious, not sure if we can come up with the > > perfect one. > > > > How about mac-platform? > > I really find it unfortunate that 'mac' for ethernet has the same name > as the Apple box as well. 'mac' in other contexts can usually > differentiate these two, but saying 'mac-platform' just screams of > forbidden fruit. > > mac-probe.h ? Maybe eth_mac_addr.h? underscore filenames outnumber dash filenames ~2:1 $ git ls-files "*_*" | while read file ; do basename $file; done | grep "_" | wc -l 11185 $ git ls-files "*-*" | while read file ; do basename $file; done | grep "-" | wc -l 6624
diff --git a/include/net/mac-la-ap.h b/include/net/mac-la-ap.h new file mode 100644 index 0000000..d5189b5 --- /dev/null +++ b/include/net/mac-la-ap.h @@ -0,0 +1,28 @@ +/* + * mac-la-ap.h: Locally Administered MAC address for Async probed devices + */ + +/** + * struct mac_la_ap - associates asynchronously probed device path with + * MAC address to be assigned to the device when it + * is created + * + * @device_path: device path name of network device + * @mac: MAC address to assign to network device matching device path + * @list: can be left uninitialized when passing from platform + */ + +struct mac_la_ap { + char *device_path; + u8 mac[6]; + struct list_head list; /* unused in platform data usage */ +}; + +/** + * mac_la_ap_register_device_macs - add an array of device path to monitor + * and MAC to apply when the network device + * at the device path appears + * @macs: array of struct mac_la_ap terminated by entry with NULL device_path + */ +int mac_la_ap_register_device_macs(const struct mac_la_ap *macs); + diff --git a/net/Kconfig b/net/Kconfig index 245831b..76ba70e 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -335,6 +335,20 @@ source "net/caif/Kconfig" source "net/ceph/Kconfig" source "net/nfc/Kconfig" +config MAC_LA_AP + bool "Locally Administered MAC for Aysnchronously Probed devices" + ---help--- + This helper allows platforms to mandate a locally-administered + sythesized MAC address for network devices that are on asynchronously- + probed buses like USB or SDIO. This is necessary when the board has + these network assets but no arrangements for storing or setting the + MAC address of the network asset (nor any official MAC address + reserved for the device). In that case, seen in Panda and other + boards, the platform can synthesize a constant locally-administered + MAC address from SoC UID bits that has a good probability of differing + between boards but will be constant on any give board. This helper + will take care of watching for the network devices to appear and + force the MAC to the synthesized one when they do. endif # if NET diff --git a/net/ethernet/Makefile b/net/ethernet/Makefile index 7cef1d8..94ee883 100644 --- a/net/ethernet/Makefile +++ b/net/ethernet/Makefile @@ -5,3 +5,5 @@ obj-y += eth.o obj-$(subst m,y,$(CONFIG_IPX)) += pe2.o obj-$(subst m,y,$(CONFIG_ATALK)) += pe2.o +obj-$(CONFIG_MAC_LA_AP) += mac-la-ap.o + diff --git a/net/ethernet/mac-la-ap.c b/net/ethernet/mac-la-ap.c new file mode 100644 index 0000000..4216c41 --- /dev/null +++ b/net/ethernet/mac-la-ap.c @@ -0,0 +1,165 @@ +/* + * Helper to allow setting locally-administered MAC addresses automatically on + * asynchronously probed devices, such as SDIO and USB based devices. + * + * Because the "device path" is used for matching, this is only useful for + * network assets physcally wired on the board, and also requires any + * different drivers that can compete for bus ordinals (eg mUSB vs ehci) to + * have fixed initialization ordering, eg, by having ehci in monolithic + * kernel + * + * Neither a driver nor a module as needs to be callable from machine file + * before the network devices are registered. + * + * (c) 2012 Andy Green <andy.green@linaro.org> + */ + +#include <linux/netdevice.h> +#include <linux/if_ether.h> +#include <net/mac-la-ap.h> + +static struct mac_la_ap mac_la_ap_list; +DEFINE_MUTEX(mac_la_ap_mutex); +bool mac_la_ap_started; + +static struct mac_la_ap *__mac_la_ap_check(struct device *dev) +{ + const char *path; + const char *p; + const char *try; + int len; + struct device *devn; + struct mac_la_ap *tmp; + struct list_head *pos; + + list_for_each(pos, &mac_la_ap_list.list) { + + tmp = list_entry(pos, struct mac_la_ap, list); + + try = tmp->device_path; + + 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 tmp; + + if (devn != NULL && (p - try) < 2) + devn = NULL; + + p--; + if (devn != NULL && *p != '/') + devn = NULL; + } + + try++; + } + + return NULL; +} + +static int mac_la_ap_netdev_event(struct notifier_block *this, + unsigned long event, void *ptr) +{ + struct net_device *dev = ptr; + struct sockaddr sa; + struct mac_la_ap *match; + + if (event != NETDEV_REGISTER) + return NOTIFY_DONE; + + mutex_lock(&mac_la_ap_mutex); + + match = __mac_la_ap_check(dev->dev.parent); + if (match == NULL) + goto bail; + + sa.sa_family = dev->type; + memcpy(sa.sa_data, match->mac, sizeof match->mac); + dev->netdev_ops->ndo_set_mac_address(dev, &sa); + +bail: + mutex_unlock(&mac_la_ap_mutex); + + return NOTIFY_DONE; +} + +static struct notifier_block mac_la_ap_netdev_notifier = { + .notifier_call = mac_la_ap_netdev_event, + .priority = 1, +}; + +static int mac_la_ap_init(void) +{ + int ret; + + INIT_LIST_HEAD(&mac_la_ap_list.list); + mutex_init(&mac_la_ap_mutex); + ret = register_netdevice_notifier(&mac_la_ap_netdev_notifier); + if (!ret) + mac_la_ap_started = 1; + else + pr_err("mac_la_ap_init: Notifier registration failed\n"); + + return ret; +} + +int mac_la_ap_register_device_macs(const struct mac_la_ap *macs) +{ + struct mac_la_ap *next; + int ret = 0; + + if (!mac_la_ap_started) { + ret = mac_la_ap_init(); + if (ret) + return ret; + } + + mutex_lock(&mac_la_ap_mutex); + + while (macs->device_path) { + + next = kmalloc(sizeof(struct mac_la_ap), GFP_KERNEL); + if (!next) { + ret = -ENOMEM; + goto bail; + } + + next->device_path = kmalloc(strlen(macs->device_path) + 1, + GFP_KERNEL); + if (!next->device_path) { + ret = -ENOMEM; + goto bail; + } + + strcpy(next->device_path, macs->device_path); + memcpy(next->mac, macs->mac, sizeof macs->mac); + list_add(&next->list, &mac_la_ap_list.list); + + macs++; + } + +bail: + mutex_unlock(&mac_la_ap_mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(mac_la_ap_register_device_macs);