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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200430103407.GS185537@smile.fi.intel.com>
Date:   Thu, 30 Apr 2020 13:34:07 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Claudiu.Beznea@...rochip.com
Cc:     Nicolas.Ferre@...rochip.com, netdev@...r.kernel.org,
        davem@...emloft.net, alexandre.belloni@...tlin.com
Subject: Re: [PATCH v1] net: macb: Fix runtime PM refcounting

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