drivers/net/phy/mdio_bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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
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
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
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!
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
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!
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?
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
© 2016 - 2025 Red Hat, Inc.