[4,2/4] NET ethernet introduce mac_platform helper

From: Andy Green <andy@warmcat.com>

This introduces a small helper in net/ethernet, which registers a network
notifier at core_initcall time, and accepts registrations mapping 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.

Other parts of this series provide an OMAP API that computes a valid
locally administered MAC address from CPU ID bits that are unique for each
physical SoC, and register those against devices wired to the board.

This solves a longstanding problem in at least Panda case that there are no
reserved MACs for either onboard Ethernet nor onboard WLAN module, and without
this patch a randomized MAC is assigned to Ethernet and 00:00:00:00:00:00 or
0xdeadbeef is assigned as the WLAN MAC address.  The series provides sane,
constant locally-administered MAC addresses that have a high probability of

On Thu, 2012-07-05 at 10:44 +0800, Andy Green wrote:
> From: Andy Green <andy@warmcat.com>
> 
> This introduces a small helper in net/ethernet, which registers a network
> notifier at core_initcall time, and accepts registrations mapping 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.

The mac prefix is poor.  I think eth_mac is better.

[]

> diff --git a/net/ethernet/mac-platform.c b/net/ethernet/mac-platform.c
[]
> +static int mac_platform_netdev_event(struct notifier_block *this,
> +						unsigned long event, void *ptr)

alignment to parenthesis please.

[]

> +int mac_platform_register_device_macs(const struct mac_platform *macs)
> +{
[]
> +		next = kmalloc(sizeof(struct mac_platform), 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);

kmemdup and kstrdup()

> +		list_add(&next->list, &mac_platform_list);
> +
> +		macs++;
> +	}
> +
> +bail:
> +	mutex_unlock(&mac_platform_mutex);
> +
> +	return ret;
> +}

leaking memory on failures.

On 05/07/12 11:12, the mail apparently from Joe Perches included:

Thanks for the comments.

>> This introduces a small helper in net/ethernet, which registers a network
>> notifier at core_initcall time, and accepts registrations mapping 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.
>
> The mac prefix is poor.  I think eth_mac is better.

OK.

>> diff --git a/net/ethernet/mac-platform.c b/net/ethernet/mac-platform.c
> []
>> +static int mac_platform_netdev_event(struct notifier_block *this,
>> +						unsigned long event, void *ptr)
>
> alignment to parenthesis please.

OK.  Although different places in the kernel seem to have different 
expectations about that.

>> +int mac_platform_register_device_macs(const struct mac_platform *macs)
>> +{
> []
>> +		next = kmalloc(sizeof(struct mac_platform), 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);
>
> kmemdup and kstrdup()

OK

>> +		list_add(&next->list, &mac_platform_list);
>> +
>> +		macs++;
>> +	}
>> +
>> +bail:
>> +	mutex_unlock(&mac_platform_mutex);
>> +
>> +	return ret;
>> +}
>
> leaking memory on failures.

Right... I'll fix these and wait for more comments.

Thanks again for the review.

-Andy

On Thu, 2012-07-05 at 11:20 +0800, Andy Green wrote:
> On 05/07/12 11:12, the mail apparently from Joe Perches included:
[]
> >> diff --git a/net/ethernet/mac-platform.c b/net/ethernet/mac-platform.c
> > []
> >> +static int mac_platform_netdev_event(struct notifier_block *this,
> >> +						unsigned long event, void *ptr)
> >
> > alignment to parenthesis please.
> 
> OK.  Although different places in the kernel seem to have different 
> expectations about that.

net and drivers/net is pretty consistent.
Most of the exceptions are old code.
Some of those exceptions are being slowly updated too.

cheers, Joe

On Thu, 2012-07-05 at 10:44 +0800, Andy Green wrote:
[...]
> 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.
[...]

This seems very restrictive... would it be practical to also allow a
driver name as a path component?

[...]
> --- /dev/null
> +++ b/include/net/mac-platform.h
> @@ -0,0 +1,39 @@
> +/*
> + * mac-platform.h:  Enforces platform-defined MAC for Async probed devices
> + */
> +
> +#ifndef __NET_MAC_PLATFORM_H__
> +#define __NET_MAC_PLATFORM_H__
> +
> +#include <linux/if_ether.h>
> +
> +/**
> + * struct mac_platform - associates asynchronously probed device path with
> + *			 MAC address to be assigned to the device when it
> + *			 is created

A kernel-doc summary is strictly limited to one line.  The longer
explanation can go in a paragraph under the field descriptions.

> + * @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_platform {
> +	char *device_path;
> +	u8 mac[ETH_ALEN];
> +	struct list_head list; /* unused in platform data usage */
> +};
[...]
> --- /dev/null
> +++ b/net/ethernet/mac-platform.c
[...]
> +static struct mac_platform *__mac_platform_check(struct device *dev)
> +{
> +	const char *path;
> +	const char *p;
> +	const char *try;
> +	int len;
> +	struct device *devn;
> +	struct mac_platform *tmp;
> +	struct list_head *pos;
> +
> +	list_for_each(pos, &mac_platform_list) {
> +
> +		tmp = list_entry(pos, struct mac_platform, 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;
[...]

There are so many blank lines here, it's hard to see much code at once.

Ben.