* [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match @ 2019-11-12 11:28 Chuhong Yuan 2019-11-12 19:13 ` David Miller 2019-11-15 20:10 ` David Miller 0 siblings, 2 replies; 7+ messages in thread From: Chuhong Yuan @ 2019-11-12 11:28 UTC (permalink / raw) Cc: Fugang Duan, David S . Miller, netdev, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-12 11:28 [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match Chuhong Yuan @ 2019-11-12 19:13 ` David Miller 2019-11-13 1:50 ` [EXT] " Andy Duan 2019-11-15 20:10 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: David Miller @ 2019-11-12 19:13 UTC (permalink / raw) To: hslester96; +Cc: fugang.duan, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-12 19:13 ` David Miller @ 2019-11-13 1:50 ` Andy Duan 0 siblings, 0 replies; 7+ messages in thread From: Andy Duan @ 2019-11-13 1:50 UTC (permalink / raw) To: David Miller, hslester96; +Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-12 11:28 [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match Chuhong Yuan 2019-11-12 19:13 ` David Miller @ 2019-11-15 20:10 ` David Miller 2019-11-16 6:57 ` [EXT] " Andy Duan 1 sibling, 1 reply; 7+ messages in thread From: David Miller @ 2019-11-15 20:10 UTC (permalink / raw) To: hslester96; +Cc: fugang.duan, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-15 20:10 ` David Miller @ 2019-11-16 6:57 ` Andy Duan 2019-11-16 14:00 ` Chuhong Yuan 0 siblings, 1 reply; 7+ messages in thread From: Andy Duan @ 2019-11-16 6:57 UTC (permalink / raw) To: David Miller, hslester96; +Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-16 6:57 ` [EXT] " Andy Duan @ 2019-11-16 14:00 ` Chuhong Yuan 2019-11-18 6:47 ` Andy Duan 0 siblings, 1 reply; 7+ messages in thread From: Chuhong Yuan @ 2019-11-16 14:00 UTC (permalink / raw) To: Andy Duan; +Cc: David Miller, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match 2019-11-16 14:00 ` Chuhong Yuan @ 2019-11-18 6:47 ` Andy Duan 0 siblings, 0 replies; 7+ messages in thread From: Andy Duan @ 2019-11-18 6:47 UTC (permalink / raw) To: Chuhong Yuan; +Cc: David Miller, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-18 6:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-12 11:28 [PATCH net v2] net: fec: add a check for CONFIG_PM to avoid clock count mis-match Chuhong Yuan 2019-11-12 19:13 ` David Miller 2019-11-13 1:50 ` [EXT] " Andy Duan 2019-11-15 20:10 ` David Miller 2019-11-16 6:57 ` [EXT] " Andy Duan 2019-11-16 14:00 ` Chuhong Yuan 2019-11-18 6:47 ` Andy Duan
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).