* [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
@ 2025-07-28 15:34 Bence Csókás
2025-07-31 1:16 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Bence Csókás @ 2025-07-28 15:34 UTC (permalink / raw)
To: Geert Uytterhoeven, Sergei Shtylyov, David S. Miller, Rob Herring,
Andrew Lunn, Andy Shevchenko, Dmitry Torokhov, netdev,
linux-kernel
Cc: Bence Csókás, Csaba Buday, Heiner Kallweit,
Russell King, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
devm_gpiod_get_optional() in favor of the non-devres managed
fwnode_get_named_gpiod(). When it was kind-of reverted by commit
40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
functionality was not reinstated. Nor was the GPIO unclaimed on device
remove. This leads to the GPIO being claimed indefinitely, even when the
device and/or the driver gets removed.
Fixes: bafbdd527d56 ("phylib: Add device reset GPIO support")
Fixes: 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()")
Cc: Csaba Buday <buday.csaba@prolan.hu>
Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---
drivers/net/phy/mdio_bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fda2e27c1810..24bdab5bdd24 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -36,8 +36,8 @@
static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
{
/* Deassert the optional reset signal */
- mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
- "reset", GPIOD_OUT_LOW);
+ mdiodev->reset_gpio = devm_gpiod_get_optional(&mdiodev->dev,
+ "reset", GPIOD_OUT_LOW);
if (IS_ERR(mdiodev->reset_gpio))
return PTR_ERR(mdiodev->reset_gpio);
base-commit: fa582ca7e187a15e772e6a72fe035f649b387a60
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-28 15:34 [PATCH net] net: mdio_bus: Use devm for getting reset GPIO Bence Csókás
@ 2025-07-31 1:16 ` Jakub Kicinski
2025-07-31 1:20 ` Andrew Lunn
2025-08-07 8:12 ` Csókás Bence
2025-07-31 2:20 ` patchwork-bot+netdevbpf
2025-08-01 12:01 ` Mark Brown
2 siblings, 2 replies; 11+ messages in thread
From: Jakub Kicinski @ 2025-07-31 1:16 UTC (permalink / raw)
To: Bence Csókás, Andrew Lunn
Cc: Geert Uytterhoeven, Sergei Shtylyov, David S. Miller, Rob Herring,
Andy Shevchenko, Dmitry Torokhov, netdev, linux-kernel,
Csaba Buday, Heiner Kallweit, Russell King, Eric Dumazet,
Paolo Abeni
On Mon, 28 Jul 2025 17:34:55 +0200 Bence Csókás wrote:
> Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> devm_gpiod_get_optional() in favor of the non-devres managed
> fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> functionality was not reinstated. Nor was the GPIO unclaimed on device
> remove. This leads to the GPIO being claimed indefinitely, even when the
> device and/or the driver gets removed.
>
> Fixes: bafbdd527d56 ("phylib: Add device reset GPIO support")
> Fixes: 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()")
> Cc: Csaba Buday <buday.csaba@prolan.hu>
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
Looks like this is a v2 / rewrite of
https://lore.kernel.org/all/20250709133222.48802-3-buday.csaba@prolan.hu/
? Please try to include more of a change log / history of the changes
(under the --- marker)
Andrew, you acked what I'm guessing was the v1, still looks good?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-31 1:16 ` Jakub Kicinski
@ 2025-07-31 1:20 ` Andrew Lunn
2025-08-07 8:12 ` Csókás Bence
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2025-07-31 1:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Bence Csókás, Geert Uytterhoeven, Sergei Shtylyov,
David S. Miller, Rob Herring, Andy Shevchenko, Dmitry Torokhov,
netdev, linux-kernel, Csaba Buday, Heiner Kallweit, Russell King,
Eric Dumazet, Paolo Abeni
> Andrew, you acked what I'm guessing was the v1, still looks good?
Yes.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-28 15:34 [PATCH net] net: mdio_bus: Use devm for getting reset GPIO Bence Csókás
2025-07-31 1:16 ` Jakub Kicinski
@ 2025-07-31 2:20 ` patchwork-bot+netdevbpf
2025-08-01 12:34 ` Russell King (Oracle)
2025-08-01 12:01 ` Mark Brown
2 siblings, 1 reply; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-07-31 2:20 UTC (permalink / raw)
To: =?utf-8?b?QmVuY2UgQ3PDs2vDoXMgPGNzb2thcy5iZW5jZUBwcm9sYW4uaHU+?=
Cc: geert+renesas, sergei.shtylyov, davem, robh, andrew,
andriy.shevchenko, dmitry.torokhov, netdev, linux-kernel,
buday.csaba, hkallweit1, linux, edumazet, kuba, pabeni
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 28 Jul 2025 17:34:55 +0200 you wrote:
> Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> devm_gpiod_get_optional() in favor of the non-devres managed
> fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> functionality was not reinstated. Nor was the GPIO unclaimed on device
> remove. This leads to the GPIO being claimed indefinitely, even when the
> device and/or the driver gets removed.
>
> [...]
Here is the summary with links:
- [net] net: mdio_bus: Use devm for getting reset GPIO
https://git.kernel.org/netdev/net/c/3b98c9352511
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-28 15:34 [PATCH net] net: mdio_bus: Use devm for getting reset GPIO Bence Csókás
2025-07-31 1:16 ` Jakub Kicinski
2025-07-31 2:20 ` patchwork-bot+netdevbpf
@ 2025-08-01 12:01 ` Mark Brown
2025-08-01 12:25 ` Geert Uytterhoeven
2 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2025-08-01 12:01 UTC (permalink / raw)
To: Bence Csókás
Cc: Geert Uytterhoeven, Sergei Shtylyov, David S. Miller, Rob Herring,
Andrew Lunn, Andy Shevchenko, Dmitry Torokhov, netdev,
linux-kernel, Csaba Buday, Heiner Kallweit, Russell King,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
[-- Attachment #1: Type: text/plain, Size: 7335 bytes --]
On Mon, Jul 28, 2025 at 05:34:55PM +0200, Bence Csókás wrote:
> Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> devm_gpiod_get_optional() in favor of the non-devres managed
> fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> functionality was not reinstated. Nor was the GPIO unclaimed on device
> remove. This leads to the GPIO being claimed indefinitely, even when the
> device and/or the driver gets removed.
I'm seeing multiple platforms including at least Beaglebone Black,
Tordax Mallow and Libre Computer Alta printing errors in
next/pending-fixes today:
[ 3.252885] mdio_bus 4a101000.mdio:00: Resources present before probing
Bisects are pointing to this patch which is 3b98c9352511db in -next,
full log for Beaglebone at:
https://lava.sirena.org.uk/scheduler/job/1627658
and Mallow at:
https://lava.sirena.org.uk/scheduler/job/1627696
and a bisect log (this one for Beaglebone):
# bad: [02694a9281c9b3da7f7574f9f39a69cc70e344ce] Merge branch 'dt/linus' of git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
# good: [f2d282e1dfb3d8cb95b5ccdea43f2411f27201db] Merge tag 'bitmap-for-6.17' of https://github.com/norov/linux
# good: [50a479527ef01f9b36dde1803a7e81741a222509] ASoC: SDCA: Add support for -cn- value properties
# good: [10dfd36f078423c51602a9a21ed85e8e6c947a00] regulator: core: correct convergence check in regulator_set_voltage()
# good: [11f74f48c14c1f4fe16541900ea5944c42e30ccf] ASoC: Intel: avs: Fix uninitialized pointer error in probe()
# good: [ca592e20659e0304ebd8f4dabb273da4f9385848] ASoC: fsl_xcvr: get channel status data when PHY is not exists
# good: [a735ee58c0d673d630a10ac2939dccb54df0622a] spi: cs42l43: Property entry should be a null-terminated array
# good: [061fade7a67f6cdfe918a675270d84107abbef61] ASoC: SDCA: Fix some holes in the regmap readable/writeable helpers
# good: [eb3bb145280b6c857a748731a229698e4a7cf37b] ASoC: SOF: amd: acp-loader: Use GFP_KERNEL for DMA allocations in resume context
# good: [e95122a32e777309412e30dc638dbc88b9036811] ASoC: codecs: Add acpi_match_table for aw88399 driver
# good: [da98e8b97058c73b5c58e9976af2e7286f1c799b] ASoC: dt-bindings: atmel,at91-ssc: add microchip,sam9x7-ssc
# good: [926406a85ad895fbe6ee4577cdbc4f55245a0742] MAINTAINERS: Add entries for the RZ/V2H(P) RSPI
# good: [8d452accd1380e1cb0b15a9876bcd19b14c5fabb] ASoC: wm8962: Clear master mode when enter runtime suspend
# good: [7379907e241d85803efc1d9eb27c28a6322e274f] ASoC: fsl_xcvr: get channel status data in two cases
# good: [2260bc6ea8bd57aec92cbda770de9cc95232f64d] ASoC: imx-card: Add WM8524 support
# good: [6776ecc9dd587c08a6bb334542f9f8821a091013] ASoC: fsl_xcvr: get channel status data with firmware exists
# good: [1032fa556c37c500bf2b93d95fa18e7d1fd1b4de] More minor SDCA changes
git bisect start '02694a9281c9b3da7f7574f9f39a69cc70e344ce' 'f2d282e1dfb3d8cb95b5ccdea43f2411f27201db' '50a479527ef01f9b36dde1803a7e81741a222509' '10dfd36f078423c51602a9a21ed85e8e6c947a00' '11f74f48c14c1f4fe16541900ea5944c42e30ccf' 'ca592e20659e0304ebd8f4dabb273da4f9385848' 'a735ee58c0d673d630a10ac2939dccb54df0622a' '061fade7a67f6cdfe918a675270d84107abbef61' 'eb3bb145280b6c857a748731a229698e4a7cf37b' 'e95122a32e777309412e30dc638dbc88b9036811' 'da98e8b97058c73b5c58e9976af2e7286f1c799b' '926406a85ad895fbe6ee4577cdbc4f55245a0742' '8d452accd1380e1cb0b15a9876bcd19b14c5fabb' '7379907e241d85803efc1d9eb27c28a6322e274f' '2260bc6ea8bd57aec92cbda770de9cc95232f64d' '6776ecc9dd587c08a6bb334542f9f8821a091013' '1032fa556c37c500bf2b93d95fa18e7d1fd1b4de'
# test job: [50a479527ef01f9b36dde1803a7e81741a222509] https://lava.sirena.org.uk/scheduler/job/1604113
# test job: [10dfd36f078423c51602a9a21ed85e8e6c947a00] https://lava.sirena.org.uk/scheduler/job/1617510
# test job: [11f74f48c14c1f4fe16541900ea5944c42e30ccf] https://lava.sirena.org.uk/scheduler/job/1622131
# test job: [ca592e20659e0304ebd8f4dabb273da4f9385848] https://lava.sirena.org.uk/scheduler/job/1604636
# test job: [a735ee58c0d673d630a10ac2939dccb54df0622a] https://lava.sirena.org.uk/scheduler/job/1625798
# test job: [061fade7a67f6cdfe918a675270d84107abbef61] https://lava.sirena.org.uk/scheduler/job/1604012
# test job: [eb3bb145280b6c857a748731a229698e4a7cf37b] https://lava.sirena.org.uk/scheduler/job/1614504
# test job: [e95122a32e777309412e30dc638dbc88b9036811] https://lava.sirena.org.uk/scheduler/job/1607730
# test job: [da98e8b97058c73b5c58e9976af2e7286f1c799b] https://lava.sirena.org.uk/scheduler/job/1604614
# test job: [926406a85ad895fbe6ee4577cdbc4f55245a0742] https://lava.sirena.org.uk/scheduler/job/1617649
# test job: [8d452accd1380e1cb0b15a9876bcd19b14c5fabb] https://lava.sirena.org.uk/scheduler/job/1621163
# test job: [7379907e241d85803efc1d9eb27c28a6322e274f] https://lava.sirena.org.uk/scheduler/job/1605837
# test job: [2260bc6ea8bd57aec92cbda770de9cc95232f64d] https://lava.sirena.org.uk/scheduler/job/1604642
# test job: [6776ecc9dd587c08a6bb334542f9f8821a091013] https://lava.sirena.org.uk/scheduler/job/1604670
# test job: [1032fa556c37c500bf2b93d95fa18e7d1fd1b4de] https://lava.sirena.org.uk/scheduler/job/1605715
# test job: [02694a9281c9b3da7f7574f9f39a69cc70e344ce] https://lava.sirena.org.uk/scheduler/job/1627658
# bad: [02694a9281c9b3da7f7574f9f39a69cc70e344ce] Merge branch 'dt/linus' of git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
git bisect bad 02694a9281c9b3da7f7574f9f39a69cc70e344ce
# test job: [4889166d1aaa4761f1162e01487b129aad7ef6a6] https://lava.sirena.org.uk/scheduler/job/1627868
# bad: [4889166d1aaa4761f1162e01487b129aad7ef6a6] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
git bisect bad 4889166d1aaa4761f1162e01487b129aad7ef6a6
# test job: [29c349380205ced75f66c0ccab821d00ff50d123] https://lava.sirena.org.uk/scheduler/job/1627932
# good: [29c349380205ced75f66c0ccab821d00ff50d123] Merge branch 'arm/fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git
git bisect good 29c349380205ced75f66c0ccab821d00ff50d123
# test job: [2da4def0f487f24bbb0cece3bb2bcdcb918a0b72] https://lava.sirena.org.uk/scheduler/job/1628009
# good: [2da4def0f487f24bbb0cece3bb2bcdcb918a0b72] netpoll: prevent hanging NAPI when netcons gets enabled
git bisect good 2da4def0f487f24bbb0cece3bb2bcdcb918a0b72
# test job: [3b98c9352511db627b606477fc7944b2fa53a165] https://lava.sirena.org.uk/scheduler/job/1628132
# bad: [3b98c9352511db627b606477fc7944b2fa53a165] net: mdio_bus: Use devm for getting reset GPIO
git bisect bad 3b98c9352511db627b606477fc7944b2fa53a165
# test job: [f2aa00e4f65efcf25ff6bc8198e21f031e7b9b1b] https://lava.sirena.org.uk/scheduler/job/1628177
# good: [f2aa00e4f65efcf25ff6bc8198e21f031e7b9b1b] net: ipa: add IPA v5.1 and v5.5 to ipa_version_string()
git bisect good f2aa00e4f65efcf25ff6bc8198e21f031e7b9b1b
# test job: [57ec5a8735dc5dccd1ee68afdb1114956a3fce0d] https://lava.sirena.org.uk/scheduler/job/1628433
# good: [57ec5a8735dc5dccd1ee68afdb1114956a3fce0d] net: phy: smsc: add proper reset flags for LAN8710A
git bisect good 57ec5a8735dc5dccd1ee68afdb1114956a3fce0d
# first bad commit: [3b98c9352511db627b606477fc7944b2fa53a165] net: mdio_bus: Use devm for getting reset GPIO
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-08-01 12:01 ` Mark Brown
@ 2025-08-01 12:25 ` Geert Uytterhoeven
2025-08-01 12:33 ` Russell King (Oracle)
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2025-08-01 12:25 UTC (permalink / raw)
To: Mark Brown
Cc: Bence Csókás, Sergei Shtylyov, David S. Miller,
Rob Herring, Andrew Lunn, Andy Shevchenko, Dmitry Torokhov,
netdev, linux-kernel, Csaba Buday, Heiner Kallweit, Russell King,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Hi Mark,
On Fri, 1 Aug 2025 at 14:01, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 28, 2025 at 05:34:55PM +0200, Bence Csókás wrote:
> > Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> > devm_gpiod_get_optional() in favor of the non-devres managed
> > fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> > 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> > functionality was not reinstated. Nor was the GPIO unclaimed on device
> > remove. This leads to the GPIO being claimed indefinitely, even when the
> > device and/or the driver gets removed.
>
> I'm seeing multiple platforms including at least Beaglebone Black,
> Tordax Mallow and Libre Computer Alta printing errors in
> next/pending-fixes today:
>
> [ 3.252885] mdio_bus 4a101000.mdio:00: Resources present before probing
>
> Bisects are pointing to this patch which is 3b98c9352511db in -next,
My guess is that &mdiodev->dev is not the correct device for
resource management.
We had a similar issue before with non-GPIO resets, cfr. commit
32085f25d7b68404 ("mdio_bus: don't use managed reset-controller").
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-08-01 12:25 ` Geert Uytterhoeven
@ 2025-08-01 12:33 ` Russell King (Oracle)
2025-08-01 13:04 ` Csókás Bence
0 siblings, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2025-08-01 12:33 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Mark Brown, Bence Csókás, Sergei Shtylyov,
David S. Miller, Rob Herring, Andrew Lunn, Andy Shevchenko,
Dmitry Torokhov, netdev, linux-kernel, Csaba Buday,
Heiner Kallweit, Eric Dumazet, Jakub Kicinski, Paolo Abeni
On Fri, Aug 01, 2025 at 02:25:17PM +0200, Geert Uytterhoeven wrote:
> Hi Mark,
>
> On Fri, 1 Aug 2025 at 14:01, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jul 28, 2025 at 05:34:55PM +0200, Bence Csókás wrote:
> > > Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> > > devm_gpiod_get_optional() in favor of the non-devres managed
> > > fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> > > 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> > > functionality was not reinstated. Nor was the GPIO unclaimed on device
> > > remove. This leads to the GPIO being claimed indefinitely, even when the
> > > device and/or the driver gets removed.
> >
> > I'm seeing multiple platforms including at least Beaglebone Black,
> > Tordax Mallow and Libre Computer Alta printing errors in
> > next/pending-fixes today:
> >
> > [ 3.252885] mdio_bus 4a101000.mdio:00: Resources present before probing
> >
> > Bisects are pointing to this patch which is 3b98c9352511db in -next,
>
> My guess is that &mdiodev->dev is not the correct device for
> resource management.
No, looking at the patch, the patch is completely wrong.
Take for example mdiobus_register_gpiod(). Using devm_*() there is
completely wrong, because this is called from mdiobus_register_device().
This is not the probe function for the device, and thus there is no
code to trigger the release of the resource on unregistration.
Moreover, when the mdiodev is eventually probed, if the driver fails
or the driver is unbound, the GPIO will be released, but a reference
will be left behind.
Using devm* with a struct device that is *not* currently being probed
is fundamentally wrong - an abuse of devm.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-31 2:20 ` patchwork-bot+netdevbpf
@ 2025-08-01 12:34 ` Russell King (Oracle)
0 siblings, 0 replies; 11+ messages in thread
From: Russell King (Oracle) @ 2025-08-01 12:34 UTC (permalink / raw)
To: patchwork-bot+netdevbpf
Cc: =?utf-8?b?QmVuY2UgQ3PDs2vDoXMgPGNzb2thcy5iZW5jZUBwcm9sYW4uaHU+?=,
geert+renesas, sergei.shtylyov, davem, robh, andrew,
andriy.shevchenko, dmitry.torokhov, netdev, linux-kernel,
buday.csaba, hkallweit1, edumazet, kuba, pabeni
On Thu, Jul 31, 2025 at 02:20:08AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
>
> This patch was applied to netdev/net.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Mon, 28 Jul 2025 17:34:55 +0200 you wrote:
> > Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> > devm_gpiod_get_optional() in favor of the non-devres managed
> > fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> > 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> > functionality was not reinstated. Nor was the GPIO unclaimed on device
> > remove. This leads to the GPIO being claimed indefinitely, even when the
> > device and/or the driver gets removed.
> >
> > [...]
>
> Here is the summary with links:
> - [net] net: mdio_bus: Use devm for getting reset GPIO
> https://git.kernel.org/netdev/net/c/3b98c9352511
This needs to be reverted, it's an abuse of devm on a device that is
not being probed. Sorry, I don't have time to generate a patch.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-08-01 12:33 ` Russell King (Oracle)
@ 2025-08-01 13:04 ` Csókás Bence
2025-08-01 13:15 ` Russell King (Oracle)
0 siblings, 1 reply; 11+ messages in thread
From: Csókás Bence @ 2025-08-01 13:04 UTC (permalink / raw)
To: Russell King (Oracle), Geert Uytterhoeven
Cc: Mark Brown, Sergei Shtylyov, David S. Miller, Rob Herring,
Andrew Lunn, Andy Shevchenko, Dmitry Torokhov, netdev,
linux-kernel, Csaba Buday, Heiner Kallweit, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Hi,
On 2025. 08. 01. 14:33, Russell King (Oracle) wrote:
> On Fri, Aug 01, 2025 at 02:25:17PM +0200, Geert Uytterhoeven wrote:
>> Hi Mark,
>>
>> On Fri, 1 Aug 2025 at 14:01, Mark Brown <broonie@kernel.org> wrote:
>>> On Mon, Jul 28, 2025 at 05:34:55PM +0200, Bence Csókás wrote:
>>>> Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
>>>> devm_gpiod_get_optional() in favor of the non-devres managed
>>>> fwnode_get_named_gpiod(). When it was kind-of reverted by commit
>>>> 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
>>>> functionality was not reinstated. Nor was the GPIO unclaimed on device
>>>> remove. This leads to the GPIO being claimed indefinitely, even when the
>>>> device and/or the driver gets removed.
>>>
>>> I'm seeing multiple platforms including at least Beaglebone Black,
>>> Tordax Mallow and Libre Computer Alta printing errors in
>>> next/pending-fixes today:
>>>
>>> [ 3.252885] mdio_bus 4a101000.mdio:00: Resources present before probing
>>>
>>> Bisects are pointing to this patch which is 3b98c9352511db in -next,
>>
>> My guess is that &mdiodev->dev is not the correct device for
>> resource management.
>
> No, looking at the patch, the patch is completely wrong.
>
> Take for example mdiobus_register_gpiod(). Using devm_*() there is
> completely wrong, because this is called from mdiobus_register_device().
> This is not the probe function for the device, and thus there is no
> code to trigger the release of the resource on unregistration.
>
> Moreover, when the mdiodev is eventually probed, if the driver fails
> or the driver is unbound, the GPIO will be released, but a reference
> will be left behind.
>
> Using devm* with a struct device that is *not* currently being probed
> is fundamentally wrong - an abuse of devm.
The real question is: why on Earth is mdiobus_register_device() called
_before_ the probe()? And mdiobus_unregister_device() after the remove()???
Anyways, in this case we could probably put the release of the GPIO into
mdiobus_unregister_device() instead. But this inverted logic should
probably be dealt with eventually.
Bence
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-08-01 13:04 ` Csókás Bence
@ 2025-08-01 13:15 ` Russell King (Oracle)
0 siblings, 0 replies; 11+ messages in thread
From: Russell King (Oracle) @ 2025-08-01 13:15 UTC (permalink / raw)
To: Csókás Bence
Cc: Geert Uytterhoeven, Mark Brown, Sergei Shtylyov, David S. Miller,
Rob Herring, Andrew Lunn, Andy Shevchenko, Dmitry Torokhov,
netdev, linux-kernel, Csaba Buday, Heiner Kallweit, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
On Fri, Aug 01, 2025 at 03:04:31PM +0200, Csókás Bence wrote:
> Hi,
>
> On 2025. 08. 01. 14:33, Russell King (Oracle) wrote:
> > On Fri, Aug 01, 2025 at 02:25:17PM +0200, Geert Uytterhoeven wrote:
> > > Hi Mark,
> > >
> > > On Fri, 1 Aug 2025 at 14:01, Mark Brown <broonie@kernel.org> wrote:
> > > > On Mon, Jul 28, 2025 at 05:34:55PM +0200, Bence Csókás wrote:
> > > > > Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
> > > > > devm_gpiod_get_optional() in favor of the non-devres managed
> > > > > fwnode_get_named_gpiod(). When it was kind-of reverted by commit
> > > > > 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
> > > > > functionality was not reinstated. Nor was the GPIO unclaimed on device
> > > > > remove. This leads to the GPIO being claimed indefinitely, even when the
> > > > > device and/or the driver gets removed.
> > > >
> > > > I'm seeing multiple platforms including at least Beaglebone Black,
> > > > Tordax Mallow and Libre Computer Alta printing errors in
> > > > next/pending-fixes today:
> > > >
> > > > [ 3.252885] mdio_bus 4a101000.mdio:00: Resources present before probing
> > > >
> > > > Bisects are pointing to this patch which is 3b98c9352511db in -next,
> > >
> > > My guess is that &mdiodev->dev is not the correct device for
> > > resource management.
> >
> > No, looking at the patch, the patch is completely wrong.
> >
> > Take for example mdiobus_register_gpiod(). Using devm_*() there is
> > completely wrong, because this is called from mdiobus_register_device().
> > This is not the probe function for the device, and thus there is no
> > code to trigger the release of the resource on unregistration.
> >
> > Moreover, when the mdiodev is eventually probed, if the driver fails
> > or the driver is unbound, the GPIO will be released, but a reference
> > will be left behind.
> >
> > Using devm* with a struct device that is *not* currently being probed
> > is fundamentally wrong - an abuse of devm.
>
> The real question is: why on Earth is mdiobus_register_device() called
> _before_ the probe()?
Please review the code and *understand* it before making changes. This
is what any experienced programmer will do, so please get into that
habbit - it'll help you not to get a bad name in the kernel community.
If you don't understand that mdiobus_register_device() would be called
outside of the device's probe function, then you need to gain that
knowledge through research.
Please treat this as a learning exercise.
First step: grep -r mdiobus_register_device drivers/net/phy
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] net: mdio_bus: Use devm for getting reset GPIO
2025-07-31 1:16 ` Jakub Kicinski
2025-07-31 1:20 ` Andrew Lunn
@ 2025-08-07 8:12 ` Csókás Bence
1 sibling, 0 replies; 11+ messages in thread
From: Csókás Bence @ 2025-08-07 8:12 UTC (permalink / raw)
To: Jakub Kicinski, Andrew Lunn
Cc: Geert Uytterhoeven, Sergei Shtylyov, David S. Miller, Rob Herring,
Andy Shevchenko, Dmitry Torokhov, netdev, linux-kernel,
Csaba Buday, Heiner Kallweit, Russell King, Eric Dumazet,
Paolo Abeni, Geert Uytterhoeven, Mark Brown
Hi,
On 2025. 07. 31. 3:16, Jakub Kicinski wrote:
> On Mon, 28 Jul 2025 17:34:55 +0200 Bence Csókás wrote:
>> Commit bafbdd527d56 ("phylib: Add device reset GPIO support") removed
>> devm_gpiod_get_optional() in favor of the non-devres managed
>> fwnode_get_named_gpiod(). When it was kind-of reverted by commit
>> 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()"), the devm
>> functionality was not reinstated. Nor was the GPIO unclaimed on device
>> remove. This leads to the GPIO being claimed indefinitely, even when the
>> device and/or the driver gets removed.
>>
>> Fixes: bafbdd527d56 ("phylib: Add device reset GPIO support")
>> Fixes: 40ba6a12a548 ("net: mdio: switch to using gpiod_get_optional()")
>> Cc: Csaba Buday <buday.csaba@prolan.hu>
>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
>
> Looks like this is a v2 / rewrite of
> https://lore.kernel.org/all/20250709133222.48802-3-buday.csaba@prolan.hu/
> ? Please try to include more of a change log / history of the changes
> (under the --- marker)
I talked with Csaba, and now I realize I shouldn't have modified the
original. I will resubmit that instead.
Bence
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-08-07 8:12 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-28 15:34 [PATCH net] net: mdio_bus: Use devm for getting reset GPIO Bence Csókás
2025-07-31 1:16 ` Jakub Kicinski
2025-07-31 1:20 ` Andrew Lunn
2025-08-07 8:12 ` Csókás Bence
2025-07-31 2:20 ` patchwork-bot+netdevbpf
2025-08-01 12:34 ` Russell King (Oracle)
2025-08-01 12:01 ` Mark Brown
2025-08-01 12:25 ` Geert Uytterhoeven
2025-08-01 12:33 ` Russell King (Oracle)
2025-08-01 13:04 ` Csókás Bence
2025-08-01 13:15 ` Russell King (Oracle)
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).