Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Mon, 19 Dec 2016 13:38:50 +0100 | Subject | Re: [PATCH v6 4/5] PM / runtime: Use device links |
| |
On Sun, Dec 18, 2016 at 5:37 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Sun, Dec 18, 2016 at 04:53:26PM +0100, Rafael J. Wysocki wrote: >> On Sun, Dec 18, 2016 at 3:01 PM, Lukas Wunner <lukas@wunner.de> wrote: >> > Hi Rafael, >> > >> > spotted what looks like a bug in the device links runtime PM code: >> > >> > When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev): >> > >> >> + retval = rpm_get_suppliers(dev); >> >> + if (retval) >> >> + goto fail; >> > >> > >> > This will walk the list of suppliers and call pm_runtime_get_sync() >> > for each of them: >> > >> >> +static int rpm_get_suppliers(struct device *dev) >> >> +{ >> >> + struct device_link *link; >> >> + >> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) { >> >> + int retval; >> > [...] >> >> + retval = pm_runtime_get_sync(link->supplier); >> >> + if (retval < 0) { >> >> + pm_runtime_put_noidle(link->supplier); >> >> + return retval; >> > >> > >> > If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled >> > on a supplier, the function will put the reference of the failed >> > supplier and return. >> > >> > Back in __rpm_callback() we jump to the fail mark, where we call >> > rpm_put_suppliers(). >> > >> >> + fail: >> >> + rpm_put_suppliers(dev); >> >> + >> >> + device_links_read_unlock(idx); >> > >> > >> > This walks the list of suppliers and releases a ref for each of them: >> > >> >> +static void rpm_put_suppliers(struct device *dev) >> >> +{ >> >> + struct device_link *link; >> >> + >> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) >> >> + if (link->rpm_active && >> >> + READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) { >> >> + pm_runtime_put(link->supplier); >> >> + link->rpm_active = false; >> >> + } >> >> +} >> > >> > >> > This looks wrong: We've already put a ref on the failed supplier, so here >> > we're putting another one. >> >> Are we? I would think link->rpm_active would be false for the failed >> one, wouldn't it? > > Ah, so link->rpm_active means the consumer is holding a ref on the supplier.
Yes, that's the idea. :-)
> Missed that, sorry for the false alarm and thanks for the clarification.
No problem.
Thanks, Rafael
| |