net: core: provide devm_register_netdev()

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.

 .../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

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.

>  .../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?


> @@ -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.

> +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.

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);
>

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);
> >
>