LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net v2 0/3] Clean up and fix error handling in mdio_mux_init()
@ 2021-08-17 18:08 Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree Saravana Kannan
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Saravana Kannan @ 2021-08-17 18:08 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Jakub Kicinski
  Cc: Saravana Kannan, Marc Zyngier, Neil Armstrong, Kevin Hilman,
	kernel-team, netdev, linux-kernel

This patch series was started due to -EPROBE_DEFER not being handled
correctly in mdio_mux_init() and causing issues [1]. While at it, I also
did some more error handling fixes and clean ups. The -EPROBE_DEFER fix is
the last patch.

Ideally, in the last patch we'd treat any error similar to -EPROBE_DEFER
but I'm not sure if it'll break any board/platforms where some child
mdiobus never successfully registers. If we treated all errors similar to
-EPROBE_DEFER, then none of the child mdiobus will work and that might be a
regression. If people are sure this is not a real case, then I can fix up
the last patch to always fail the entire mdio-mux init if any of the child
mdiobus registration fails.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Kevin Hilman <khilman@baylibre.com>
[1] - https://lore.kernel.org/lkml/CAGETcx95kHrv8wA-O+-JtfH7H9biJEGJtijuPVN0V5dUKUAB3A@mail.gmail.com/#t

v1 -> v2:
- Added Acked-by, Tested-by and Reviewed-by
- Fixing the subject so it goes to "net" tree

Saravana Kannan (3):
  net: mdio-mux: Delete unnecessary devm_kfree
  net: mdio-mux: Don't ignore memory allocation errors
  net: mdio-mux: Handle -EPROBE_DEFER correctly

 drivers/net/mdio/mdio-mux.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree
  2021-08-17 18:08 [PATCH net v2 0/3] Clean up and fix error handling in mdio_mux_init() Saravana Kannan
@ 2021-08-17 18:08 ` Saravana Kannan
  2021-08-17 21:10   ` Andrew Lunn
  2021-08-17 18:08 ` [PATCH net v2 2/3] net: mdio-mux: Don't ignore memory allocation errors Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 3/3] net: mdio-mux: Handle -EPROBE_DEFER correctly Saravana Kannan
  2 siblings, 1 reply; 7+ messages in thread
From: Saravana Kannan @ 2021-08-17 18:08 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Jakub Kicinski
  Cc: Saravana Kannan, Marc Zyngier, Neil Armstrong, Kevin Hilman,
	kernel-team, netdev, linux-kernel

The whole point of devm_* APIs is that you don't have to undo them if you
are returning an error that's going to get propagated out of a probe()
function. So delete unnecessary devm_kfree() call in the error return path.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>
Acked-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/net/mdio/mdio-mux.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
index 110e4ee85785..5b37284f54d6 100644
--- a/drivers/net/mdio/mdio-mux.c
+++ b/drivers/net/mdio/mdio-mux.c
@@ -181,7 +181,6 @@ int mdio_mux_init(struct device *dev,
 	}
 
 	dev_err(dev, "Error: No acceptable child buses found\n");
-	devm_kfree(dev, pb);
 err_pb_kz:
 	put_device(&parent_bus->dev);
 err_parent_bus:
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net v2 2/3] net: mdio-mux: Don't ignore memory allocation errors
  2021-08-17 18:08 [PATCH net v2 0/3] Clean up and fix error handling in mdio_mux_init() Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree Saravana Kannan
@ 2021-08-17 18:08 ` Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 3/3] net: mdio-mux: Handle -EPROBE_DEFER correctly Saravana Kannan
  2 siblings, 0 replies; 7+ messages in thread
From: Saravana Kannan @ 2021-08-17 18:08 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Jakub Kicinski
  Cc: Saravana Kannan, Marc Zyngier, Neil Armstrong, Kevin Hilman,
	kernel-team, netdev, linux-kernel

