| Message ID | 20201006202029.254212-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | net: fec: Fix phy_device lookup for phy_reset_after_clk_enable() | expand |
On 10/6/2020 1:20 PM, Marek Vasut wrote: > The phy_reset_after_clk_enable() is always called with ndev->phydev, > however that pointer may be NULL even though the PHY device instance > already exists and is sufficient to perform the PHY reset. > > If the PHY still is not bound to the MAC, but there is OF PHY node > and a matching PHY device instance already, use the OF PHY node to > obtain the PHY device instance, and then use that PHY device instance > when triggering the PHY reset. > > Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> > Cc: David S. Miller <davem@davemloft.net> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Richard Leitner <richard.leitner@skidata.com> > Cc: Shawn Guo <shawnguo@kernel.org> > --- > drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 2d5433301843..5a4b20941aeb 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1912,6 +1912,24 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, > return ret; > } > > +static void fec_enet_phy_reset_after_clk_enable(struct net_device *ndev) > +{ > + struct fec_enet_private *fep = netdev_priv(ndev); > + struct phy_device *phy_dev = ndev->phydev; > + > + /* > + * If the PHY still is not bound to the MAC, but there is > + * OF PHY node and a matching PHY device instance already, > + * use the OF PHY node to obtain the PHY device instance, > + * and then use that PHY device instance when triggering > + * the PHY reset. > + */ > + if (!phy_dev && fep->phy_node) > + phy_dev = of_phy_find_device(fep->phy_node); Don't you need to put the phy_dev reference at some point? -- Florian
On 10/6/20 11:09 PM, Florian Fainelli wrote: > > > On 10/6/2020 1:20 PM, Marek Vasut wrote: >> The phy_reset_after_clk_enable() is always called with ndev->phydev, >> however that pointer may be NULL even though the PHY device instance >> already exists and is sufficient to perform the PHY reset. >> >> If the PHY still is not bound to the MAC, but there is OF PHY node >> and a matching PHY device instance already, use the OF PHY node to >> obtain the PHY device instance, and then use that PHY device instance >> when triggering the PHY reset. >> >> Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() >> support") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> >> Cc: David S. Miller <davem@davemloft.net> >> Cc: NXP Linux Team <linux-imx@nxp.com> >> Cc: Richard Leitner <richard.leitner@skidata.com> >> Cc: Shawn Guo <shawnguo@kernel.org> >> --- >> drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c >> b/drivers/net/ethernet/freescale/fec_main.c >> index 2d5433301843..5a4b20941aeb 100644 >> --- a/drivers/net/ethernet/freescale/fec_main.c >> +++ b/drivers/net/ethernet/freescale/fec_main.c >> @@ -1912,6 +1912,24 @@ static int fec_enet_mdio_write(struct mii_bus >> *bus, int mii_id, int regnum, >> return ret; >> } >> +static void fec_enet_phy_reset_after_clk_enable(struct net_device >> *ndev) >> +{ >> + struct fec_enet_private *fep = netdev_priv(ndev); >> + struct phy_device *phy_dev = ndev->phydev; >> + >> + /* >> + * If the PHY still is not bound to the MAC, but there is >> + * OF PHY node and a matching PHY device instance already, >> + * use the OF PHY node to obtain the PHY device instance, >> + * and then use that PHY device instance when triggering >> + * the PHY reset. >> + */ >> + if (!phy_dev && fep->phy_node) >> + phy_dev = of_phy_find_device(fep->phy_node); > > Don't you need to put the phy_dev reference at some point? Probably, yes. But first, does this approach and this patch even make sense ? I mean, it fixes my problem, but is this right ?
On Wed, 7 Oct 2020 00:02:42 +0200 Marek Vasut wrote: > On 10/6/20 11:09 PM, Florian Fainelli wrote: > > On 10/6/2020 1:20 PM, Marek Vasut wrote: > >> The phy_reset_after_clk_enable() is always called with ndev->phydev, > >> however that pointer may be NULL even though the PHY device instance > >> already exists and is sufficient to perform the PHY reset. > >> > >> If the PHY still is not bound to the MAC, but there is OF PHY node > >> and a matching PHY device instance already, use the OF PHY node to > >> obtain the PHY device instance, and then use that PHY device instance > >> when triggering the PHY reset. > >> > >> Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() > >> support") > >> Signed-off-by: Marek Vasut <marex@denx.de> > >> diff --git a/drivers/net/ethernet/freescale/fec_main.c > >> b/drivers/net/ethernet/freescale/fec_main.c > >> index 2d5433301843..5a4b20941aeb 100644 > >> --- a/drivers/net/ethernet/freescale/fec_main.c > >> +++ b/drivers/net/ethernet/freescale/fec_main.c > >> @@ -1912,6 +1912,24 @@ static int fec_enet_mdio_write(struct mii_bus > >> *bus, int mii_id, int regnum, > >> return ret; > >> } > >> +static void fec_enet_phy_reset_after_clk_enable(struct net_device > >> *ndev) > >> +{ > >> + struct fec_enet_private *fep = netdev_priv(ndev); > >> + struct phy_device *phy_dev = ndev->phydev; > >> + > >> + /* > >> + * If the PHY still is not bound to the MAC, but there is > >> + * OF PHY node and a matching PHY device instance already, > >> + * use the OF PHY node to obtain the PHY device instance, > >> + * and then use that PHY device instance when triggering > >> + * the PHY reset. > >> + */ > >> + if (!phy_dev && fep->phy_node) > >> + phy_dev = of_phy_find_device(fep->phy_node); > > > > Don't you need to put the phy_dev reference at some point? > > Probably, yes. > > But first, does this approach and this patch even make sense ? > I mean, it fixes my problem, but is this right ? Can you describe your problem in detail? To an untrained eye this looks pretty weird.
On 10/9/20 2:46 AM, Jakub Kicinski wrote: > On Wed, 7 Oct 2020 00:02:42 +0200 Marek Vasut wrote: >> On 10/6/20 11:09 PM, Florian Fainelli wrote: >>> On 10/6/2020 1:20 PM, Marek Vasut wrote: >>>> The phy_reset_after_clk_enable() is always called with ndev->phydev, >>>> however that pointer may be NULL even though the PHY device instance >>>> already exists and is sufficient to perform the PHY reset. >>>> >>>> If the PHY still is not bound to the MAC, but there is OF PHY node >>>> and a matching PHY device instance already, use the OF PHY node to >>>> obtain the PHY device instance, and then use that PHY device instance >>>> when triggering the PHY reset. >>>> >>>> Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() >>>> support") >>>> Signed-off-by: Marek Vasut <marex@denx.de> > >>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c >>>> b/drivers/net/ethernet/freescale/fec_main.c >>>> index 2d5433301843..5a4b20941aeb 100644 >>>> --- a/drivers/net/ethernet/freescale/fec_main.c >>>> +++ b/drivers/net/ethernet/freescale/fec_main.c >>>> @@ -1912,6 +1912,24 @@ static int fec_enet_mdio_write(struct mii_bus >>>> *bus, int mii_id, int regnum, >>>> return ret; >>>> } >>>> +static void fec_enet_phy_reset_after_clk_enable(struct net_device >>>> *ndev) >>>> +{ >>>> + struct fec_enet_private *fep = netdev_priv(ndev); >>>> + struct phy_device *phy_dev = ndev->phydev; >>>> + >>>> + /* >>>> + * If the PHY still is not bound to the MAC, but there is >>>> + * OF PHY node and a matching PHY device instance already, >>>> + * use the OF PHY node to obtain the PHY device instance, >>>> + * and then use that PHY device instance when triggering >>>> + * the PHY reset. >>>> + */ >>>> + if (!phy_dev && fep->phy_node) >>>> + phy_dev = of_phy_find_device(fep->phy_node); >>> >>> Don't you need to put the phy_dev reference at some point? >> >> Probably, yes. >> >> But first, does this approach and this patch even make sense ? >> I mean, it fixes my problem, but is this right ? > > Can you describe your problem in detail? Yes, I tried to do that in the commit message and the extra detailed comment above the code. What exactly do you not understand from that? > To an untrained eye this looks pretty weird. I see, I'm not quite sure how to address this comment.
On Fri, 9 Oct 2020 09:20:30 +0200 Marek Vasut wrote: > > Can you describe your problem in detail? > > Yes, I tried to do that in the commit message and the extra detailed > comment above the code. What exactly do you not understand from that? Why it's not bound on the first open (I'm guessing it's the first open that has the ndev->phydev == NULL? I shouldn't have to guess). > > To an untrained eye this looks pretty weird. > > I see, I'm not quite sure how to address this comment. If ndev->phydev sometimes is not-NULL on open, then that's a valid state to be in. Why not make sure that we're always in that state and can depend on ndev->phydev rather than rummaging around for the phy_device instance.
On 10/9/20 5:15 PM, Jakub Kicinski wrote: > On Fri, 9 Oct 2020 09:20:30 +0200 Marek Vasut wrote: >>> Can you describe your problem in detail? >> >> Yes, I tried to do that in the commit message and the extra detailed >> comment above the code. What exactly do you not understand from that? > > Why it's not bound on the first open It is getting bound on the first open. The problem is in probe(), where fec_enet_clk_enable(ndev, true) [yes, the name of that function is bad] calls fec_enet_phy_reset_after_clk_enable() and the ndev->phydev is NULL while there is already existing instance of that phydev . So this patch adds this extra look up to get the phydev, which then permits phy_reset_after_clk_enable() to call phy_device_reset() instead of returning -ENODEV. > (I'm guessing it's the first open > that has the ndev->phydev == NULL? I shouldn't have to guess). If I had a crystal ball that'd tell me all the review questions up front, I would write perfect patches with all the feedback sorted out in V1. Sorry, I don't have one ... >>> To an untrained eye this looks pretty weird. >> >> I see, I'm not quite sure how to address this comment. > > If ndev->phydev sometimes is not-NULL on open, then that's a valid > state to be in. Why not make sure that we're always in that state > and can depend on ndev->phydev rather than rummaging around for > the phy_device instance. Nope, the problem is in probe() in this case.
On Fri, 9 Oct 2020 19:34:10 +0200 Marek Vasut wrote: > >>> To an untrained eye this looks pretty weird. > >> > >> I see, I'm not quite sure how to address this comment. > > > > If ndev->phydev sometimes is not-NULL on open, then that's a valid > > state to be in. Why not make sure that we're always in that state > > and can depend on ndev->phydev rather than rummaging around for > > the phy_device instance. > > Nope, the problem is in probe() in this case. In that case it would be cleaner to pass fep and phydev as arguments into fec_enet_clk_enable(), rather than netdev, and have only probe() do the necessary dance.
On 10/9/20 8:02 PM, Jakub Kicinski wrote: > On Fri, 9 Oct 2020 19:34:10 +0200 Marek Vasut wrote: >>>>> To an untrained eye this looks pretty weird. >>>> >>>> I see, I'm not quite sure how to address this comment. >>> >>> If ndev->phydev sometimes is not-NULL on open, then that's a valid >>> state to be in. Why not make sure that we're always in that state >>> and can depend on ndev->phydev rather than rummaging around for >>> the phy_device instance. >> >> Nope, the problem is in probe() in this case. > > In that case it would be cleaner to pass fep and phydev as arguments > into fec_enet_clk_enable(), rather than netdev, and have only probe() > do the necessary dance. So correction, the problem does only happen in open(), in probe() the phydev->drv is still NULL so the PHY reset cannot be triggered. In open(), first the clock have to be enabled, then the reset must toggle, then the PHY IDs can be reliably read. I suspect reset in probe() will need another patch.
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 2d5433301843..5a4b20941aeb 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1912,6 +1912,24 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, return ret; } +static void fec_enet_phy_reset_after_clk_enable(struct net_device *ndev) +{ + struct fec_enet_private *fep = netdev_priv(ndev); + struct phy_device *phy_dev = ndev->phydev; + + /* + * If the PHY still is not bound to the MAC, but there is + * OF PHY node and a matching PHY device instance already, + * use the OF PHY node to obtain the PHY device instance, + * and then use that PHY device instance when triggering + * the PHY reset. + */ + if (!phy_dev && fep->phy_node) + phy_dev = of_phy_find_device(fep->phy_node); + + phy_reset_after_clk_enable(phy_dev); +} + static int fec_enet_clk_enable(struct net_device *ndev, bool enable) { struct fec_enet_private *fep = netdev_priv(ndev); @@ -1938,7 +1956,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable) if (ret) goto failed_clk_ref; - phy_reset_after_clk_enable(ndev->phydev); + fec_enet_phy_reset_after_clk_enable(ndev); } else { clk_disable_unprepare(fep->clk_enet_out); if (fep->clk_ptp) { @@ -2987,7 +3005,7 @@ fec_enet_open(struct net_device *ndev) * phy_reset_after_clk_enable() before because the PHY wasn't probed. */ if (reset_again) - phy_reset_after_clk_enable(ndev->phydev); + fec_enet_phy_reset_after_clk_enable(ndev); /* Probe and connect to PHY when open the interface */ ret = fec_enet_mii_probe(ndev);
The phy_reset_after_clk_enable() is always called with ndev->phydev, however that pointer may be NULL even though the PHY device instance already exists and is sufficient to perform the PHY reset. If the PHY still is not bound to the MAC, but there is OF PHY node and a matching PHY device instance already, use the OF PHY node to obtain the PHY device instance, and then use that PHY device instance when triggering the PHY reset. Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> Cc: David S. Miller <davem@davemloft.net> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: Richard Leitner <richard.leitner@skidata.com> Cc: Shawn Guo <shawnguo@kernel.org> --- drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)