| Message ID | 20191118181505.32298-1-marek.behun@nic.cz |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/1] mdio_bus: fix mdio_register_device when RESET_CONTROLLER is disabled | expand |
On Mon, Nov 18, 2019 at 07:15:05PM +0100, Marek Behún wrote: > When CONFIG_RESET_CONTROLLER is disabled, the > devm_reset_control_get_exclusive function returns -ENOTSUPP. This is not > handled in subsequent check and then the mdio device fails to probe. > > When CONFIG_RESET_CONTROLLER is enabled, its code checks in OF for reset > device, and since it is not present, returns -ENOENT. -ENOENT is handled. > Add -ENOTSUPP also. > > This happened to me when upgrading kernel on Turris Omnia. You either > have to enable CONFIG_RESET_CONTROLLER or use this patch. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Fixes: 71dd6c0dff51b ("net: phy: add support for reset-controller") This seems reasonable. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
From: Marek Behún <marek.behun@nic.cz> Date: Mon, 18 Nov 2019 19:15:05 +0100 > When CONFIG_RESET_CONTROLLER is disabled, the > devm_reset_control_get_exclusive function returns -ENOTSUPP. This is not > handled in subsequent check and then the mdio device fails to probe. > > When CONFIG_RESET_CONTROLLER is enabled, its code checks in OF for reset > device, and since it is not present, returns -ENOENT. -ENOENT is handled. > Add -ENOTSUPP also. > > This happened to me when upgrading kernel on Turris Omnia. You either > have to enable CONFIG_RESET_CONTROLLER or use this patch. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Fixes: 71dd6c0dff51b ("net: phy: add support for reset-controller") Applied and queued up for -stable.
On Mon, Nov 18, 2019 at 07:15:05PM +0100, Marek Behún wrote: > When CONFIG_RESET_CONTROLLER is disabled, the > devm_reset_control_get_exclusive function returns -ENOTSUPP. This is not > handled in subsequent check and then the mdio device fails to probe. > > When CONFIG_RESET_CONTROLLER is enabled, its code checks in OF for reset > device, and since it is not present, returns -ENOENT. -ENOENT is handled. > Add -ENOTSUPP also. > > This happened to me when upgrading kernel on Turris Omnia. You either > have to enable CONFIG_RESET_CONTROLLER or use this patch. In the long term prospective shouldn't it use reset_control_get_optional_exclusive() instead?
Hi all, On Tue, 19 Nov 2019, Andy Shevchenko wrote: > On Mon, Nov 18, 2019 at 07:15:05PM +0100, Marek Behún wrote: >> When CONFIG_RESET_CONTROLLER is disabled, the >> devm_reset_control_get_exclusive function returns -ENOTSUPP. This is not >> handled in subsequent check and then the mdio device fails to probe. >> >> When CONFIG_RESET_CONTROLLER is enabled, its code checks in OF for reset >> device, and since it is not present, returns -ENOENT. -ENOENT is handled. >> Add -ENOTSUPP also. >> >> This happened to me when upgrading kernel on Turris Omnia. You either >> have to enable CONFIG_RESET_CONTROLLER or use this patch. > > In the long term prospective shouldn't it use > reset_control_get_optional_exclusive() instead? I was thinking the same, hence I tried. Without a reset (i.e. the returned reset is now NULL instead of -ENOENT): WARNING: CPU: 1 PID: 120 at drivers/base/dd.c:519 really_probe+0xb0/0x314 Modules linked in: CPU: 1 PID: 120 Comm: kworker/1:2 Not tainted 5.4.0-rc8-koelsch-08348-gbc6931ed2139022b-dirty #615 Hardware name: Generic R-Car Gen2 (Flattened Device Tree) Workqueue: events deferred_probe_work_func [<c020e268>] (unwind_backtrace) from [<c020a428>] (show_stack+0x10/0x14) [<c020a428>] (show_stack) from [<c07a075c>] (dump_stack+0x88/0xa8) [<c07a075c>] (dump_stack) from [<c0220980>] (__warn+0xb8/0xd0) [<c0220980>] (__warn) from [<c0220a08>] (warn_slowpath_fmt+0x70/0x9c) [<c0220a08>] (warn_slowpath_fmt) from [<c05150b0>] (really_probe+0xb0/0x314) [<c05150b0>] (really_probe) from [<c0515598>] (driver_probe_device+0x13c/0x154) [<c0515598>] (driver_probe_device) from [<c0513784>] (bus_for_each_drv+0xa0/0xb4) [<c0513784>] (bus_for_each_drv) from [<c05153c0>] (__device_attach+0xac/0x124) [<c05153c0>] (__device_attach) from [<c05143f0>] (bus_probe_device+0x28/0x80) [<c05143f0>] (bus_probe_device) from [<c051227c>] (device_add+0x4d8/0x698) [<c051227c>] (device_add) from [<c0581110>] (phy_device_register+0x3c/0x74) [<c0581110>] (phy_device_register) from [<c0675efc>] (of_mdiobus_register_phy+0x144/0x17c) [<c0675efc>] (of_mdiobus_register_phy) from [<c06760f8>] (of_mdiobus_register+0x1c4/0x2d0) [<c06760f8>] (of_mdiobus_register) from [<c0589f18>] (sh_eth_drv_probe+0x778/0x8ac) [<c0589f18>] (sh_eth_drv_probe) from [<c0516ce8>] (platform_drv_probe+0x48/0x94) [<c0516ce8>] (platform_drv_probe) from [<c05151f8>] (really_probe+0x1f8/0x314) [<c05151f8>] (really_probe) from [<c0515598>] (driver_probe_device+0x13c/0x154) The difference with the non-optional case is that __devm_reset_control_get() registers a cleanup function if there's no error condition, even for NULL (which is futile, will send a patch). However, more importantly, mdiobus_register_reset() calls a devm_*() function on "&mdiodev->dev" ("mdio_bus ee700000.ethernet-ffffffff:01"), which is a different device than the one being probed (("ee700000.ethernet"), see also the callstack below). In fact "&mdiodev->dev" hasn't been probed yet, leading to the WARNING when it is probed later. [<c0582de8>] (mdiobus_register_device) from [<c05810e0>] (phy_device_register+0xc/0x74) [<c05810e0>] (phy_device_register) from [<c0675ef4>] (of_mdiobus_register_phy+0x144/0x17c) [<c0675ef4>] (of_mdiobus_register_phy) from [<c06760f0>] (of_mdiobus_register+0x1c4/0x2d0) [<c06760f0>] (of_mdiobus_register) from [<c0589f0c>] (sh_eth_drv_probe+0x778/0x8ac) [<c0589f0c>] (sh_eth_drv_probe) from [<c0516ce8>] (platform_drv_probe+0x48/0x94) Has commit 71dd6c0dff51b5f1 ("net: phy: add support for reset-controller") been tested with an actual reset present? Are Ethernet drivers not (no longer) allowed to register MDIO busses? Thanks! 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
> The difference with the non-optional case is that > __devm_reset_control_get() registers a cleanup function if there's > no error condition, even for NULL (which is futile, will send a patch). > > However, more importantly, mdiobus_register_reset() calls a devm_*() > function on "&mdiodev->dev" ("mdio_bus ee700000.ethernet-ffffffff:01"), > which is a different device than the one being probed > (("ee700000.ethernet"), see also the callstack below). > In fact "&mdiodev->dev" hasn't been probed yet, leading to the WARNING > when it is probed later. > > [<c0582de8>] (mdiobus_register_device) from [<c05810e0>] (phy_device_register+0xc/0x74) > [<c05810e0>] (phy_device_register) from [<c0675ef4>] (of_mdiobus_register_phy+0x144/0x17c) > [<c0675ef4>] (of_mdiobus_register_phy) from [<c06760f0>] (of_mdiobus_register+0x1c4/0x2d0) > [<c06760f0>] (of_mdiobus_register) from [<c0589f0c>] (sh_eth_drv_probe+0x778/0x8ac) > [<c0589f0c>] (sh_eth_drv_probe) from [<c0516ce8>] (platform_drv_probe+0x48/0x94) > > Has commit 71dd6c0dff51b5f1 ("net: phy: add support for > reset-controller") been tested with an actual reset present? > > Are Ethernet drivers not (no longer) allowed to register MDIO busses? That is not good. The devm_reset_control_get() call need replaces with an unmanaged version, and a call to reset_control_put() added to mdiobus_unregister_device(). David, could you look at this, it was a patch from you that added this. Andrew
Hello Andrew, On 11/21/19 3:08 AM, Andrew Lunn wrote: >> The difference with the non-optional case is that >> __devm_reset_control_get() registers a cleanup function if there's >> no error condition, even for NULL (which is futile, will send a patch). >> >> However, more importantly, mdiobus_register_reset() calls a devm_*() >> function on "&mdiodev->dev" ("mdio_bus ee700000.ethernet-ffffffff:01"), >> which is a different device than the one being probed >> (("ee700000.ethernet"), see also the callstack below). >> In fact "&mdiodev->dev" hasn't been probed yet, leading to the WARNING >> when it is probed later. >> >> [<c0582de8>] (mdiobus_register_device) from [<c05810e0>] (phy_device_register+0xc/0x74) >> [<c05810e0>] (phy_device_register) from [<c0675ef4>] (of_mdiobus_register_phy+0x144/0x17c) >> [<c0675ef4>] (of_mdiobus_register_phy) from [<c06760f0>] (of_mdiobus_register+0x1c4/0x2d0) >> [<c06760f0>] (of_mdiobus_register) from [<c0589f0c>] (sh_eth_drv_probe+0x778/0x8ac) >> [<c0589f0c>] (sh_eth_drv_probe) from [<c0516ce8>] (platform_drv_probe+0x48/0x94) >> >> Has commit 71dd6c0dff51b5f1 ("net: phy: add support for >> reset-controller") been tested with an actual reset present? I'm using it on a AR9132 board, however the mdio bus is probed before the ethernet driver, hence why i was not experiencing this misbehavior. >> >> Are Ethernet drivers not (no longer) allowed to register MDIO busses? > > That is not good. The devm_reset_control_get() call need replaces with > an unmanaged version, and a call to reset_control_put() added to > mdiobus_unregister_device(). > > David, could you look at this, it was a patch from you that added > this. I will prepare patches to fix this bug. Best wishes David > > Andrew >
Hi David, On Thu, Nov 21, 2019 at 9:38 AM David Bauer <mail@david-bauer.net> wrote: > On 11/21/19 3:08 AM, Andrew Lunn wrote: > >> The difference with the non-optional case is that > >> __devm_reset_control_get() registers a cleanup function if there's > >> no error condition, even for NULL (which is futile, will send a patch). > >> > >> However, more importantly, mdiobus_register_reset() calls a devm_*() > >> function on "&mdiodev->dev" ("mdio_bus ee700000.ethernet-ffffffff:01"), > >> which is a different device than the one being probed > >> (("ee700000.ethernet"), see also the callstack below). > >> In fact "&mdiodev->dev" hasn't been probed yet, leading to the WARNING > >> when it is probed later. > >> > >> [<c0582de8>] (mdiobus_register_device) from [<c05810e0>] (phy_device_register+0xc/0x74) > >> [<c05810e0>] (phy_device_register) from [<c0675ef4>] (of_mdiobus_register_phy+0x144/0x17c) > >> [<c0675ef4>] (of_mdiobus_register_phy) from [<c06760f0>] (of_mdiobus_register+0x1c4/0x2d0) > >> [<c06760f0>] (of_mdiobus_register) from [<c0589f0c>] (sh_eth_drv_probe+0x778/0x8ac) > >> [<c0589f0c>] (sh_eth_drv_probe) from [<c0516ce8>] (platform_drv_probe+0x48/0x94) > >> > >> Has commit 71dd6c0dff51b5f1 ("net: phy: add support for > >> reset-controller") been tested with an actual reset present? > > I'm using it on a AR9132 board, however the mdio bus is probed before the > ethernet driver, hence why i was not experiencing this misbehavior. Thank you, that explains it. > >> Are Ethernet drivers not (no longer) allowed to register MDIO busses? > > > > That is not good. The devm_reset_control_get() call need replaces with > > an unmanaged version, and a call to reset_control_put() added to > > mdiobus_unregister_device(). > > > > David, could you look at this, it was a patch from you that added > > this. > > I will prepare patches to fix this bug. Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 35876562e32a..c87cb8c0dac8 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -65,7 +65,8 @@ static int mdiobus_register_reset(struct mdio_device *mdiodev) reset = devm_reset_control_get_exclusive(&mdiodev->dev, "phy"); if (IS_ERR(reset)) { - if (PTR_ERR(reset) == -ENOENT || PTR_ERR(reset) == -ENOSYS) + if (PTR_ERR(reset) == -ENOENT || PTR_ERR(reset) == -ENOSYS || + PTR_ERR(reset) == -ENOTSUPP) reset = NULL; else return PTR_ERR(reset);
When CONFIG_RESET_CONTROLLER is disabled, the devm_reset_control_get_exclusive function returns -ENOTSUPP. This is not handled in subsequent check and then the mdio device fails to probe. When CONFIG_RESET_CONTROLLER is enabled, its code checks in OF for reset device, and since it is not present, returns -ENOENT. -ENOENT is handled. Add -ENOTSUPP also. This happened to me when upgrading kernel on Turris Omnia. You either have to enable CONFIG_RESET_CONTROLLER or use this patch. Signed-off-by: Marek Behún <marek.behun@nic.cz> Fixes: 71dd6c0dff51b ("net: phy: add support for reset-controller") Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/phy/mdio_bus.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)