[PATCH v1] net: macb: Fix runtime PM refcounting

[PATCH v1] net: macb: Fix runtime PM refcounting

[Andy Shevchenko]

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.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@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);
 		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);
 		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);
 		return ret;
+	}
 
 	/* Clear internal statistics */
 	ctl = macb_readl(lp, NCR);
-- 
2.26.2

Re: [PATCH v1] net: macb: Fix runtime PM refcounting

[Claudiu.Beznea]

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.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@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
> 

Re: [PATCH v1] net: macb: Fix runtime PM refcounting

[Andy Shevchenko]

On Thu, Apr 30, 2020 at 07:59:41AM +0000, Claudiu.Beznea@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

Re: [PATCH v1] net: macb: Fix runtime PM refcounting

[Claudiu.Beznea]

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.Beznea@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
> 
> 

Re: [PATCH v1] net: macb: Fix runtime PM refcounting

[David Miller]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Mon, 27 Apr 2020 13:51:20 +0300

> 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.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thank you.