From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH] net: core: provide devm_register_netdev()
Date: Thu, 26 Mar 2020 18:48:00 +0100 [thread overview]
Message-ID: <CAMpxmJWLbUR5ojGi3vdMw-vrG3ias9yUE+ycrhZ8m=EL-GrreQ@mail.gmail.com> (raw)
In-Reply-To: <c71a132d-dc33-0b8a-29e0-9cf93056ef52@gmail.com>
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);
> >
>
prev parent reply other threads:[~2020-03-26 17:48 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-26 13:17 [PATCH] net: core: provide devm_register_netdev() Bartosz Golaszewski
2020-03-26 17:39 ` Heiner Kallweit
2020-03-26 17:48 ` Bartosz Golaszewski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMpxmJWLbUR5ojGi3vdMw-vrG3ias9yUE+ycrhZ8m=EL-GrreQ@mail.gmail.com' \
--to=bgolaszewski@baylibre.com \
--cc=brgl@bgdev.pl \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).