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.
>
> Fixes: e6a41c23df0d ("net: macb: ensure interface is not suspended on
> at91rm9200")
> Cc: Alexandre Belloni <alexandre.bell...@bootlin.com>
> Cc: Claudiu Beznea <claudiu.bez...@microchip.com>
> Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index a0e8c5bbabc01..f739d16d29b1d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -334,8 +334,10 @@ static int macb_mdio_read(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);
pm_runtime_get_sync() calls __pm_runtime_resume(dev, RPM_GET_PUT),
increment refcounter and resume the device calling rpm_resume().
pm_runtime_put_noidle() just decrement the refcounter. 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.
> 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.
> 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.
> return ret;
> + }
>
> /* Clear internal statistics */
> ctl = macb_readl(lp, NCR);
> --
> 2.26.2
>
