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

[Thierry Reding]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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

Attachment: signature.asc
Description: PGP signature