netdev - RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <VI1PR0402MB36002B858030B02BCBCA8500FF4D0@VI1PR0402MB3600.eurprd04.prod.outlook.com>
Date:   Mon, 18 Nov 2019 06:47:26 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Chuhong Yuan <hslester96@...il.com>
CC:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH net v2] net: fec: add a check for CONFIG_PM to
 avoid clock count mis-match

From: Chuhong Yuan <hslester96@...il.com> Sent: Saturday, November 16, 2019 10:00 PM
> On Sat, Nov 16, 2019 at 2:57 PM Andy Duan <fugang.duan@....com> wrote:
> >
> > From: David Miller <davem@...emloft.net> Sent: Saturday, November 16,
> > 2019 4:11 AM
> > > From: Chuhong Yuan <hslester96@...il.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@...il.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