[PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[Chuhong Yuan]

If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
automatically to disable clks.
Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
Add this check to avoid clock count mis-match caused by double-disable.

Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in remove")
Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
Changes in v2:
  - Add fixes tag.

 drivers/net/ethernet/freescale/fec_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index a9c386b63581..696550f4972f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3645,8 +3645,10 @@ fec_drv_remove(struct platform_device *pdev)
 		regulator_disable(fep->reg_phy);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+#ifndef CONFIG_PM
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
+#endif
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 	of_node_put(fep->phy_node);
-- 
2.23.0

Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[David Miller]

From: Chuhong Yuan <hslester96@gmail.com>
Date: Tue, 12 Nov 2019 19:28:30 +0800

> If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
> automatically to disable clks.
> Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
> Add this check to avoid clock count mis-match caused by double-disable.
> 
> Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in remove")
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

I don't understand this at all.

The clk disables here match the unconditional clk enables in the probe
function.

And that is how this is supposed to work, probe enables match remove
disables.  And suspend disables match resume enables.

Why isn't the probe enable taking the correct count, which the remove
function must match with an appropriate disable?  There is no CONFIG_PM
guarding the probe time clk enables.

RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Wednesday, November 13, 2019 3:13 AM
> From: Chuhong Yuan <hslester96@gmail.com>
> Date: Tue, 12 Nov 2019 19:28:30 +0800
> 
> > If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
> > automatically to disable clks.
> > Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
> > Add this check to avoid clock count mis-match caused by double-disable.
> >
> > Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in
> > remove")
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> 
> I don't understand this at all.
> 
> The clk disables here match the unconditional clk enables in the probe
> function.
> 
> And that is how this is supposed to work, probe enables match remove
> disables.  And suspend disables match resume enables.
> 
> Why isn't the probe enable taking the correct count, which the remove
> function must match with an appropriate disable?  There is no CONFIG_PM
> guarding the probe time clk enables.

Current driver runtime pm callback enable/disable clk_ipg/clk_ahb two clks.
CONFIG_PM is a optional config, if CONFIG_PM is disabled, runtime callbacks will
Not be called.
The driver enable clk_ipg/clk_ahb two clks during probe, and depends runtime
suspend to disable the two clks if CONFIG_PM is enabled.

In driver remove() also need to disable the two clks if CONIFG_PM is disabled.
So the patch c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in emove")
target the fixes if CONFIG_PM is not enabled, but the patch ignore to check the 
CONFIG_PM that make clock count mismatch in CONFIG_PM enabled case.

Andy

Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[David Miller]

From: Chuhong Yuan <hslester96@gmail.com>
Date: Tue, 12 Nov 2019 19:28:30 +0800

> If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
> automatically to disable clks.
> Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
> Add this check to avoid clock count mis-match caused by double-disable.
> 
> Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in remove")
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

Your explanation in your reply to my feedback still doesn't explain the
situation to me.

For every clock enable done during probe, there must be a matching clock
disable during remove.

Period.

There is no CONFIG_PM guarding the clock enables during probe in this driver,
therefore there should be no reason to require CONFIG_PM guards to the clock
disables during the remove method,

You have to explain clearly, and in detail, why my logic and analysis of this
situation is not correct.

And when you do so, you will need to add those important details to
the commit message of this change and submit a v3.

Thank you.

RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Saturday, November 16, 2019 4:11 AM
> From: Chuhong Yuan <hslester96@gmail.com>
> Date: Tue, 12 Nov 2019 19:28:30 +0800
> 
> > If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
> > automatically to disable clks.
> > Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
> > Add this check to avoid clock count mis-match caused by double-disable.
> >
> > Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in
> > remove")
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> 
> Your explanation in your reply to my feedback still doesn't explain the
> situation to me.
> 
> For every clock enable done during probe, there must be a matching clock
> disable during remove.
> 
> Period.
> 
> There is no CONFIG_PM guarding the clock enables during probe in this driver,
> therefore there should be no reason to require CONFIG_PM guards to the
> clock disables during the remove method,
> 
> You have to explain clearly, and in detail, why my logic and analysis of this
> situation is not correct.
> 
> And when you do so, you will need to add those important details to the
> commit message of this change and submit a v3.
> 
> Thank you.

I agree with David. Below fixes is more reasonable.
Chuhong, if there has no voice about below fixes, you can submit v3 later.

@@ -3636,6 +3636,11 @@ fec_drv_remove(struct platform_device *pdev)
        struct net_device *ndev = platform_get_drvdata(pdev);
        struct fec_enet_private *fep = netdev_priv(ndev);
        struct device_node *np = pdev->dev.of_node;
+       int ret;
+
+       ret = pm_runtime_get_sync(&pdev->dev);
+       if (ret < 0)
+               return ret;

        cancel_work_sync(&fep->tx_timeout_work);
        fec_ptp_stop(pdev);
@@ -3643,15 +3648,17 @@ fec_drv_remove(struct platform_device *pdev)
        fec_enet_mii_remove(fep);
        if (fep->reg_phy)
                regulator_disable(fep->reg_phy);
-       pm_runtime_put(&pdev->dev);
-       pm_runtime_disable(&pdev->dev);
-       clk_disable_unprepare(fep->clk_ahb);
-       clk_disable_unprepare(fep->clk_ipg);
+
        if (of_phy_is_fixed_link(np))
                of_phy_deregister_fixed_link(np);
        of_node_put(fep->phy_node);
        free_netdev(ndev);

+       clk_disable_unprepare(fep->clk_ahb);
+       clk_disable_unprepare(fep->clk_ipg);
+       pm_runtime_put_noidle(&pdev->dev);
+       pm_runtime_disable(&pdev->dev);
+
        return 0;
 }

