2020-05-05 14:07:49

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH 05/11] net: core: provide devm_register_netdev()

From: Bartosz Golaszewski <[email protected]>

Provide devm_register_netdev() - a device resource managed variant
of register_netdev(). This new helper will only work for net_device
structs that have a parent device assigned and are devres managed too.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
include/linux/netdevice.h | 4 ++++
net/core/dev.c | 48 +++++++++++++++++++++++++++++++++++++++
net/ethernet/eth.c | 1 +
3 files changed, 53 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 130a668049ab..433bd5ca2efc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1515,6 +1515,8 @@ struct net_device_ops {
* @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
* @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
* @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
+ * @IFF_IS_DEVRES: this structure was allocated dynamically and is managed by
+ * devres
*/
enum netdev_priv_flags {
IFF_802_1Q_VLAN = 1<<0,
@@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
IFF_FAILOVER_SLAVE = 1<<28,
IFF_L3MDEV_RX_HANDLER = 1<<29,
IFF_LIVE_RENAME_OK = 1<<30,
+ IFF_IS_DEVRES = 1<<31,
};

#define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
@@ -4206,6 +4209,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 net_device *ndev);
void unregister_netdev(struct net_device *dev);

/* General hardware address lists handling functions */
diff --git a/net/core/dev.c b/net/core/dev.c
index 522288177bbd..99db537c9468 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
}
EXPORT_SYMBOL(register_netdev);

+struct netdevice_devres {
+ struct net_device *ndev;
+};
+
+static void devm_netdev_release(struct device *dev, void *this)
+{
+ struct netdevice_devres *res = this;
+
+ unregister_netdev(res->ndev);
+}
+
+/**
+ * devm_register_netdev - resource managed variant of register_netdev()
+ * @ndev: device to register
+ *
+ * This is a devres variant of register_netdev() for which the unregister
+ * function will be call automatically when the parent device of ndev
+ * is detached.
+ */
+int devm_register_netdev(struct net_device *ndev)
+{
+ struct netdevice_devres *dr;
+ int ret;
+
+ /* struct net_device itself must be devres managed. */
+ BUG_ON(!(ndev->priv_flags & IFF_IS_DEVRES));
+ /* struct net_device must have a parent device - it will be the device
+ * managing this resource.
+ */
+ BUG_ON(!ndev->dev.parent);
+
+ dr = devres_alloc(devm_netdev_release, sizeof(*dr), GFP_KERNEL);
+ if (!dr)
+ return -ENOMEM;
+
+ ret = register_netdev(ndev);
+ if (ret) {
+ devres_free(dr);
+ return ret;
+ }
+
+ dr->ndev = ndev;
+ devres_add(ndev->dev.parent, dr);
+
+ return 0;
+}
+EXPORT_SYMBOL(devm_register_netdev);
+
int netdev_refcnt_read(const struct net_device *dev)
{
int i, refcnt = 0;
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index c8b903302ff2..ce9b5e576f20 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -423,6 +423,7 @@ struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv,

*dr = netdev;
devres_add(dev, dr);
+ netdev->priv_flags |= IFF_IS_DEVRES;

return netdev;
}
--
2.25.0


2020-05-05 17:33:19

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Provide devm_register_netdev() - a device resource managed variant
> of register_netdev(). This new helper will only work for net_device
> structs that have a parent device assigned and are devres managed too.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 522288177bbd..99db537c9468 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> }
> EXPORT_SYMBOL(register_netdev);
>
> +struct netdevice_devres {
> + struct net_device *ndev;
> +};

Is there really a need to define a structure if we only need a pointer?

