Re: [PATCH 05/14] Phonet: network device and address handling

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Tue, Sep 16, 2008 at 06:08:05PM +0300, Rémi Denis-Courmont escreveu:
> This provides support for adding Phonet addresses to and removing
> Phonet addresses from network devices.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> ---
>  include/net/phonet/pn_dev.h |   63 ++++++++++++
>  net/phonet/Makefile         |    1 +
>  net/phonet/af_phonet.c      |   12 ++
>  net/phonet/pn_dev.c         |  233 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 309 insertions(+), 0 deletions(-)
>  create mode 100644 include/net/phonet/pn_dev.h
>  create mode 100644 net/phonet/pn_dev.c
> 
> diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
> new file mode 100644
> index 0000000..c53d364
> --- /dev/null
> +++ b/include/net/phonet/pn_dev.h
> @@ -0,0 +1,63 @@
> +/*
> + * File: pn_dev.h
> + *
> + * Phonet network device
> + *
> + * Copyright (C) 2008 Nokia Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef PN_DEV_H
> +#define PN_DEV_H
> +
> +struct phonet_device_list {
> +	struct list_head list;
> +	spinlock_t lock;
> +};
> +
> +extern struct phonet_device_list pndevs;
> +
> +struct phonet_device {
> +	struct list_head list;
> +	struct net_device *netdev;
> +	struct list_head own;
> +};
> +
> +struct phonet_address {
> +	struct list_head list;
> +	struct phonet_device *pnd;
> +	uint8_t addr;
> +	unsigned char prefix;
> +};
> +
> +int phonet_device_init(void);
> +void phonet_device_exit(void);
> +struct phonet_device *phonet_device_alloc(struct net_device *dev);
> +struct phonet_device *__phonet_get_by_index(unsigned ifindex);
> +void phonet_device_free(struct phonet_device *pnd);
> +
> +struct phonet_address *phonet_address_alloc(void);
> +void phonet_address_add(struct list_head *list, struct phonet_address *pna);
> +void phonet_address_free(struct phonet_address *pna);
> +struct phonet_address *phonet_addr2addr(uint8_t addr, int mode);
> +struct phonet_address *phonet_dev2addr(struct phonet_device *pnd, uint8_t addr,
> +					int mode);
> +
> +#define PN_FIND_EXACT	(1 << 0)
> +
> +#define MKMASK(a)	((1 << (a)) - 1)
> +
> +#endif
> diff --git a/net/phonet/Makefile b/net/phonet/Makefile
> index 5dbff68..980a386 100644
> --- a/net/phonet/Makefile
> +++ b/net/phonet/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_PHONET) += phonet.o
>  
>  phonet-objs := \
> +	pn_dev.o \
>  	af_phonet.o
> diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
> index ed66a81..744b47f 100644
> --- a/net/phonet/af_phonet.c
> +++ b/net/phonet/af_phonet.c
> @@ -31,6 +31,7 @@
>  #include <linux/if_phonet.h>
>  #include <linux/phonet.h>
>  #include <net/phonet/phonet.h>
> +#include <net/phonet/pn_dev.h>
>  
>  static struct net_proto_family phonet_proto_family;
>  static struct phonet_protocol *phonet_proto_get(int protocol);
> @@ -202,14 +203,25 @@ static int __init phonet_init(void)
>  		return err;
>  	}
>  
> +	err = phonet_device_init();
> +	if (err) {
> +		printk(KERN_ALERT "phonet devices initialization failed\n");
> +		goto unregister;
> +	}
> +
>  	dev_add_pack(&phonet_packet_type);
>  	return 0;
> +
> +unregister:
> +	sock_unregister(AF_PHONET);
> +	return err;
>  }
>  
>  static void __exit phonet_exit(void)
>  {
>  	sock_unregister(AF_PHONET);
>  	dev_remove_pack(&phonet_packet_type);
> +	phonet_device_exit();
>  }
>  
>  module_init(phonet_init);
> diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
> new file mode 100644
> index 0000000..cae37a6
> --- /dev/null
> +++ b/net/phonet/pn_dev.c
> @@ -0,0 +1,233 @@
> +/*
> + * File: pn_dev.c
> + *
> + * Phonet network device
> + *
> + * Copyright (C) 2008 Nokia Corporation.
> + *
> + * Contact: Remi Denis-Courmont <remi.denis-courmont@nokia.com>
> + * Original author: Sakari Ailus <sakari.ailus@nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/net.h>
> +#include <linux/netdevice.h>
> +#include <linux/phonet.h>
> +#include <net/sock.h>
> +#include <net/phonet/pn_dev.h>
> +
> +static int phonet_device_notify(struct notifier_block *self, unsigned long what,
> +				void *dev);
> +
> +/* when accessing, remember to lock with spin_lock(&pndevs.lock); */
> +struct phonet_device_list pndevs;

