On Thu, Dec 13, 2018 at 09:31:05AM +0530, Ramalingam C wrote:
> From: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> Drivers might want to remove some sysfs files, which needs the same
> locks and ends up angering lockdep. Relevant snippet of the stack
> trace:
>
> kernfs_remove_by_name_ns+0x3b/0x80
> bus_remove_driver+0x92/0xa0
> acpi_video_unregister+0x24/0x40
> i915_driver_unload+0x42/0x130 [i915]
> i915_pci_remove+0x19/0x30 [i915]
> pci_device_remove+0x36/0xb0
> device_release_driver_internal+0x185/0x250
> unbind_store+0xaf/0x180
> kernfs_fop_write+0x104/0x190
>
> I've stumbled over this because some new patches by Ram connect the
> snd-hda-intel unload (where we do use sysfs unbind) with the locking
> chains in the i915 unload code (but without creating a new loop),
> which upset our CI. But the bug is already there and can be easily
> reproduced by unbind i915 directly.
>
> No idea whether this is the correct place to fix this, should at least
> get CI happy again.
>
> Note that the bus locking is already done by device_release_driver ->
> device_release_driver_internal, so I dropped that part. Also note that
> we don't recheck that the device is still bound by the same driver,
> but neither does the current code do that without races. And I figured
> that's a obscure enough corner case to not bother.
>
> v2: Use a task work. An entirely async work leads to impressive
> fireworks in our CI, notably in the vtcon bind/unbind code. Task work
> will be as synchronous as the current code, and so keep all these
> preexisting races neatly tugged under the rug.
>
> Cc: Ramalingam C <ramalingam.c@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Revised version of this just landed, so we won't need this anymore for
merging. I'll put my patch into topic/core-for-CI, so you can drop this
for the next version.
-Daniel
> ---
> drivers/base/bus.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..095c4a140d76 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/mutex.h>
> #include <linux/sysfs.h>
> +#include <linux/task_work.h>
> #include "base.h"
> #include "power/power.h"
>
> @@ -174,22 +175,44 @@ static const struct kset_uevent_ops bus_uevent_ops = {
>
> static struct kset *bus_kset;
>
> +struct unbind_work {
> + struct callback_head twork;
> + struct device *dev;
> +};
> +
> +void unbind_work_fn(struct callback_head *work)
> +{
> + struct unbind_work *unbind_work =
> + container_of(work, struct unbind_work, twork);
> +
> + device_release_driver_internal(unbind_work->dev, NULL,
> + unbind_work->dev->parent);
> + put_device(unbind_work->dev);
> + kfree(unbind_work);
> +}
> +
> /* Manually detach a device from its associated driver. */
> static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> size_t count)
> {
> struct bus_type *bus = bus_get(drv->bus);
> + struct unbind_work *unbind_work;
> struct device *dev;
> int err = -ENODEV;
>
> dev = bus_find_device_by_name(bus, NULL, buf);
> if (dev && dev->driver == drv) {
> - if (dev->parent && dev->bus->need_parent_lock)
> - device_lock(dev->parent);
> - device_release_driver(dev);
> - if (dev->parent && dev->bus->need_parent_lock)
> - device_unlock(dev->parent);
> - err = count;
> + unbind_work = kmalloc(sizeof(*unbind_work), GFP_KERNEL);
> + if (unbind_work) {
> + unbind_work->dev = dev;
> + get_device(dev);
> + init_task_work(&unbind_work->twork, unbind_work_fn);
> + task_work_add(current, &unbind_work->twork, true);
> +
> + err = count;
> + } else {
> + err = -ENOMEM;
> + }
> }
> put_device(dev);
> bus_put(bus);
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
