On Sun, Dec 18, 2016 at 3:01 PM, Lukas Wunner <[email protected]> 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? > And if there are further suppliers in the list > following the failed one, we'll decrement their refcount even though we've > never incremented it. I'm not following you here, sorry. Thanks, Rafael
