Messages in this thread | | | Date | Sun, 18 Dec 2016 17:37:25 +0100 | From | Lukas Wunner <> | Subject | Re: [PATCH v6 4/5] PM / runtime: Use device links |
| |
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. Missed that, sorry for the false alarm and thanks for the clarification.
Lukas
| |