Re: [PATCH] net: core: provide devm_register_netdev()

[Bartosz Golaszewski]

czw., 26 mar 2020 o 18:39 Heiner Kallweit <hkallweit1@xxxxxxxxx> napisaÅ(a):
>
> On 26.03.2020 14:17, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx>
> > + */
> > +
> > +#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);
> >
>