* [PATCH] mdio: mux: fix device_node_continue.cocci warnings
@ 2017-05-12 14:54 Julia Lawall
2017-05-12 16:22 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2017-05-12 14:54 UTC (permalink / raw)
To: netdev, Jon Mason; +Cc: Andrew Lunn, Florian Fainelli, kbuild-all, linux-kernel
Device node iterators put the previous value of the index variable, so an
explicit put causes a double put.
In particular, of_mdiobus_register can fail before doing anything
interesting, so one could view it as a no-op from the reference count
point of view.
Generated by: scripts/coccinelle/iterators/device_node_continue.cocci
CC: Jon Mason <jon.mason@broadcom.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
tree:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
head: 8785ded64cfb68b8d8b2583c7c1fc611f99eabf2
commit: b60161668199ac62011c024adc9e66713b9554e7 [13999/14120] mdio: mux:
mdio-mux.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev,
if (r) {
mdiobus_free(cb->mii_bus);
devm_kfree(dev, cb);
- of_node_put(child_bus_node);
} else {
cb->next = pb->children;
pb->children = cb;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings 2017-05-12 14:54 [PATCH] mdio: mux: fix device_node_continue.cocci warnings Julia Lawall @ 2017-05-12 16:22 ` David Miller 2017-05-12 22:52 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2017-05-12 16:22 UTC (permalink / raw) To: julia.lawall Cc: netdev, jon.mason, andrew, f.fainelli, kbuild-all, linux-kernel From: Julia Lawall <julia.lawall@lip6.fr> Date: Fri, 12 May 2017 22:54:23 +0800 (SGT) > Device node iterators put the previous value of the index variable, so an > explicit put causes a double put. ... > @@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev, > if (r) { > mdiobus_free(cb->mii_bus); > devm_kfree(dev, cb); > - of_node_put(child_bus_node); > } else { I think we're instead simply missing a break; statement here. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings 2017-05-12 16:22 ` David Miller @ 2017-05-12 22:52 ` Florian Fainelli 2017-05-13 0:40 ` Julia Lawall 2017-05-15 17:37 ` Jon Mason 0 siblings, 2 replies; 6+ messages in thread From: Florian Fainelli @ 2017-05-12 22:52 UTC (permalink / raw) To: David Miller, julia.lawall, jon.mason Cc: netdev, jon.mason, andrew, kbuild-all, linux-kernel On 05/12/2017 09:22 AM, David Miller wrote: > From: Julia Lawall <julia.lawall@lip6.fr> > Date: Fri, 12 May 2017 22:54:23 +0800 (SGT) > >> Device node iterators put the previous value of the index variable, so an >> explicit put causes a double put. > ... >> @@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev, >> if (r) { >> mdiobus_free(cb->mii_bus); >> devm_kfree(dev, cb); >> - of_node_put(child_bus_node); >> } else { > > I think we're instead simply missing a break; statement here. It's kind of questionable, if we have an error initializing one of our child MDIO bus controller (child from the perspective of the MDIO mux, boy this is getting complicated...), should we keep on going, or should we abort entirely and rollback what we have successfully registered? I don't think Julia's patch makes thing worse, in that if we had to rollback, we would not be doing this correctly now anyway. Jon, what do you think? -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings 2017-05-12 22:52 ` Florian Fainelli @ 2017-05-13 0:40 ` Julia Lawall 2017-05-15 17:37 ` Jon Mason 1 sibling, 0 replies; 6+ messages in thread From: Julia Lawall @ 2017-05-13 0:40 UTC (permalink / raw) To: Florian Fainelli Cc: David Miller, jon.mason, netdev, andrew, kbuild-all, linux-kernel On Fri, 12 May 2017, Florian Fainelli wrote: > On 05/12/2017 09:22 AM, David Miller wrote: > > From: Julia Lawall <julia.lawall@lip6.fr> > > Date: Fri, 12 May 2017 22:54:23 +0800 (SGT) > > > >> Device node iterators put the previous value of the index variable, so an > >> explicit put causes a double put. > > ... > >> @@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev, > >> if (r) { > >> mdiobus_free(cb->mii_bus); > >> devm_kfree(dev, cb); > >> - of_node_put(child_bus_node); > >> } else { > > > > I think we're instead simply missing a break; statement here. > > It's kind of questionable, if we have an error initializing one of our > child MDIO bus controller (child from the perspective of the MDIO mux, > boy this is getting complicated...), should we keep on going, or should > we abort entirely and rollback what we have successfully registered? > > I don't think Julia's patch makes thing worse, in that if we had to > rollback, we would not be doing this correctly now anyway. Just to be clear, if you want the break instead, then you need to keep the put. julia > > Jon, what do you think? > -- > Florian > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings 2017-05-12 22:52 ` Florian Fainelli 2017-05-13 0:40 ` Julia Lawall @ 2017-05-15 17:37 ` Jon Mason 2017-05-15 18:29 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: Jon Mason @ 2017-05-15 17:37 UTC (permalink / raw) To: Florian Fainelli Cc: David Miller, Julia Lawall, Network Development, Andrew Lunn, kbuild-all, open list On Fri, May 12, 2017 at 6:52 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 05/12/2017 09:22 AM, David Miller wrote: >> From: Julia Lawall <julia.lawall@lip6.fr> >> Date: Fri, 12 May 2017 22:54:23 +0800 (SGT) >> >>> Device node iterators put the previous value of the index variable, so an >>> explicit put causes a double put. >> ... >>> @@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev, >>> if (r) { >>> mdiobus_free(cb->mii_bus); >>> devm_kfree(dev, cb); >>> - of_node_put(child_bus_node); >>> } else { >> >> I think we're instead simply missing a break; statement here. > > It's kind of questionable, if we have an error initializing one of our > child MDIO bus controller (child from the perspective of the MDIO mux, > boy this is getting complicated...), should we keep on going, or should > we abort entirely and rollback what we have successfully registered? > > I don't think Julia's patch makes thing worse, in that if we had to > rollback, we would not be doing this correctly now anyway. > > Jon, what do you think? If every other case is fatal, then it is odd that this one is permissive. I think we should go 100% one way or the other. So, the options here are to: 1. Encounter an error, unroll any mallocs, etc created by this entry, but continue on to the next entry and return success if any are created 2. Encounter an error, unroll any mallocs, etc created by this entry and any others that were created, and return an error 3. Encounter an error, unroll any mallocs, etc created by this entry, exit and return success if any are created #1 would be the most accepting of any errors encountered #2 would identify any poorly written DTs by breaking their currently working functionality (though we should add some error messages to let them know why) #3 matches the suggestion by David Miller, and would be a hybrid of #1 and #2 in outcome I would prefer #1, as I would not want to break something that was currently working. However, I think we should add much error logging here to let people know their DT is hosed (instead of silently working). So, this would mean applying Julia's patch, and I'll do a follow-on to change the breaks to continues and add the error logging (assuming others agree with me). Thanks, Jon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings 2017-05-15 17:37 ` Jon Mason @ 2017-05-15 18:29 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2017-05-15 18:29 UTC (permalink / raw) To: jon.mason Cc: f.fainelli, julia.lawall, netdev, andrew, kbuild-all, linux-kernel From: Jon Mason <jon.mason@broadcom.com> Date: Mon, 15 May 2017 13:37:09 -0400 > I would prefer #1, as I would not want to break something that was > currently working. However, I think we should add much error logging > here to let people know their DT is hosed (instead of silently > working). So, this would mean applying Julia's patch, and I'll do a > follow-on to change the breaks to continues and add the error logging > (assuming others agree with me). Ok, I've applied Julia's patch. I agree that we shouldn't fail the whole list just because one does. And yes, we should emit enough diagnostics so that people can figure out what the problem is. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-15 18:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-12 14:54 [PATCH] mdio: mux: fix device_node_continue.cocci warnings Julia Lawall 2017-05-12 16:22 ` David Miller 2017-05-12 22:52 ` Florian Fainelli 2017-05-13 0:40 ` Julia Lawall 2017-05-15 17:37 ` Jon Mason 2017-05-15 18:29 ` 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).