Regards,
Fugang Duan

Re: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[Chuhong Yuan]

On Sat, Nov 16, 2019 at 2:57 PM Andy Duan <fugang.duan@nxp.com> wrote:
>
> From: David Miller <davem@davemloft.net> Sent: Saturday, November 16, 2019 4:11 AM
> > From: Chuhong Yuan <hslester96@gmail.com>
> > Date: Tue, 12 Nov 2019 19:28:30 +0800
> >
> > > If CONFIG_PM is enabled, runtime pm will work and call runtime_suspend
> > > automatically to disable clks.
> > > Therefore, remove only needs to disable clks when CONFIG_PM is disabled.
> > > Add this check to avoid clock count mis-match caused by double-disable.
> > >
> > > Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare in
> > > remove")
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> >
> > Your explanation in your reply to my feedback still doesn't explain the
> > situation to me.
> >
> > For every clock enable done during probe, there must be a matching clock
> > disable during remove.
> >
> > Period.
> >
> > There is no CONFIG_PM guarding the clock enables during probe in this driver,
> > therefore there should be no reason to require CONFIG_PM guards to the
> > clock disables during the remove method,
> >
> > You have to explain clearly, and in detail, why my logic and analysis of this
> > situation is not correct.
> >
> > And when you do so, you will need to add those important details to the
> > commit message of this change and submit a v3.
> >
> > Thank you.
>
> I agree with David. Below fixes is more reasonable.
> Chuhong, if there has no voice about below fixes, you can submit v3 later.
>

I get confused that how does this work to solve the CONFIG_PM problem.
And why do we need to adjust the position of the latter four functions?
I need to explain them in the commit message.

> @@ -3636,6 +3636,11 @@ fec_drv_remove(struct platform_device *pdev)
>         struct net_device *ndev = platform_get_drvdata(pdev);
>         struct fec_enet_private *fep = netdev_priv(ndev);
>         struct device_node *np = pdev->dev.of_node;
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(&pdev->dev);
> +       if (ret < 0)
> +               return ret;
>
>         cancel_work_sync(&fep->tx_timeout_work);
>         fec_ptp_stop(pdev);
> @@ -3643,15 +3648,17 @@ fec_drv_remove(struct platform_device *pdev)
>         fec_enet_mii_remove(fep);
>         if (fep->reg_phy)
>                 regulator_disable(fep->reg_phy);
> -       pm_runtime_put(&pdev->dev);
> -       pm_runtime_disable(&pdev->dev);
> -       clk_disable_unprepare(fep->clk_ahb);
> -       clk_disable_unprepare(fep->clk_ipg);
> +
>         if (of_phy_is_fixed_link(np))
>                 of_phy_deregister_fixed_link(np);
>         of_node_put(fep->phy_node);
>         free_netdev(ndev);
>
> +       clk_disable_unprepare(fep->clk_ahb);
> +       clk_disable_unprepare(fep->clk_ipg);
> +       pm_runtime_put_noidle(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +
>         return 0;
>  }
>
> Regards,
> Fugang Duan

RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match

[Andy Duan]

From: Chuhong Yuan <hslester96@gmail.com> Sent: Saturday, November 16, 2019 10:00 PM
> On Sat, Nov 16, 2019 at 2:57 PM Andy Duan <fugang.duan@nxp.com> wrote:
> >
> > From: David Miller <davem@davemloft.net> Sent: Saturday, November 16,
> > 2019 4:11 AM
> > > From: Chuhong Yuan <hslester96@gmail.com>
> > > Date: Tue, 12 Nov 2019 19:28:30 +0800
> > >
> > > > If CONFIG_PM is enabled, runtime pm will work and call
> > > > runtime_suspend automatically to disable clks.
> > > > Therefore, remove only needs to disable clks when CONFIG_PM is
> disabled.
> > > > Add this check to avoid clock count mis-match caused by double-disable.
> > > >
> > > > Fixes: c43eab3eddb4 ("net: fec: add missed clk_disable_unprepare
> > > > in
> > > > remove")
> > > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > >
> > > Your explanation in your reply to my feedback still doesn't explain
> > > the situation to me.
> > >
> > > For every clock enable done during probe, there must be a matching
> > > clock disable during remove.
> > >
> > > Period.
> > >
> > > There is no CONFIG_PM guarding the clock enables during probe in
> > > this driver, therefore there should be no reason to require
> > > CONFIG_PM guards to the clock disables during the remove method,
> > >
> > > You have to explain clearly, and in detail, why my logic and
> > > analysis of this situation is not correct.
> > >
> > > And when you do so, you will need to add those important details to
> > > the commit message of this change and submit a v3.
> > >
> > > Thank you.
> >
> > I agree with David. Below fixes is more reasonable.
> > Chuhong, if there has no voice about below fixes, you can submit v3 later.
> >
> 
> I get confused that how does this work to solve the CONFIG_PM problem.
> And why do we need to adjust the position of the latter four functions?
Just looks better, no function change.
> I need to explain them in the commit message.

Please see below logic in remove():
If CONFIG_PM is enabled:
.probe()
enable clks
pm_runtime_mark_last_busy()
pm_runtime_put_autosuspend()
    disable clks

.remove():
pm_runtime_get_sync()
    runtime resume callback is called, enable clks
disable clks
pm_runtime_put_noidle()
    runtime suspend callback is not called


If CONFIG_PM is disabled, runtime pm is NULL operation:
.probe()
    enable clks
.remove():
    disable clks



> 
> > @@ -3636,6 +3636,11 @@ fec_drv_remove(struct platform_device *pdev)
> >         struct net_device *ndev = platform_get_drvdata(pdev);
> >         struct fec_enet_private *fep = netdev_priv(ndev);
> >         struct device_node *np = pdev->dev.of_node;
> > +       int ret;
> > +
> > +       ret = pm_runtime_get_sync(&pdev->dev);
> > +       if (ret < 0)
> > +               return ret;
> >
> >         cancel_work_sync(&fep->tx_timeout_work);
> >         fec_ptp_stop(pdev);
> > @@ -3643,15 +3648,17 @@ fec_drv_remove(struct platform_device
> *pdev)
> >         fec_enet_mii_remove(fep);
> >         if (fep->reg_phy)
> >                 regulator_disable(fep->reg_phy);
> > -       pm_runtime_put(&pdev->dev);
> > -       pm_runtime_disable(&pdev->dev);
> > -       clk_disable_unprepare(fep->clk_ahb);
> > -       clk_disable_unprepare(fep->clk_ipg);
> > +
> >         if (of_phy_is_fixed_link(np))
> >                 of_phy_deregister_fixed_link(np);
> >         of_node_put(fep->phy_node);
> >         free_netdev(ndev);
> >
> > +       clk_disable_unprepare(fep->clk_ahb);
> > +       clk_disable_unprepare(fep->clk_ipg);
> > +       pm_runtime_put_noidle(&pdev->dev);
> > +       pm_runtime_disable(&pdev->dev);
> > +
> >         return 0;
> >  }
> >
> > Regards,
> > Fugang Duan