On Monday 02 July 2012, Andy Green wrote:
> From: Andy Green <andy@xxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
