On 30.04.2020 13:34, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > 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.
Yep, my mistake. Your code is good. > > 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 > >