> +static void devm_netdev_release(struct device *dev, void *this)
> +{
> + struct netdevice_devres *res = this;
> +
> + unregister_netdev(res->ndev);
> +}
> +
> +/**
> + * devm_register_netdev - resource managed variant of register_netdev()
> + * @ndev: device to register
> + *
> + * This is a devres variant of register_netdev() for which the unregister
> + * function will be call automatically when the parent device of ndev
> + * is detached.
> + */
> +int devm_register_netdev(struct net_device *ndev)
> +{
> + struct netdevice_devres *dr;
> + int ret;
> +
> + /* struct net_device itself must be devres managed. */
> + BUG_ON(!(ndev->priv_flags & IFF_IS_DEVRES));
> + /* struct net_device must have a parent device - it will be the device
> + * managing this resource.
> + */
> + BUG_ON(!ndev->dev.parent);

Please convert those to WARN_ON, and return an error. No need to crash
the kernel.

> + dr = devres_alloc(devm_netdev_release, sizeof(*dr), GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + ret = register_netdev(ndev);
> + if (ret) {
> + devres_free(dr);
> + return ret;
> + }
> +
> + dr->ndev = ndev;
> + devres_add(ndev->dev.parent, dr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(devm_register_netdev);

2020-05-05 19:27:32

by Edwin Peer

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Tue, May 5, 2020 at 7:05 AM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> Provide devm_register_netdev() - a device resource managed variant
> of register_netdev(). This new helper will only work for net_device
> structs that have a parent device assigned and are devres managed too.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> include/linux/netdevice.h | 4 ++++
> net/core/dev.c | 48 +++++++++++++++++++++++++++++++++++++++
> net/ethernet/eth.c | 1 +
> 3 files changed, 53 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 130a668049ab..433bd5ca2efc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1515,6 +1515,8 @@ struct net_device_ops {
> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
> * @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
> + * @IFF_IS_DEVRES: this structure was allocated dynamically and is managed by
> + * devres
> */
> enum netdev_priv_flags {
> IFF_802_1Q_VLAN = 1<<0,
> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> IFF_FAILOVER_SLAVE = 1<<28,
> IFF_L3MDEV_RX_HANDLER = 1<<29,
> IFF_LIVE_RENAME_OK = 1<<30,
> + IFF_IS_DEVRES = 1<<31,
> };
>
> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> @@ -4206,6 +4209,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 net_device *ndev);
> void unregister_netdev(struct net_device *dev);
>
> /* General hardware address lists handling functions */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 522288177bbd..99db537c9468 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> }
> EXPORT_SYMBOL(register_netdev);
>
> +struct netdevice_devres {
> + struct net_device *ndev;
> +};
> +
> +static void devm_netdev_release(struct device *dev, void *this)
> +{
> + struct netdevice_devres *res = this;
> +
> + unregister_netdev(res->ndev);
> +}
> +
> +/**
> + * devm_register_netdev - resource managed variant of register_netdev()
> + * @ndev: device to register
> + *
> + * This is a devres variant of register_netdev() for which the unregister
> + * function will be call automatically when the parent device of ndev
> + * is detached.
> + */
> +int devm_register_netdev(struct net_device *ndev)
> +{
> + struct netdevice_devres *dr;
> + int ret;
> +
> + /* struct net_device itself must be devres managed. */
> + BUG_ON(!(ndev->priv_flags & IFF_IS_DEVRES));
> + /* struct net_device must have a parent device - it will be the device
> + * managing this resource.
> + */

Catching static programming errors seems like an expensive use of the
last runtime flag in the enum. It would be weird to devres manage the
unregister and not also choose to manage the underlying memory in the
same fashion, so it wouldn't be an obvious mistake to make. If it must
be enforced, one could also iterate over the registered release
functions and check for the presence of devm_free_netdev without
burning the flag.

> + BUG_ON(!ndev->dev.parent);
> +
> + dr = devres_alloc(devm_netdev_release, sizeof(*dr), GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + ret = register_netdev(ndev);
> + if (ret) {
> + devres_free(dr);
> + return ret;
> + }
> +
> + dr->ndev = ndev;
> + devres_add(ndev->dev.parent, dr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(devm_register_netdev);
> +
> int netdev_refcnt_read(const struct net_device *dev)
> {
> int i, refcnt = 0;
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index c8b903302ff2..ce9b5e576f20 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -423,6 +423,7 @@ struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv,
>
> *dr = netdev;
> devres_add(dev, dr);
> + netdev->priv_flags |= IFF_IS_DEVRES;
>
> return netdev;
> }
> --
> 2.25.0
>

Regards,
Edwin Peer

2020-05-06 06:44:59

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

wt., 5 maj 2020 o 19:31 Jakub Kicinski <[email protected]> napisał(a):
>
> On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Provide devm_register_netdev() - a device resource managed variant
> > of register_netdev(). This new helper will only work for net_device
> > structs that have a parent device assigned and are devres managed too.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 522288177bbd..99db537c9468 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> > }
> > EXPORT_SYMBOL(register_netdev);
> >
> > +struct netdevice_devres {
> > + struct net_device *ndev;
> > +};
>
> Is there really a need to define a structure if we only need a pointer?
>

There is no need for that, but it really is more readable this way.
Also: using a pointer directly doesn't save us any memory nor code
here.

Bart

2020-05-06 06:48:43

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

wt., 5 maj 2020 o 21:25 Edwin Peer <[email protected]> napisał(a):
> > +
> > +static void devm_netdev_release(struct device *dev, void *this)
> > +{
> > + struct netdevice_devres *res = this;
> > +
> > + unregister_netdev(res->ndev);
> > +}
> > +
> > +/**
> > + * devm_register_netdev - resource managed variant of register_netdev()
> > + * @ndev: device to register
> > + *
> > + * This is a devres variant of register_netdev() for which the unregister
> > + * function will be call automatically when the parent device of ndev
> > + * is detached.
> > + */
> > +int devm_register_netdev(struct net_device *ndev)
> > +{
> > + struct netdevice_devres *dr;
> > + int ret;
> > +
> > + /* struct net_device itself must be devres managed. */
> > + BUG_ON(!(ndev->priv_flags & IFF_IS_DEVRES));
> > + /* struct net_device must have a parent device - it will be the device
> > + * managing this resource.
> > + */
>
> Catching static programming errors seems like an expensive use of the
> last runtime flag in the enum. It would be weird to devres manage the
> unregister and not also choose to manage the underlying memory in the
> same fashion, so it wouldn't be an obvious mistake to make. If it must
> be enforced, one could also iterate over the registered release
> functions and check for the presence of devm_free_netdev without
> burning the flag.
>

Hi Edwin,

I've submitted this patch some time ago already and was told to check
if the underlying memory is managed too. I guess I could try to use
devres_find() here though.

Re the last bit in priv_flags: is this really a problem though? It's
not like struct net_device must remain stable - e.g. we can make
priv_flags a bitmap.

Bart

2020-05-06 17:16:28

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Wed, 6 May 2020 08:39:47 +0200 Bartosz Golaszewski wrote:
> wt., 5 maj 2020 o 19:31 Jakub Kicinski <[email protected]> napisał(a):
> >
> > On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > Provide devm_register_netdev() - a device resource managed variant
> > > of register_netdev(). This new helper will only work for net_device
> > > structs that have a parent device assigned and are devres managed too.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 522288177bbd..99db537c9468 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> > > }
> > > EXPORT_SYMBOL(register_netdev);
> > >
> > > +struct netdevice_devres {
> > > + struct net_device *ndev;
> > > +};
> >
> > Is there really a need to define a structure if we only need a pointer?
> >
>
> There is no need for that, but it really is more readable this way.
> Also: using a pointer directly doesn't save us any memory nor code
> here.

I don't care either way but devm_alloc_etherdev_mqs() and co. are using
the double pointer directly. Please make things consistent. Either do
the same, or define the structure in some header and convert other
helpers to also make use of it.

2020-05-06 18:24:15

by Edwin Peer

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Tue, May 5, 2020 at 11:46 PM Bartosz Golaszewski <[email protected]> wrote:

> Re the last bit in priv_flags: is this really a problem though? It's
> not like struct net_device must remain stable - e.g. we can make
> priv_flags a bitmap.

Fair enough.

Regards,
Edwin Peer

2020-05-07 09:27:57

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

śr., 6 maj 2020 o 19:12 Jakub Kicinski <[email protected]> napisał(a):
>
> On Wed, 6 May 2020 08:39:47 +0200 Bartosz Golaszewski wrote:
> > wt., 5 maj 2020 o 19:31 Jakub Kicinski <[email protected]> napisał(a):
> > >
> > > On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > Provide devm_register_netdev() - a device resource managed variant
> > > > of register_netdev(). This new helper will only work for net_device
> > > > structs that have a parent device assigned and are devres managed too.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > >
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 522288177bbd..99db537c9468 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> > > > }
> > > > EXPORT_SYMBOL(register_netdev);
> > > >
> > > > +struct netdevice_devres {
> > > > + struct net_device *ndev;
> > > > +};
> > >
> > > Is there really a need to define a structure if we only need a pointer?
> > >
> >
> > There is no need for that, but it really is more readable this way.
> > Also: using a pointer directly doesn't save us any memory nor code
> > here.
>
> I don't care either way but devm_alloc_etherdev_mqs() and co. are using
> the double pointer directly. Please make things consistent. Either do
> the same, or define the structure in some header and convert other
> helpers to also make use of it.

In order to use devres_find() to check if struct net_device is managed
in devm_register_netdev() I need to know the address of the release
function used by devm_alloc_etherdev_mqs(). Do you mind if I move all
networking devres routines (currently only devm_alloc_etherdev_mqs())
into a separate .c file (e.g. under net/devres.c)?

Bart

2020-05-07 16:55:18

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Thu, 7 May 2020 11:25:01 +0200 Bartosz Golaszewski wrote:
> śr., 6 maj 2020 o 19:12 Jakub Kicinski <[email protected]> napisał(a):
> >
> > On Wed, 6 May 2020 08:39:47 +0200 Bartosz Golaszewski wrote:
> > > wt., 5 maj 2020 o 19:31 Jakub Kicinski <[email protected]> napisał(a):
> > > >
> > > > On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <[email protected]>
> > > > >
> > > > > Provide devm_register_netdev() - a device resource managed variant
> > > > > of register_netdev(). This new helper will only work for net_device
> > > > > structs that have a parent device assigned and are devres managed too.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > >
> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > index 522288177bbd..99db537c9468 100644
> > > > > --- a/net/core/dev.c
> > > > > +++ b/net/core/dev.c
> > > > > @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> > > > > }
> > > > > EXPORT_SYMBOL(register_netdev);
> > > > >
> > > > > +struct netdevice_devres {
> > > > > + struct net_device *ndev;
> > > > > +};
> > > >
> > > > Is there really a need to define a structure if we only need a pointer?
> > > >
> > >
> > > There is no need for that, but it really is more readable this way.
> > > Also: using a pointer directly doesn't save us any memory nor code
> > > here.
> >
> > I don't care either way but devm_alloc_etherdev_mqs() and co. are using
> > the double pointer directly. Please make things consistent. Either do
> > the same, or define the structure in some header and convert other
> > helpers to also make use of it.
>
> In order to use devres_find() to check if struct net_device is managed
> in devm_register_netdev() I need to know the address of the release
> function used by devm_alloc_etherdev_mqs(). Do you mind if I move all
> networking devres routines (currently only devm_alloc_etherdev_mqs())
> into a separate .c file (e.g. under net/devres.c)?

To implement Edwin's suggestion? Makes sense, but I'm no expert, let's
also CC Heiner since he was asking about it last time.

2020-05-07 17:06:43

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

czw., 7 maj 2020 o 18:53 Jakub Kicinski <[email protected]> napisał(a):
>
> On Thu, 7 May 2020 11:25:01 +0200 Bartosz Golaszewski wrote:
> > śr., 6 maj 2020 o 19:12 Jakub Kicinski <[email protected]> napisał(a):
> > >
> > > On Wed, 6 May 2020 08:39:47 +0200 Bartosz Golaszewski wrote:
> > > > wt., 5 maj 2020 o 19:31 Jakub Kicinski <[email protected]> napisał(a):
> > > > >
> > > > > On Tue, 5 May 2020 16:02:25 +0200 Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <[email protected]>
> > > > > >
> > > > > > Provide devm_register_netdev() - a device resource managed variant
> > > > > > of register_netdev(). This new helper will only work for net_device
> > > > > > structs that have a parent device assigned and are devres managed too.
> > > > > >
> > > > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > > >
> > > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > > index 522288177bbd..99db537c9468 100644
> > > > > > --- a/net/core/dev.c
> > > > > > +++ b/net/core/dev.c
> > > > > > @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
> > > > > > }
> > > > > > EXPORT_SYMBOL(register_netdev);
> > > > > >
> > > > > > +struct netdevice_devres {
> > > > > > + struct net_device *ndev;
> > > > > > +};
> > > > >
> > > > > Is there really a need to define a structure if we only need a pointer?
> > > > >
> > > >
> > > > There is no need for that, but it really is more readable this way.
> > > > Also: using a pointer directly doesn't save us any memory nor code
> > > > here.
> > >
> > > I don't care either way but devm_alloc_etherdev_mqs() and co. are using
> > > the double pointer directly. Please make things consistent. Either do
> > > the same, or define the structure in some header and convert other
> > > helpers to also make use of it.
> >
> > In order to use devres_find() to check if struct net_device is managed
> > in devm_register_netdev() I need to know the address of the release
> > function used by devm_alloc_etherdev_mqs(). Do you mind if I move all
> > networking devres routines (currently only devm_alloc_etherdev_mqs())
> > into a separate .c file (e.g. under net/devres.c)?
>
> To implement Edwin's suggestion? Makes sense, but I'm no expert, let's
> also CC Heiner since he was asking about it last time.

Yes, because taking the last bit of priv_flags from net_device seems
to be more controversial but if net maintainers are fine with that I
can simply go with the current approach.

Bart

2020-05-07 23:00:36

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Thu, 7 May 2020 19:03:44 +0200 Bartosz Golaszewski wrote:
>> To implement Edwin's suggestion? Makes sense, but I'm no expert, let's
>> also CC Heiner since he was asking about it last time.
>
> Yes, because taking the last bit of priv_flags from net_device seems
> to be more controversial but if net maintainers are fine with that I
> can simply go with the current approach.

From my perspective what Edwin suggests makes sense. Apart from
little use for the bit after probe, it also seems cleaner for devres
to be able to recognize managed objects based on its own state.

2020-05-08 05:56:31

by Heiner Kallweit

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On 08.05.2020 00:56, Jakub Kicinski wrote:
> On Thu, 7 May 2020 19:03:44 +0200 Bartosz Golaszewski wrote:
>>> To implement Edwin's suggestion? Makes sense, but I'm no expert, let's
>>> also CC Heiner since he was asking about it last time.
>>
>> Yes, because taking the last bit of priv_flags from net_device seems
>> to be more controversial but if net maintainers are fine with that I
>> can simply go with the current approach.
>
> From my perspective what Edwin suggests makes sense. Apart from
> little use for the bit after probe, it also seems cleaner for devres
> to be able to recognize managed objects based on its own state.
>
What I was saying is that we should catch the case that a driver
author uses a device-managed register() w/o doing the same for the
alloc(). A core function should not assume that driver authors do
sane things only.
I don't have a strong preference how it should be done.
Considering what is being discussed, have a look at get_pci_dr() and
find_pci_dr(), they deal with managing which parts of the PCI
subsystem are device-managed.

2020-05-08 18:41:50

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

pt., 8 maj 2020 o 07:54 Heiner Kallweit <[email protected]> napisał(a):
>
> On 08.05.2020 00:56, Jakub Kicinski wrote:
> > On Thu, 7 May 2020 19:03:44 +0200 Bartosz Golaszewski wrote:
> >>> To implement Edwin's suggestion? Makes sense, but I'm no expert, let's
> >>> also CC Heiner since he was asking about it last time.
> >>
> >> Yes, because taking the last bit of priv_flags from net_device seems
> >> to be more controversial but if net maintainers are fine with that I
> >> can simply go with the current approach.
> >
> > From my perspective what Edwin suggests makes sense. Apart from
> > little use for the bit after probe, it also seems cleaner for devres
> > to be able to recognize managed objects based on its own state.
> >
> What I was saying is that we should catch the case that a driver
> author uses a device-managed register() w/o doing the same for the
> alloc(). A core function should not assume that driver authors do
> sane things only.
> I don't have a strong preference how it should be done.
> Considering what is being discussed, have a look at get_pci_dr() and
> find_pci_dr(), they deal with managing which parts of the PCI
> subsystem are device-managed.

Yes, I have - that's why I asked if anyone objects to me moving all
networking devres functions into their own source file. The reason for
that being: devres_find() needs to know the address of the release
function, meanwhile devm_register_netdev() would have to go into
net/core, while devm_alloc_etherdev() lives in net/ethernet.

Bart