Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 1 Sep 2020 14:17:22 +0200 | Subject | Re: [PATCH v1] driver core: Fix device_pm_lock() locking for device links |
| |
On Tue, Sep 1, 2020 at 12:10 AM Saravana Kannan <saravanak@google.com> wrote: > > This commit fixes two issues: > > 1. The lockdep warning reported by Dong Aisheng <dongas86@gmail.com> [1]. > > It is a warning about a cycle (dpm_list_mtx --> kn->active#3 --> fw_lock) > that was introduced when device-link devices were added to expose device > link information in sysfs. > > The patch that "introduced" this cycle can't be reverted because it's fixes > a real SRCU issue and also ensures that the device-link device is deleted > as soon as the device-link is deleted. This is important to avoid sysfs > name collisions if the device-link is create again immediately (this can > happen a lot with deferred probing). > > 2. device_link_drop_managed() is not grabbing device_pm_lock(). > > When device_link_del() calls __device_link_del() (device_link_del() -> > device_link_put_kref() kref_put() -> __device_link_del()) it grabs the > device_pm_lock(). > > However, when device_link_drop_managed() calls __device_link_del() > (device_link_drop_managed() -> kref_put() -> __device_link_del()) it > doesn't grab device_pm_lock(). There's nothing special about managed > device-links that remove the need for grabbing device_pm_lock(). So, this > patch makes sure device_pm_lock() is always held when deleting managed > links. > > And thanks to Stephen Boyd for helping me understand the lockdep splat. > > Fixes: 843e600b8a2b ("driver core: Fix sleeping in invalid context during device link deletion") > Fixes: 515db266a9da ("driver core: Remove device link creation limitation") > [1] - https://lore.kernel.org/lkml/CAA+hA=S4eAreb7vo69LAXSk2t5=DEKNxHaiY1wSpk4xTp9urLg@mail.gmail.com/ > Reported-by: Dong Aisheng <dongas86@gmail.com> > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > > Rafael, > > A bigger question I had is why we need to grab device_pm_lock() around > device_link_del() in the first place. I understand the need to grab it > during device_link_add() -- it's because we are checking the supplier is > in the dpm_list and because we are reordering devices on the dpm_list. > > But during deletion, we don't need to do either one of those. So, why > do we even need to grab the device_pm_lock() in the first place.
It is not strictly necessary AFAICS.
> The device_links_write_lock() that we already grab before deleting a device > link seems like it'd be sufficient. If you agree we don't need to grab > device_pm_lock() during deletion, then I can change this patch to just > delete that locking.
Yes, please.
Thanks!
| |