From: Bartosz Golaszewski <redacted>
When I first submitted the series adding devm_register_netdev() I was
told during review that it should check if the underlying struct net_device
is managed too before proceeding. I initially accepted this as the right
approach but in the back of my head something seemed wrong about this.
I started looking around and noticed how devm_mdiobus_register()
is implemented.
It turned out that struct mii_bus contains information about whether it's
managed or not and the release callback of devm_mdiobus_alloc() is responsible
for calling mdiobus_unregister(). This seems wrong to me as managed structures
shouldn't care about who manages them. It's devres' code task to correctly undo
whatever it registers/allocates.
With this series I propose to make the release callbacks of mdiobus devm
helpers only release the resources they actually allocate themselves as it the
standard in devm routines. I also propose to not check whether the structures
passed to devm_mdiobus_register() and devm_register_netdev() are already
managed as they could have been allocated over devres as part of bigger
memory chunk. I see this as an unnecessary limitation.
First two patches aim at removing the only use of devm_mdiobus_free(). It
modifies the ixgbe driver. I only compile tested it as I don't have the
relevant hw.
Next two patches relax devm_register_netdev() - we stop checking whether
struct net_device was registered using devm_etherdev_alloc().
We then document the mdio devres helper that's missing in devres.rst list
and un-inline the current implementation of devm_mdiobus_register().
Patch 8 re-implements the devres helpers for mdio conforming to common
devres patterns.
Patches 9 and 10 provide devm_of_mdiobus_register() and the last patch
adds its first user.
Bartosz Golaszewski (11):
net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()
net: ethernet: ixgbe: don't call devm_mdiobus_free()
net: devres: relax devm_register_netdev()
net: devres: rename the release callback of devm_register_netdev()
Documentation: devres: add missing mdio helper
phy: un-inline devm_mdiobus_register()
phy: mdio: add kerneldoc for __devm_mdiobus_register()
net: phy: don't abuse devres in devm_mdiobus_register()
of: mdio: remove the 'extern' keyword from function declarations
of: mdio: provide devm_of_mdiobus_register()
net: ethernet: mtk-star-emac: use devm_of_mdiobus_register()
.../driver-api/driver-model/devres.rst | 3 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 14 +---
drivers/net/ethernet/mediatek/mtk_star_emac.c | 13 +--
drivers/net/ethernet/realtek/r8169_main.c | 2 +-
drivers/net/phy/Makefile | 2 +-
drivers/net/phy/mdio_bus.c | 73 ----------------
drivers/net/phy/mdio_devres.c | 83 +++++++++++++++++++
drivers/of/of_mdio.c | 43 ++++++++++
include/linux/of_mdio.h | 40 ++++-----
include/linux/phy.h | 21 +----
net/devres.c | 23 +----
12 files changed, 167 insertions(+), 156 deletions(-)
create mode 100644 drivers/net/phy/mdio_devres.c
--
2.26.1
From: Bartosz Golaszewski <redacted>
This devres helper registers a release callback that only unregisters
the net_device. It works perfectly fine with netdev structs that are
not managed on their own. There's no reason to check this - drop the
warning.
Signed-off-by: Bartosz Golaszewski <redacted>
---
net/devres.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
@@ -61,22 +53,13 @@ static int netdev_devres_match(struct device *dev, void *this, void *match_data)**Thisisadevresvariantofregister_netdev()forwhichtheunregister*functionwillbecallautomaticallywhenthemanagingdeviceis-*detached.Note:thenet_deviceusedmustalsoberesourcemanagedby-*thesamestructdevice.+*detached.*/intdevm_register_netdev(structdevice*dev,structnet_device*ndev){structnet_device_devres*dr;intret;-/* struct net_device must itself be managed. For now a managed netdev-*canonlybeallocatedbydevm_alloc_etherdev_mqs()sothecheckis-*straightforward.-*/-if(WARN_ON(!devres_find(dev,devm_free_netdev,-netdev_devres_match,ndev)))-return-EINVAL;-dr=devres_alloc(devm_netdev_release,sizeof(*dr),GFP_KERNEL);if(!dr)return-ENOMEM;
From: Bartosz Golaszewski <redacted>
Implement a managed variant of of_mdiobus_register(). We need to
reimplement the devres structure and the release callback because we
can't put this function in drivers/net/phy/mdio_devres.c or we'd hit
circular dependencies between module symbols. We also don't want to
build this bit if OF is not selected in Kconfig.
Signed-off-by: Bartosz Golaszewski <redacted>
---
.../driver-api/driver-model/devres.rst | 1 +
drivers/of/of_mdio.c | 43 +++++++++++++++++++
include/linux/of_mdio.h | 3 ++
3 files changed, 47 insertions(+)
@@ -330,6 +330,49 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)}EXPORT_SYMBOL(of_mdiobus_register);+/* This duplicates the devres code from drivers/net/phy/mdio_devres.c but+*ifweputdevm_of_mdiobus_register()overtherewe'dhitcircularsymbol+*dependenciesbetweenthelibphyandof_mdiomodules.+*/+structmdiobus_devres{+structmii_bus*mii;+};++staticvoiddevm_mdiobus_unregister(structdevice*dev,void*this)+{+structmdiobus_devres*dr=this;++mdiobus_unregister(dr->mii);+}++/**+*devm_of_mdiobus_register-Resourcemanagedvariantofof_mdiobus_register()+*@dev:Devicetoregistermii_busfor+*@mdio:MIIbusstructuretoregister+*@np:Devicenodetoparse+*/+intdevm_of_mdiobus_register(structdevice*dev,structmii_bus*mdio,+structdevice_node*np)+{+structmdiobus_devres*dr;+intret;++dr=devres_alloc(devm_mdiobus_unregister,sizeof(*dr),GFP_KERNEL);+if(!dr)+return-ENOMEM;++ret=of_mdiobus_register(mdio,np);+if(ret){+devres_free(dr);+returnret;+}++dr->mii=mdio;+devres_add(dev,dr);+return0;+}+EXPORT_SYMBOL(devm_of_mdiobus_register);+/***of_phy_find_device-GiveaPHYnode,findthephy_device*@phy_np:Pointertothephy'sdevicetreenode
From: Bartosz Golaszewski <redacted>
We have a devres variant of mdiobus_register() but it's not listed in
devres.rst. Add it under other mdio devm functions.
Signed-off-by: Bartosz Golaszewski <redacted>
---
Documentation/driver-api/driver-model/devres.rst | 1 +
1 file changed, 1 insertion(+)
From: Bartosz Golaszewski <redacted>
The 'extern' keyword in headers doesn't have any benefit. Remove them
all from the of_mdio.h header.
Signed-off-by: Bartosz Golaszewski <redacted>
---
include/linux/of_mdio.h | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
From: Bartosz Golaszewski <redacted>
We currently have two managed helpers for mdiobus - devm_mdiobus_alloc()
and devm_mdiobus_register(). The idea behind devres is that the release
callback releases whatever resource the devm function allocates. In the
mdiobus case however there's no devres associated with the device by
devm_mdiobus_register(). Instead the release callback for
devm_mdiobus_alloc(): _devm_mdiobus_free() unregisters the device if
it is marked as managed.
This all seems wrong. The managed structure shouldn't need to know or
care about whether it's managed or not - and this is the case now for
struct mii_bus. The devres wrapper should be opaque to the managed
resource.
This changeset makes devm_mdiobus_alloc() and devm_mdiobus_register()
conform to common devres standards: devm_mdiobus_alloc() allocates a
devres structure and registers a callback that will call mdiobus_free().
__devm_mdiobus_register() allocated another devres and registers a
callback that will unregister the bus. Similarily to how we modified
devm_register_netdev() - we're not checking whether struct mii_bus is
managed - it could have been allocated as part of a bigger structure.
Signed-off-by: Bartosz Golaszewski <redacted>
---
.../driver-api/driver-model/devres.rst | 1 -
drivers/net/ethernet/realtek/r8169_main.c | 2 +-
drivers/net/phy/mdio_bus.c | 73 -------------------
drivers/net/phy/mdio_devres.c | 70 ++++++++++++++++--
include/linux/phy.h | 10 +--
5 files changed, 69 insertions(+), 87 deletions(-)
From: Bartosz Golaszewski <redacted>
Make it an explicit counterpart to devm_register_netdev() just like we
do with devm_free_netdev() for better clarity.
Signed-off-by: Bartosz Golaszewski <redacted>
---
net/devres.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Bartosz Golaszewski <redacted>
This function is not documented. Add a short kerneldoc description.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/phy/mdio_devres.c | 7 +++++++
1 file changed, 7 insertions(+)
From: Bartosz Golaszewski <redacted>
Functions should only be static inline if they're very short. This
devres helper is already over 10 lines and it will grow soon as we'll
be improving upon its approach. Pull it into mdio_devres.c.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/phy/Makefile | 2 +-
drivers/net/phy/mdio_devres.c | 18 ++++++++++++++++++
include/linux/phy.h | 15 ++-------------
3 files changed, 21 insertions(+), 14 deletions(-)
create mode 100644 drivers/net/phy/mdio_devres.c
From: Bartosz Golaszewski <redacted>
The idea behind devres is that the release callbacks are called if
probe fails. As we now check the return value of ixgbe_mii_bus_init(),
we can drop the call devm_mdiobus_free() in error path as the release
callback will be called automatically.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
From: Bartosz Golaszewski <redacted>
This function may fail. Check its return value and propagate the error
code.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-06-22 22:49:49
On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote:
From: Bartosz Golaszewski <redacted>
This devres helper registers a release callback that only unregisters
the net_device. It works perfectly fine with netdev structs that are
not managed on their own. There's no reason to check this - drop the
warning.
Signed-off-by: Bartosz Golaszewski <redacted>
I think the reasoning for this suggestion was to catch possible UAF
errors. The netdev doesn't necessarily has to be from devm_alloc_*
but it has to be part of devm-ed memory or memory which is freed
after driver's remove callback.
Are there cases in practice where you've seen the netdev not being
devm allocated?
From: Bartosz Golaszewski <redacted>
Functions should only be static inline if they're very short. This
devres helper is already over 10 lines and it will grow soon as we'll
be improving upon its approach. Pull it into mdio_devres.c.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/phy/Makefile | 2 +-
drivers/net/phy/mdio_devres.c | 18 ++++++++++++++++++
include/linux/phy.h | 15 ++-------------
3 files changed, 21 insertions(+), 14 deletions(-)
create mode 100644 drivers/net/phy/mdio_devres.c
This would likely require an update to the MAINTAINERS file for this new
file to be picked up by the correct entry.
--
Florian
wt., 23 cze 2020 o 00:49 Jakub Kicinski [off-list ref] napisał(a):
On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote:
quoted
From: Bartosz Golaszewski <redacted>
This devres helper registers a release callback that only unregisters
the net_device. It works perfectly fine with netdev structs that are
not managed on their own. There's no reason to check this - drop the
warning.
Signed-off-by: Bartosz Golaszewski <redacted>
I think the reasoning for this suggestion was to catch possible UAF
errors. The netdev doesn't necessarily has to be from devm_alloc_*
but it has to be part of devm-ed memory or memory which is freed
after driver's remove callback.
Yes I understand that UAF was the concern here, but this limitation is
unnecessary. In its current form devm_register_netdev() only works for
struct net_device allocated with devm_alloc_etherdev(). Meanwhile
calling alloc_netdev() (which doesn't have its devm counterpart yet -
I may look into it shortly), then registering a devm action with
devm_add_action_or_reset() which would free this memory is a perfectly
fine use case. This patch would make it possible.
Are there cases in practice where you've seen the netdev not being
devm allocated?
As I said above - alloc_netdev() used by wireless, can, usb etc.
drivers doesn't have a devres variant.
Bartosz
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-06-23 20:16:37
On Tue, 23 Jun 2020 11:12:24 +0200 Bartosz Golaszewski wrote:
wt., 23 cze 2020 o 00:49 Jakub Kicinski [off-list ref] napisał(a):
quoted
On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote:
quoted
From: Bartosz Golaszewski <redacted>
This devres helper registers a release callback that only unregisters
the net_device. It works perfectly fine with netdev structs that are
not managed on their own. There's no reason to check this - drop the
warning.
Signed-off-by: Bartosz Golaszewski <redacted>
I think the reasoning for this suggestion was to catch possible UAF
errors. The netdev doesn't necessarily has to be from devm_alloc_*
but it has to be part of devm-ed memory or memory which is freed
after driver's remove callback.
Yes I understand that UAF was the concern here, but this limitation is
unnecessary. In its current form devm_register_netdev() only works for
struct net_device allocated with devm_alloc_etherdev(). Meanwhile
calling alloc_netdev() (which doesn't have its devm counterpart yet -
I may look into it shortly),
If resource managed alloc_netdev() is needed devm_alloc_netdev() can
be created, and even reuse devm_free_netdev() so no changes to the
warning are even necessary for such extension.
then registering a devm action with devm_add_action_or_reset() which
would free this memory is a perfectly fine use case. This patch would
make it possible.
alloc_netdev() + devm_add_action makes no sense in the upstream kernel,
just add the appropriate helper, we care little about out of tree code.
quoted
Are there cases in practice where you've seen the netdev not being
devm allocated?
As I said above - alloc_netdev() used by wireless, can, usb etc.
drivers doesn't have a devres variant.
On Tue, Jun 23, 2020 at 1:55 AM Florian Fainelli [off-list ref] wrote:
On 6/22/20 3:00 AM, Bartosz Golaszewski wrote:
quoted
From: Bartosz Golaszewski <redacted>
Functions should only be static inline if they're very short. This
devres helper is already over 10 lines and it will grow soon as we'll
be improving upon its approach. Pull it into mdio_devres.c.
Signed-off-by: Bartosz Golaszewski <redacted>
---
drivers/net/phy/Makefile | 2 +-
drivers/net/phy/mdio_devres.c | 18 ++++++++++++++++++
include/linux/phy.h | 15 ++-------------
3 files changed, 21 insertions(+), 14 deletions(-)
create mode 100644 drivers/net/phy/mdio_devres.c
This would likely require an update to the MAINTAINERS file for this new
file to be picked up by the correct entry.
It's already included in drivers/net/phy/ in the ETHERNET PHY LIBRARY entry.
Bartosz