* [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
@ 2020-02-17 20:34 Heiner Kallweit
2020-02-17 20:34 ` [PATCH net-next 1/2] " Heiner Kallweit
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Heiner Kallweit @ 2020-02-17 20:34 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux,
David Miller, Realtek linux nic maintainers
Cc: netdev
If using managed MDIO bus handling (devm_mdiobus_alloc et al) we still
have to manually unregister the MDIO bus. For drivers that don't depend
on unregistering the MDIO bus at a specific, earlier point in time we
can make driver author's life easier by automagically unregistering
the MDIO bus. This extension is transparent to existing drivers.
Heiner Kallweit (2):
net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
r8169: let managed MDIO bus handling unregister the MDIO bus
drivers/net/ethernet/realtek/r8169_main.c | 11 ++---------
drivers/net/phy/mdio_bus.c | 7 ++++++-
2 files changed, 8 insertions(+), 10 deletions(-)
--
2.25.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next 1/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
2020-02-17 20:34 [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed Heiner Kallweit
@ 2020-02-17 20:34 ` Heiner Kallweit
2020-02-19 22:21 ` Andrew Lunn
2020-02-17 20:35 ` [PATCH net-next 2/2] r8169: let managed MDIO bus handling unregister the MDIO bus Heiner Kallweit
2020-02-21 19:42 ` [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2020-02-17 20:34 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux,
David Miller, Realtek linux nic maintainers
Cc: netdev
If using managed MDIO bus handling (devm_mdiobus_alloc et al) we still
have to manually unregister the MDIO bus. For drivers that don't depend
on unregistering the MDIO bus at a specific, earlier point in time we
can make driver author's life easier by automagically unregistering
the MDIO bus. This extension is transparent to existing drivers.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/mdio_bus.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 9bb9f37f2..6af51cbdb 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -170,7 +170,12 @@ EXPORT_SYMBOL(mdiobus_alloc_size);
static void _devm_mdiobus_free(struct device *dev, void *res)
{
- mdiobus_free(*(struct mii_bus **)res);
+ struct mii_bus *bus = *(struct mii_bus **)res;
+
+ if (bus->state == MDIOBUS_REGISTERED)
+ mdiobus_unregister(bus);
+
+ mdiobus_free(bus);
}
static int devm_mdiobus_match(struct device *dev, void *res, void *data)
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next 2/2] r8169: let managed MDIO bus handling unregister the MDIO bus
2020-02-17 20:34 [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed Heiner Kallweit
2020-02-17 20:34 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2020-02-17 20:35 ` Heiner Kallweit
2020-02-21 19:42 ` [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2020-02-17 20:35 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux,
David Miller, Realtek linux nic maintainers
Cc: netdev
Make use of the recent extension to automagically unregister the MDIO
bus in case managed MDIO bus handling is used.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169_main.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ad4bb5ac6..5a9143b50 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5102,7 +5102,6 @@ static void rtl_remove_one(struct pci_dev *pdev)
netif_napi_del(&tp->napi);
unregister_netdev(dev);
- mdiobus_unregister(tp->phydev->mdio.bus);
rtl_release_firmware(tp);
@@ -5244,10 +5243,8 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
return ret;
tp->phydev = mdiobus_get_phy(new_bus, 0);
- if (!tp->phydev) {
- mdiobus_unregister(new_bus);
+ if (!tp->phydev)
return -ENODEV;
- }
/* PHY will be woken up in rtl_open() */
phy_suspend(tp->phydev);
@@ -5585,7 +5582,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
rc = register_netdev(dev);
if (rc)
- goto err_mdio_unregister;
+ return rc;
netif_info(tp, probe, dev, "%s, %pM, XID %03x, IRQ %d\n",
rtl_chip_infos[chipset].name, dev->dev_addr, xid,
@@ -5604,10 +5601,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pm_runtime_put_sync(&pdev->dev);
return 0;
-
-err_mdio_unregister:
- mdiobus_unregister(tp->phydev->mdio.bus);
- return rc;
}
static struct pci_driver rtl8169_pci_driver = {
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
2020-02-17 20:34 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2020-02-19 22:21 ` Andrew Lunn
2020-02-19 22:51 ` Heiner Kallweit
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2020-02-19 22:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, Russell King - ARM Linux, David Miller,
Realtek linux nic maintainers, netdev
On Mon, Feb 17, 2020 at 09:34:57PM +0100, Heiner Kallweit wrote:
> If using managed MDIO bus handling (devm_mdiobus_alloc et al) we still
> have to manually unregister the MDIO bus. For drivers that don't depend
> on unregistering the MDIO bus at a specific, earlier point in time we
> can make driver author's life easier by automagically unregistering
> the MDIO bus. This extension is transparent to existing drivers.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/mdio_bus.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 9bb9f37f2..6af51cbdb 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -170,7 +170,12 @@ EXPORT_SYMBOL(mdiobus_alloc_size);
>
> static void _devm_mdiobus_free(struct device *dev, void *res)
> {
> - mdiobus_free(*(struct mii_bus **)res);
> + struct mii_bus *bus = *(struct mii_bus **)res;
> +
> + if (bus->state == MDIOBUS_REGISTERED)
> + mdiobus_unregister(bus);
> +
> + mdiobus_free(bus);
> }
Hi Heiner
The API is rather asymmetric. The alloc is not just setting up a free,
but also an unregister. Are there other examples of this in the
kernel?
Maybe a devm_of_mdiobus_register() would be better? It is then clear
that the unregister happens because of this call, and the free because
of the devm_mdiobus_alloc().
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
2020-02-19 22:21 ` Andrew Lunn
@ 2020-02-19 22:51 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2020-02-19 22:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Russell King - ARM Linux, David Miller,
Realtek linux nic maintainers, netdev
On 19.02.2020 23:21, Andrew Lunn wrote:
> On Mon, Feb 17, 2020 at 09:34:57PM +0100, Heiner Kallweit wrote:
>> If using managed MDIO bus handling (devm_mdiobus_alloc et al) we still
>> have to manually unregister the MDIO bus. For drivers that don't depend
>> on unregistering the MDIO bus at a specific, earlier point in time we
>> can make driver author's life easier by automagically unregistering
>> the MDIO bus. This extension is transparent to existing drivers.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/mdio_bus.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 9bb9f37f2..6af51cbdb 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -170,7 +170,12 @@ EXPORT_SYMBOL(mdiobus_alloc_size);
>>
>> static void _devm_mdiobus_free(struct device *dev, void *res)
>> {
>> - mdiobus_free(*(struct mii_bus **)res);
>> + struct mii_bus *bus = *(struct mii_bus **)res;
>> +
>> + if (bus->state == MDIOBUS_REGISTERED)
>> + mdiobus_unregister(bus);
>> +
>> + mdiobus_free(bus);
>> }
>
> Hi Heiner
>
> The API is rather asymmetric. The alloc is not just setting up a free,
> but also an unregister. Are there other examples of this in the
> kernel?
>
To a certain extent pcim_release() is similar. It silently reverses
previous calls to pci_enable_msi / pci_enable_msix (after calling
pcim_enable_device).
> Maybe a devm_of_mdiobus_register() would be better? It is then clear
> that the unregister happens because of this call, and the free because
> of the devm_mdiobus_alloc().
>
I understand the concern regarding the asymmetry. For me the question
is whether the additional effort for avoiding it is worth it.
We'd have to create at least devm_register_mdiobus and
devm_of_mdiobus_register.
Also then we'd have to think about how to deal with the case that a
non-perfect driver author combines a non-managed mdiobus_alloc
with a later call to devm_register_mdiobus, because we'd like to
avoid the case that mdiobus_free is called before mdiobus_unregister.
Maybe we would need a mechanism similar to the one used in the
PCI core functions mentioned earlier.
devm_register_mdiobus would have to warn if the bus allocation was
non-managed.
> Andrew
>
Heiner
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
2020-02-17 20:34 [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed Heiner Kallweit
2020-02-17 20:34 ` [PATCH net-next 1/2] " Heiner Kallweit
2020-02-17 20:35 ` [PATCH net-next 2/2] r8169: let managed MDIO bus handling unregister the MDIO bus Heiner Kallweit
@ 2020-02-21 19:42 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-02-21 19:42 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, linux, nic_swsd, netdev
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Mon, 17 Feb 2020 21:34:10 +0100
> If using managed MDIO bus handling (devm_mdiobus_alloc et al) we still
> have to manually unregister the MDIO bus. For drivers that don't depend
> on unregistering the MDIO bus at a specific, earlier point in time we
> can make driver author's life easier by automagically unregistering
> the MDIO bus. This extension is transparent to existing drivers.
>
> Heiner Kallweit (2):
> net: phy: unregister MDIO bus in _devm_mdiobus_free if needed
> r8169: let managed MDIO bus handling unregister the MDIO bus
Heiner, I'm going to defer on this.
The existing behavior, this proposal, and the alternatives you
described with Andrew Lunn in the patch #1 discussion all seem
equally cumbersome and awkward to me.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-21 19:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 20:34 [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed Heiner Kallweit
2020-02-17 20:34 ` [PATCH net-next 1/2] " Heiner Kallweit
2020-02-19 22:21 ` Andrew Lunn
2020-02-19 22:51 ` Heiner Kallweit
2020-02-17 20:35 ` [PATCH net-next 2/2] r8169: let managed MDIO bus handling unregister the MDIO bus Heiner Kallweit
2020-02-21 19:42 ` [PATCH net-next 0/2] net: phy: unregister MDIO bus in _devm_mdiobus_free if needed David Miller
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).