If we are seeing memory allocation errors, don't try to continue
registering child mdiobus devices. It's unlikely they'll succeed.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>
Acked-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/net/mdio/mdio-mux.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
index 5b37284f54d6..13035e2685c4 100644
--- a/drivers/net/mdio/mdio-mux.c
+++ b/drivers/net/mdio/mdio-mux.c
@@ -82,6 +82,17 @@ static int mdio_mux_write(struct mii_bus *bus, int phy_id,
 
 static int parent_count;
 
+static void mdio_mux_uninit_children(struct mdio_mux_parent_bus *pb)
+{
+	struct mdio_mux_child_bus *cb = pb->children;
+
+	while (cb) {
+		mdiobus_unregister(cb->mii_bus);
+		mdiobus_free(cb->mii_bus);
+		cb = cb->next;
+	}
+}
+
 int mdio_mux_init(struct device *dev,
 		  struct device_node *mux_node,
 		  int (*switch_fn)(int cur, int desired, void *data),
@@ -144,7 +155,7 @@ int mdio_mux_init(struct device *dev,
 		cb = devm_kzalloc(dev, sizeof(*cb), GFP_KERNEL);
 		if (!cb) {
 			ret_val = -ENOMEM;
-			continue;
+			goto err_loop;
 		}
 		cb->bus_number = v;
 		cb->parent = pb;
@@ -152,8 +163,7 @@ int mdio_mux_init(struct device *dev,
 		cb->mii_bus = mdiobus_alloc();
 		if (!cb->mii_bus) {
 			ret_val = -ENOMEM;
-			devm_kfree(dev, cb);
-			continue;
+			goto err_loop;
 		}
 		cb->mii_bus->priv = cb;
 
@@ -181,6 +191,10 @@ int mdio_mux_init(struct device *dev,
 	}
 
 	dev_err(dev, "Error: No acceptable child buses found\n");
+
+err_loop:
+	mdio_mux_uninit_children(pb);
+	of_node_put(child_bus_node);
 err_pb_kz:
 	put_device(&parent_bus->dev);
 err_parent_bus:
@@ -192,14 +206,8 @@ EXPORT_SYMBOL_GPL(mdio_mux_init);
 void mdio_mux_uninit(void *mux_handle)
 {
 	struct mdio_mux_parent_bus *pb = mux_handle;
-	struct mdio_mux_child_bus *cb = pb->children;
-
-	while (cb) {
-		mdiobus_unregister(cb->mii_bus);
-		mdiobus_free(cb->mii_bus);
-		cb = cb->next;
-	}
 
+	mdio_mux_uninit_children(pb);
 	put_device(&pb->mii_bus->dev);
 }
 EXPORT_SYMBOL_GPL(mdio_mux_uninit);
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net v2 3/3] net: mdio-mux: Handle -EPROBE_DEFER correctly
  2021-08-17 18:08 [PATCH net v2 0/3] Clean up and fix error handling in mdio_mux_init() Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree Saravana Kannan
  2021-08-17 18:08 ` [PATCH net v2 2/3] net: mdio-mux: Don't ignore memory allocation errors Saravana Kannan
@ 2021-08-17 18:08 ` Saravana Kannan
  2 siblings, 0 replies; 7+ messages in thread
From: Saravana Kannan @ 2021-08-17 18:08 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Jakub Kicinski
  Cc: Saravana Kannan, Marc Zyngier, Neil Armstrong, Kevin Hilman,
	kernel-team, netdev, linux-kernel

When registering mdiobus children, if we get an -EPROBE_DEFER, we shouldn't
ignore it and continue registering the rest of the mdiobus children. This
would permanently prevent the deferring child mdiobus from working instead
of reattempting it in the future. So, if a child mdiobus needs to be
reattempted in the future, defer the entire mdio-mux initialization.

This fixes the issue where PHYs sitting under the mdio-mux aren't
initialized correctly if the PHY's interrupt controller is not yet ready
when the mdio-mux is being probed. Additional context in the link below.

Link: https://lore.kernel.org/lkml/CAGETcx95kHrv8wA-O+-JtfH7H9biJEGJtijuPVN0V5dUKUAB3A@mail.gmail.com/#t
Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>
Acked-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/net/mdio/mdio-mux.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
index 13035e2685c4..ebd001f0eece 100644
--- a/drivers/net/mdio/mdio-mux.c
+++ b/drivers/net/mdio/mdio-mux.c
@@ -175,11 +175,15 @@ int mdio_mux_init(struct device *dev,
 		cb->mii_bus->write = mdio_mux_write;
 		r = of_mdiobus_register(cb->mii_bus, child_bus_node);
 		if (r) {
+			mdiobus_free(cb->mii_bus);
+			if (r == -EPROBE_DEFER) {
+				ret_val = r;
+				goto err_loop;
+			}
+			devm_kfree(dev, cb);
 			dev_err(dev,
 				"Error: Failed to register MDIO bus for child %pOF\n",
 				child_bus_node);
-			mdiobus_free(cb->mii_bus);
-			devm_kfree(dev, cb);
 		} else {
 			cb->next = pb->children;
 			pb->children = cb;
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree
  2021-08-17 18:08 ` [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree Saravana Kannan
@ 2021-08-17 21:10   ` Andrew Lunn
  2021-08-18  2:56     ` Saravana Kannan
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2021-08-17 21:10 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Heiner Kallweit, Russell King, David S. Miller, Jakub Kicinski,
	Marc Zyngier, Neil Armstrong, Kevin Hilman, kernel-team, netdev,
	linux-kernel

On Tue, Aug 17, 2021 at 11:08:39AM -0700, Saravana Kannan wrote:
> The whole point of devm_* APIs is that you don't have to undo them if you
> are returning an error that's going to get propagated out of a probe()
> function. So delete unnecessary devm_kfree() call in the error return path.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Tested-by: Marc Zyngier <maz@kernel.org>
> Acked-by: Kevin Hilman <khilman@baylibre.com>
> Tested-by: Kevin Hilman <khilman@baylibre.com>

Please add a Fixes: tag, since you want this in stable.

All three patches need fixes tags, possibly different for each patch?

       Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree
  2021-08-17 21:10   ` Andrew Lunn
@ 2021-08-18  2:56     ` Saravana Kannan
  2021-08-18 18:46       ` Andrew Lunn
  0 siblings, 1 reply; 7+ messages in thread
From: Saravana Kannan @ 2021-08-18  2:56 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Russell King, David S. Miller, Jakub Kicinski,
	Marc Zyngier, Neil Armstrong, Kevin Hilman, kernel-team, netdev,
	linux-kernel

On Tue, Aug 17, 2021 at 2:10 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 17, 2021 at 11:08:39AM -0700, Saravana Kannan wrote:
> > The whole point of devm_* APIs is that you don't have to undo them if you
> > are returning an error that's going to get propagated out of a probe()
> > function. So delete unnecessary devm_kfree() call in the error return path.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Acked-by: Marc Zyngier <maz@kernel.org>
> > Tested-by: Marc Zyngier <maz@kernel.org>
> > Acked-by: Kevin Hilman <khilman@baylibre.com>
> > Tested-by: Kevin Hilman <khilman@baylibre.com>
>
> Please add a Fixes: tag, since you want this in stable.
>
> All three patches need fixes tags, possibly different for each patch?

I generally ask for patches to be picked up by stable only if it fixes
a bug that puts the kernel in a bad state or if it fixes an issue
someone actually reported on the stable kernel. In this case, it's
just failing device probes in some cases and I didn't think that met
the bar for stable. But if you think they should, then that's fine by
me.

I'll send out v3 patches with Fixes. I'm fairly sure these issues were
present since the time mdio-mux was added. Hopefully v3 will be the
last version I have to send out :)

-Saravana

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree
  2021-08-18  2:56     ` Saravana Kannan
@ 2021-08-18 18:46       ` Andrew Lunn
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Lunn @ 2021-08-18 18:46 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Heiner Kallweit, Russell King, David S. Miller, Jakub Kicinski,
	Marc Zyngier, Neil Armstrong, Kevin Hilman, kernel-team, netdev,
	linux-kernel

On Tue, Aug 17, 2021 at 07:56:42PM -0700, Saravana Kannan wrote:
> On Tue, Aug 17, 2021 at 2:10 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Tue, Aug 17, 2021 at 11:08:39AM -0700, Saravana Kannan wrote:
> > > The whole point of devm_* APIs is that you don't have to undo them if you
> > > are returning an error that's going to get propagated out of a probe()
> > > function. So delete unnecessary devm_kfree() call in the error return path.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > Acked-by: Marc Zyngier <maz@kernel.org>
> > > Tested-by: Marc Zyngier <maz@kernel.org>
> > > Acked-by: Kevin Hilman <khilman@baylibre.com>
> > > Tested-by: Kevin Hilman <khilman@baylibre.com>
> >
> > Please add a Fixes: tag, since you want this in stable.
> >
> > All three patches need fixes tags, possibly different for each patch?
> 
> I generally ask for patches to be picked up by stable only if it fixes
> a bug that puts the kernel in a bad state

Actually, you asked for stable. You set the subject to

[PATCH net v3 0/3] Clean up and fix error

where [PATCH net ] means stable. If you think this is just ongoing
development work, use [PATCH net-next ]. Then the Fixes tags are
optional.

	Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-18 18:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-17 18:08 [PATCH net v2 0/3] Clean up and fix error handling in mdio_mux_init() Saravana Kannan
2021-08-17 18:08 ` [PATCH net v2 1/3] net: mdio-mux: Delete unnecessary devm_kfree Saravana Kannan
2021-08-17 21:10   ` Andrew Lunn
2021-08-18  2:56     ` Saravana Kannan
2021-08-18 18:46       ` Andrew Lunn
2021-08-17 18:08 ` [PATCH net v2 2/3] net: mdio-mux: Don't ignore memory allocation errors Saravana Kannan
2021-08-17 18:08 ` [PATCH net v2 3/3] net: mdio-mux: Handle -EPROBE_DEFER correctly Saravana Kannan

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).