linux-kernel - Re: [PATCH 2/7] net: stmmac: Balance PTP reference clock enable/disable

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Mar 2017 20:30:12 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Mikko Perttunen <cyndis@...si.fi>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Alexandre Courbot <gnurou@...il.com>,
        Jon Hunter <jonathanh@...dia.com>, netdev@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] net: stmmac: Balance PTP reference clock
 enable/disable

On Mon, Feb 27, 2017 at 11:31:39AM +0200, Mikko Perttunen wrote:
> On 23.02.2017 19:24, Thierry Reding wrote:
> > From: Thierry Reding <treding@...dia.com>
> > 
> > clk_prepare_enable() and clk_disable_unprepare() for this clock aren't
> > properly balanced, which can trigger a WARN_ON() in the common clock
> > framework.
> > 
> > Signed-off-by: Thierry Reding <treding@...dia.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 4 ++++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 -
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3cbe09682afe..6b7a5ce19589 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1711,6 +1711,10 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
> >  	stmmac_mmc_setup(priv);
> > 
> >  	if (init_ptp) {
> > +		ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> > +		if (ret < 0)
> > +			netdev_warn(priv->dev, "failed to enable PTP reference clock: %d\n", ret);
> > +
> 
> Should we return an error code if the clock enable fails?

Yeah, that's probably a good idea.

> >  		ret = stmmac_init_ptp(priv);
> >  		if (ret == -EOPNOTSUPP)
> >  			netdev_warn(priv->dev, "PTP not supported by HW\n");
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 5b18355c0d2b..d285d6cfbd0d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -365,7 +365,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
> >  		plat->clk_ptp_ref = NULL;
> >  		dev_warn(&pdev->dev, "PTP uses main clock\n");
> >  	} else {
> > -		clk_prepare_enable(plat->clk_ptp_ref);
> >  		plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
> >  		dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate);
> >  	}
> > 
> 
> It seems like there will still be a refcount mismatch for the clock if any
> of the request_irqs that are after stmmac_hw_setup in stmmac_open fail.

Looks like there's a few more things that could be cleaned up on failure
to request those interrupts. I've added another patch to the series that
will attempt to do this.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)