Please look at net/ipv4/tcp_minisocks.c to see how tcp_death_row is
initialized, that way you don't have to first initialize it to zeros
(BSS as it is static and uninitialized) and then, at phonet_device_init
time, again.

> +static struct notifier_block phonet_device_notifier = {
> +	.notifier_call = &phonet_device_notify,
> +	.priority = 0,
> +};
> +
> +/* initialise Phonet device list */
> +int phonet_device_init(void)
> +{
> +	INIT_LIST_HEAD(&pndevs.list);
> +	spin_lock_init(&pndevs.lock);
> +	register_netdevice_notifier(&phonet_device_notifier);
> +
> +	return 0;
> +}
> +
> +void phonet_device_exit(void)
> +{
> +	struct list_head *ptr;
> +
> +	rtnl_unregister_all(PF_PHONET);
> +	rtnl_lock();
> +	spin_lock_bh(&pndevs.lock);
> +
> +	for (ptr = pndevs.list.next; ptr != &pndevs.list;
> +		ptr = pndevs.list.next) {

Perhaps a phone_for_each_dev() macro like for_each_netdev() in
linux/netdevice.h?

> +		struct phonet_device *pnd = list_entry(ptr,
> +						struct phonet_device, list);
> +		phonet_device_free(pnd);
> +	}
> +
> +	spin_unlock_bh(&pndevs.lock);
> +	rtnl_unlock();
> +	unregister_netdevice_notifier(&phonet_device_notifier);
> +}
> +
> +struct phonet_address *phonet_address_alloc(void)
> +{
> +	struct phonet_address *pna;
> +
> +	pna = kmalloc(sizeof(struct phonet_address), GFP_KERNEL);
> +
> +	if (pna == NULL)
> +		return NULL;
> +
> +	INIT_LIST_HEAD(&pna->list);


minor nitpick:

	if (pna != NULL)
		INIT_LIST_HEAD(&pna->list);

shorter :)

> +
> +	return pna;
> +}
> +
> +/* add Phonet address to list */
> +void phonet_address_add(struct list_head *list, struct phonet_address *pna)
> +{
> +	list_add(&pna->list, list);
> +}
> +
> +/* remove Phonet address from list and free it */
> +void phonet_address_free(struct phonet_address *pna)
> +{
> +	list_del(&pna->list);
> +	kfree(pna);
> +}

Not symetric, i.e. you have a list_add operation and not a list_del,
only one that in addition to removing from the list also destroys the
address, confusing.

