On Thu, Apr 30, 2020 at 07:59:41AM +0000, claudiu.bez...@microchip.com wrote:
>
>
> On 27.04.2020 13:51, Andy Shevchenko wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > The commit e6a41c23df0d, while trying to fix an issue,
> >
> > ("net: macb: ensure interface is not suspended on at91rm9200")
> >
> > introduced a refcounting regression, because in error case refcounter
> > must be balanced. Fix it by calling pm_runtime_put_noidle() in error case.
> >
> > While here, fix the same mistake in other couple of places.
...
> > status = pm_runtime_get_sync(&bp->pdev->dev);
> > - if (status < 0)
> > + if (status < 0) {
> > + pm_runtime_put_noidle(&bp->pdev->dev);
>
> pm_runtime_get_sync() calls __pm_runtime_resume(dev, RPM_GET_PUT),
> increment refcounter and resume the device calling rpm_resume().
Read the code further than the header file, please.
> pm_runtime_put_noidle() just decrement the refcounter.
which is exactly what has to be done on error path.
> The proper way,
> should be calling suspend again if the operation fails as
> pm_runtime_put_autosuspend() does. So, what the code under mdio_pm_exit
> label does should be enough.
Huh? It returns an error without rebalancing refcounter.
Yeah, one more time an evidence that people do not get runtime PM properly.
> > goto mdio_pm_exit;
> > + }
> >
> > status = macb_mdio_wait_for_idle(bp);
> > if (status < 0)
> > @@ -386,8 +388,10 @@ static int macb_mdio_write(struct mii_bus *bus, int
> > mii_id, int regnum,
> > int status;
> >
> > status = pm_runtime_get_sync(&bp->pdev->dev);
> > - if (status < 0)
> > + if (status < 0) {
> > + pm_runtime_put_noidle(&bp->pdev->dev);
>
> Ditto.
Ditto.
>
> > goto mdio_pm_exit;
> > + }
> >
> > status = macb_mdio_wait_for_idle(bp);
> > if (status < 0)
> > @@ -3816,8 +3820,10 @@ static int at91ether_open(struct net_device *dev)
> > int ret;
> >
> > ret = pm_runtime_get_sync(&lp->pdev->dev);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(&lp->pdev->dev);
>
> The proper way should be calling pm_runtime_put_sync() not only for this
> returning path but for all of them in this function.
Of course not.
> > return ret;
> > + }
> >
> > /* Clear internal statistics */
> > ctl = macb_readl(lp, NCR);
> > --
> > 2.26.2
> >
--
With Best Regards,
Andy Shevchenko
