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