> +/* Allocate new Phonet device. */
> +struct phonet_device *phonet_device_alloc(struct net_device *dev)
> +{
> +	struct phonet_device *pnd;
> +
> +	pnd = kmalloc(sizeof(struct phonet_device), GFP_ATOMIC);
> +
> +	if (pnd == NULL)
> +		return NULL;
> +
> +	INIT_LIST_HEAD(&pnd->own);
> +	pnd->netdev = dev;
> +	list_add(&pnd->list, &pndevs.list);
> +	return pnd;
> +}
> +
> +struct phonet_device *__phonet_get_by_index(unsigned ifindex)
> +{
> +	struct list_head *p;
> +
> +	list_for_each(p, &pndevs.list) {
> +		struct phonet_device *pnd;
> +
> +		pnd = list_entry(p, struct phonet_device, list);
> +		BUG_ON(!pnd->netdev);
> +
> +		if (pnd->netdev->ifindex == ifindex)
> +			return pnd;
> +	}
> +	return NULL;
> +}
> +
> +void phonet_device_free(struct phonet_device *pnd)
> +{
> +	struct list_head *ptr;
> +
> +	/* remove device from list */
> +	list_del(&pnd->list);
> +
> +	for (ptr = pnd->own.next; ptr != &pnd->own; ptr = pnd->own.next) {
> +		struct phonet_address *pna =
> +			list_entry(ptr, struct phonet_address, list);
> +		phonet_address_free(pna);

Why not use list_for_each_entry()? _safe() even, since
phonet_address_free also messes with this list, no?

> +	}
> +
> +	kfree(pnd);
> +}
> +
> +/*
> + * Find the address from the device's address list having the given address
> + */
> +struct phonet_address *phonet_dev2addr(struct phonet_device *pnd,
> +					uint8_t addr, int mode)
> +{
> +	struct list_head *list = &pnd->own, *ptr;
> +	struct phonet_address *best = NULL;
> +
> +	for (ptr = list->next; ptr != list; ptr = ptr->next) {
> +		struct phonet_address *pna =
> +			list_entry(ptr, struct phonet_address, list);

list_for_each_entry()

> +		if (mode & PN_FIND_EXACT) {
> +			/* try to find exact address */
> +			if (pna->addr == addr) {
> +				best = pna;
> +				break;
> +			}
> +		} else if (best == NULL ||
> +			   best->prefix < pna->prefix) {
> +			if ((pna->addr & MKMASK(pna->prefix)) ==
> +					(addr & MKMASK(pna->prefix))) {
> +				best = pna;
> +			}
> +		}
> +	}
> +
> +	return best;
> +}
> +
> +/*
> + * Find the device having a given address (remember locking!!)
> + */
> +struct phonet_address *phonet_addr2addr(uint8_t addr, int mode)
> +{
> +	struct list_head *ptr;
> +	struct phonet_address *best = NULL;
> +
> +	for (ptr = pndevs.list.next; ptr != &pndevs.list; ptr = ptr->next) {
> +		struct phonet_address *pna = NULL;
> +		struct phonet_device *pnd =
> +		    list_entry(ptr, struct phonet_device, list);

Ditto

> +
> +		/* Don't allow unregistering devices! */
> +		if ((pnd->netdev->reg_state != NETREG_REGISTERED) ||
> +			((pnd->netdev->flags & IFF_UP)) != IFF_UP)
> +			continue;
> +
> +		pna = phonet_dev2addr(pnd, addr, mode);
> +
> +		if (pna != NULL) {
> +			if (best == NULL || best->prefix < pna->prefix)
> +				best = pna;
> +		}
> +	}
> +
> +	return best;
> +}
> +
> +/* notify Phonet of device events */
> +static int phonet_device_notify(struct notifier_block *self, unsigned long what,
> +				void *_dev)
> +{
> +	struct net_device *dev = _dev;
> +	struct phonet_device *pnd;
> +
> +	spin_lock_bh(&pndevs.lock);
> +	pnd = __phonet_get_by_index(dev->ifindex);
> +
> +	switch (what) {
> +	case NETDEV_UNREGISTER:
> +		if (pnd == NULL)
> +			break;
> +		phonet_device_free(pnd);
> +		break;
> +	default:
> +		break;
> +	}

	if (what == NETDEV_UNREGISTER && pnd != NULL)
		phonet_device_free(pnd);

Shorter, no? :)

> +
> +	spin_unlock_bh(&pndevs.lock);
> +
> +	return 0;
> +
> +}
> -- 
> 1.5.4.3
> 
> --
> 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