Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Thu, 26 Mar 2020 18:48:00 +0100 | Subject | Re: [PATCH] net: core: provide devm_register_netdev() |
| |
czw., 26 mar 2020 o 18:39 Heiner Kallweit <hkallweit1@gmail.com> napisał(a): > > On 26.03.2020 14:17, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Create a new source file for networking devres helpers and provide > > devm_register_netdev() - a managed variant of register_netdev(). > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > I'm writing a new ethernet driver and I realized there's no devres > > variant for register_netdev(). Since this is the only function I need > > to get rid of the remove() callback, I thought I'll just go ahead and > > add it and send it even before the driver to make it available to other > > drivers. > > > > Such a new functionality typically is accepted as part of series adding > at least one user only. Therefore best submit it together with the new > network driver. >
Sure, will do.
> > .../driver-api/driver-model/devres.rst | 3 ++ > > include/linux/netdevice.h | 1 + > > net/core/Makefile | 2 +- > > net/core/devres.c | 41 +++++++++++++++++++ > > 4 files changed, 46 insertions(+), 1 deletion(-) > > create mode 100644 net/core/devres.c > > > > diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst > > index 46c13780994c..11a03b65196e 100644 > > --- a/Documentation/driver-api/driver-model/devres.rst > > +++ b/Documentation/driver-api/driver-model/devres.rst > > @@ -372,6 +372,9 @@ MUX > > devm_mux_chip_register() > > devm_mux_control_get() > > > > +NET > > + devm_register_netdev() > > + > > PER-CPU MEM > > devm_alloc_percpu() > > devm_free_percpu() > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 6c3f7032e8d9..710a7bcfc3dc 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -4196,6 +4196,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, > > count) > > > > int register_netdev(struct net_device *dev); > > +int devm_register_netdev(struct device *dev, struct net_device *ndev); > > void unregister_netdev(struct net_device *dev); > > > > /* General hardware address lists handling functions */ > > diff --git a/net/core/Makefile b/net/core/Makefile > > index 3e2c378e5f31..f530894068d2 100644 > > --- a/net/core/Makefile > > +++ b/net/core/Makefile > > @@ -8,7 +8,7 @@ obj-y := sock.o request_sock.o skbuff.o datagram.o stream.o scm.o \ > > > > obj-$(CONFIG_SYSCTL) += sysctl_net_core.o > > > > -obj-y += dev.o dev_addr_lists.o dst.o netevent.o \ > > +obj-y += dev.o devres.o dev_addr_lists.o dst.o netevent.o \ > > neighbour.o rtnetlink.o utils.o link_watch.o filter.o \ > > sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \ > > fib_notifier.o xdp.o flow_offload.o > > diff --git a/net/core/devres.c b/net/core/devres.c > > new file mode 100644 > > index 000000000000..3c080abd1935 > > --- /dev/null > > +++ b/net/core/devres.c > > Why a new source file and not just add the function to net/core/dev.c? >
This is a common approach in most sub-systems to have a dedicated devres.c source file for managed helpers. Eventually we could move devm_alloc_etherdev() here as well.
> > > @@ -0,0 +1,41 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2020 BayLibre SAS > > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/netdevice.h> > > + > > +struct netdevice_devres { > > + struct net_device *ndev; > > +}; > > + > > Adding such a struct isn't strictly needed. >
I believe it's better in terms of readability.
> > +static void devm_netdev_release(struct device *dev, void *res) > > +{ > > + struct netdevice_devres *this = res; > > + > > + unregister_netdev(this->ndev); > > +} > > + > > +int devm_register_netdev(struct device *dev, struct net_device *ndev) > > +{ > > In this function you'd need to consider the dependency on a previous > call to devm_alloc_etherdev(). If the netdevice is allocated non-managed, > then free_netdev() would be called whilst the netdevice is still > registered, what would trigger a BUG_ON(). Therefore devm_register_netdev() > should return an error if the netdevice was allocated non-managed. > The mentioned scenario would result from a severe programming error > of course, but there are less experienced driver authors and the net core > should deal gently with wrong API usage. >
Thank you for bringing this to my attention, I wasn't aware of that. I'll rework this.
Best regards, Bartosz
> An example how this could be done you can find in the PCI subsystem, > see pcim_release() and related functions like pcim_enable() and > pcim_set_mwi(). > > > + struct netdevice_devres *devres; > > + int ret; > > + > > + devres = devres_alloc(devm_netdev_release, sizeof(*devres), GFP_KERNEL); > > + if (!devres) > > + return -ENOMEM; > > + > > + ret = register_netdev(ndev); > > + if (ret) { > > + devres_free(devres); > > + return ret; > > + } > > + > > + devres->ndev = ndev; > > + devres_add(dev, devres); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(devm_register_netdev); > > >
| |