| Message ID | 20240812190700.14270-3-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | use more devm | expand |
On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > Allows removing ag71xx_mdio_remove. > > Removed local mii_bus variable and assign struct members directly. > Easier to reason about. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> Reviewed-by: Daniel Golle <daniel@makrotopia.org>
On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > Allows removing ag71xx_mdio_remove. > > Removed local mii_bus variable and assign struct members directly. > Easier to reason about. This mixes up two different things, making the patch harder to review. Ideally you want lots of little patches, each doing one thing, and being obviously correct. Is ag->mii_bus actually used anywhere, outside of ag71xx_mdio_probe()? Often swapping to devm_ means the driver does not need to keep hold of the resources. So i actually think you can remove ag->mii_bus. This might of been more obvious if you had first swapped to devm_of_mdiobus_register() without the other changes mixed in. Andrew --- pw-bot: cr
On Mon, Aug 12, 2024 at 2:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > > Allows removing ag71xx_mdio_remove. > > > > Removed local mii_bus variable and assign struct members directly. > > Easier to reason about. > > This mixes up two different things, making the patch harder to > review. Ideally you want lots of little patches, each doing one thing, > and being obviously correct. > > Is ag->mii_bus actually used anywhere, outside of ag71xx_mdio_probe()? > Often swapping to devm_ means the driver does not need to keep hold of > the resources. So i actually think you can remove ag->mii_bus. This > might of been more obvious if you had first swapped to > devm_of_mdiobus_register() without the other changes mixed in. not sure I follow. mdiobus_unregister would need to be called in remove without devm. That would need a private mii_bus of some kind. So with devm this is unneeded? > > Andrew > > --- > pw-bot: cr
On Mon, Aug 12, 2024 at 02:35:45PM -0700, Rosen Penev wrote: > On Mon, Aug 12, 2024 at 2:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > > > Allows removing ag71xx_mdio_remove. > > > > > > Removed local mii_bus variable and assign struct members directly. > > > Easier to reason about. > > > > This mixes up two different things, making the patch harder to > > review. Ideally you want lots of little patches, each doing one thing, > > and being obviously correct. > > > > Is ag->mii_bus actually used anywhere, outside of ag71xx_mdio_probe()? > > Often swapping to devm_ means the driver does not need to keep hold of > > the resources. So i actually think you can remove ag->mii_bus. This > > might of been more obvious if you had first swapped to > > devm_of_mdiobus_register() without the other changes mixed in. > not sure I follow. mdiobus_unregister would need to be called in > remove without devm. That would need a private mii_bus of some kind. > So with devm this is unneeded? If you use devm_of_mdiobus_register(), the device core will call devm_mdiobus_unregister() on remove. Your patch removed mdiobus_unregister() in remove.... Is there any user of ag->mii_bus left after converting to devm_of_mdiobus_register()? Andrew
On Mon, Aug 12, 2024 at 2:35 PM Rosen Penev <rosenp@gmail.com> wrote: > > On Mon, Aug 12, 2024 at 2:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > > > Allows removing ag71xx_mdio_remove. > > > > > > Removed local mii_bus variable and assign struct members directly. > > > Easier to reason about. > > > > This mixes up two different things, making the patch harder to > > review. Ideally you want lots of little patches, each doing one thing, > > and being obviously correct. > > > > Is ag->mii_bus actually used anywhere, outside of ag71xx_mdio_probe()? > > Often swapping to devm_ means the driver does not need to keep hold of > > the resources. So i actually think you can remove ag->mii_bus. This > > might of been more obvious if you had first swapped to > > devm_of_mdiobus_register() without the other changes mixed in. > not sure I follow. mdiobus_unregister would need to be called in > remove without devm. That would need a private mii_bus of some kind. > So with devm this is unneeded? Just checked drivers/net/dsa/lantiq_gswip.c This seems correct. Will make the change for v2. > > > > Andrew > > > > --- > > pw-bot: cr
On Mon, Aug 12, 2024 at 7:41 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Aug 12, 2024 at 02:35:45PM -0700, Rosen Penev wrote: > > On Mon, Aug 12, 2024 at 2:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > > > > Allows removing ag71xx_mdio_remove. > > > > > > > > Removed local mii_bus variable and assign struct members directly. > > > > Easier to reason about. > > > > > > This mixes up two different things, making the patch harder to > > > review. Ideally you want lots of little patches, each doing one thing, > > > and being obviously correct. > > > > > > Is ag->mii_bus actually used anywhere, outside of ag71xx_mdio_probe()? > > > Often swapping to devm_ means the driver does not need to keep hold of > > > the resources. So i actually think you can remove ag->mii_bus. This > > > might of been more obvious if you had first swapped to > > > devm_of_mdiobus_register() without the other changes mixed in. > > not sure I follow. mdiobus_unregister would need to be called in > > remove without devm. That would need a private mii_bus of some kind. > > So with devm this is unneeded? > > If you use devm_of_mdiobus_register(), the device core will call > devm_mdiobus_unregister() on remove. Your patch removed > mdiobus_unregister() in remove.... > > Is there any user of ag->mii_bus left after converting to > devm_of_mdiobus_register()? There is not. I've applied the change removing mii_bus from ag locally. From testing, nothing has blown up. Although there's still a problem with switched ports (except for lan1) dying after a while. I think that bug's in qca8k though. calling restart results in no surprises, unlike with some of my other questionable patches. > > Andrew
On Mon, Aug 12, 2024 at 12:06:52PM -0700, Rosen Penev wrote: > Allows removing ag71xx_mdio_remove. > > Removed local mii_bus variable and assign struct members directly. > Easier to reason about. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > drivers/net/ethernet/atheros/ag71xx.c | 39 ++++++++------------------- > 1 file changed, 11 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > index c22ebd3c1f46..1bc882fc1388 100644 > --- a/drivers/net/ethernet/atheros/ag71xx.c > +++ b/drivers/net/ethernet/atheros/ag71xx.c > @@ -684,12 +684,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > { > struct device *dev = &ag->pdev->dev; > struct net_device *ndev = ag->ndev; > - static struct mii_bus *mii_bus; > struct device_node *np, *mnp; > int err; > > np = dev->of_node; > - ag->mii_bus = NULL; > > ag->clk_mdio = devm_clk_get_enabled(dev, "mdio"); > if (IS_ERR(ag->clk_mdio)) { > @@ -703,7 +701,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > return err; > } > > - mii_bus = devm_mdiobus_alloc(dev); > + ag->mii_bus = devm_mdiobus_alloc(dev); > if (!mii_bus) Above mii_bus is removed, but here it is used. > return -ENOMEM; > ...
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c index c22ebd3c1f46..1bc882fc1388 100644 --- a/drivers/net/ethernet/atheros/ag71xx.c +++ b/drivers/net/ethernet/atheros/ag71xx.c @@ -684,12 +684,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) { struct device *dev = &ag->pdev->dev; struct net_device *ndev = ag->ndev; - static struct mii_bus *mii_bus; struct device_node *np, *mnp; int err; np = dev->of_node; - ag->mii_bus = NULL; ag->clk_mdio = devm_clk_get_enabled(dev, "mdio"); if (IS_ERR(ag->clk_mdio)) { @@ -703,7 +701,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) return err; } - mii_bus = devm_mdiobus_alloc(dev); + ag->mii_bus = devm_mdiobus_alloc(dev); if (!mii_bus) return -ENOMEM; @@ -713,13 +711,13 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) return PTR_ERR(ag->mdio_reset); } - mii_bus->name = "ag71xx_mdio"; - mii_bus->read = ag71xx_mdio_mii_read; - mii_bus->write = ag71xx_mdio_mii_write; - mii_bus->reset = ag71xx_mdio_reset; - mii_bus->priv = ag; - mii_bus->parent = dev; - snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); + ag->mii_bus->name = "ag71xx_mdio"; + ag->mii_bus->read = ag71xx_mdio_mii_read; + ag->mii_bus->write = ag71xx_mdio_mii_write; + ag->mii_bus->reset = ag71xx_mdio_reset; + ag->mii_bus->priv = ag; + ag->mii_bus->parent = dev; + snprintf(ag->mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); if (!IS_ERR(ag->mdio_reset)) { reset_control_assert(ag->mdio_reset); @@ -729,22 +727,14 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) } mnp = of_get_child_by_name(np, "mdio"); - err = of_mdiobus_register(mii_bus, mnp); + err = devm_of_mdiobus_register(ag->mii_bus, mnp); of_node_put(mnp); if (err) return err; - ag->mii_bus = mii_bus; - return 0; } -static void ag71xx_mdio_remove(struct ag71xx *ag) -{ - if (ag->mii_bus) - mdiobus_unregister(ag->mii_bus); -} - static void ag71xx_hw_stop(struct ag71xx *ag) { /* disable all interrupts and stop the rx/tx engine */ @@ -1930,14 +1920,14 @@ static int ag71xx_probe(struct platform_device *pdev) err = ag71xx_phylink_setup(ag); if (err) { netif_err(ag, probe, ndev, "failed to setup phylink (%d)\n", err); - goto err_mdio_remove; + return err; } err = register_netdev(ndev); if (err) { netif_err(ag, probe, ndev, "unable to register net device\n"); platform_set_drvdata(pdev, NULL); - goto err_mdio_remove; + return err; } netif_info(ag, probe, ndev, "Atheros AG71xx at 0x%08lx, irq %d, mode:%s\n", @@ -1945,23 +1935,16 @@ static int ag71xx_probe(struct platform_device *pdev) phy_modes(ag->phy_if_mode)); return 0; - -err_mdio_remove: - ag71xx_mdio_remove(ag); - return err; } static void ag71xx_remove(struct platform_device *pdev) { struct net_device *ndev = platform_get_drvdata(pdev); - struct ag71xx *ag; if (!ndev) return; - ag = netdev_priv(ndev); unregister_netdev(ndev); - ag71xx_mdio_remove(ag); platform_set_drvdata(pdev, NULL); }
Allows removing ag71xx_mdio_remove. Removed local mii_bus variable and assign struct members directly. Easier to reason about. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- drivers/net/ethernet/atheros/ag71xx.c | 39 ++++++++------------------- 1 file changed, 11 insertions(+), 28 deletions(-)