From patchwork Mon Apr 17 08:52:39 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 9683663 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4B3BB600C5 for ; Mon, 17 Apr 2017 08:53:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2F07627CF9 for ; Mon, 17 Apr 2017 08:53:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 203AE27F17; Mon, 17 Apr 2017 08:53:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 60EF527CF9 for ; Mon, 17 Apr 2017 08:53:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753439AbdDQIxS (ORCPT ); Mon, 17 Apr 2017 04:53:18 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:33424 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753368AbdDQIxQ (ORCPT ); Mon, 17 Apr 2017 04:53:16 -0400 Received: by mail-pg0-f65.google.com with SMTP id 63so12584486pgh.0 for ; Mon, 17 Apr 2017 01:53:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id; bh=IvuwCp/ZgrhRnuu+l2Yd1KW3of0VAdfg17ZaT3ojksE=; b=DioklCaWiIY6hRoOv68cThqCtcWKYvHZ1ZNGCssyIGcGnkxKAEsoplReG05Z1qIV++ eq5NTTAFN1UfBBE9GYpneS7bGt+3Nlh5MVrE9K9DKrhJEUjTBI85J/JkxA8eEMg/Rgtk is6nl9Wynj+ml6Azx7K7AEfXi3CX0E43sbuTo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=IvuwCp/ZgrhRnuu+l2Yd1KW3of0VAdfg17ZaT3ojksE=; b=o0tSpE73Qtiu23L8NqGLfZmB9fDreDWo+tFJ0JIVWQ0lE6uBtUy4xvaWvDxdi0RV1y dJsKNMi6Lo0AoUXDOZJjr80parsEjSqasSUxDRd8i3E0WyFiNoK0vD1MGI3nPRf3I9vH wNd6326UXOAZwAQ2686KsqXryOEA4XM5sYDllnD1XnOMRevLbizgrFZDTs9H3nRYMB2v ODky+KU5TcCP961774LcuO4d9dJbD1wbl6Y3LcklVDitUqlXvqqzPU8dMWqFe80DpCZT BufpyiqDUha0f6MnpkXVMTSnvrGQvD1jwY1QInzz42ymmspnGMhUR8jUNL1ihtG6SM6j AhnQ== X-Gm-Message-State: AN3rC/4wNZrQiMDAV5jAkfJHZ1daSuovNWRQ0z5m6R1CbGPb3s4QXRGl YVOOsCxasSSNXQ== X-Received: by 10.98.84.194 with SMTP id i185mr10909764pfb.234.1492419195840; Mon, 17 Apr 2017 01:53:15 -0700 (PDT) Received: from localhost.localdomain (124-171-208-74.dyn.iinet.net.au. [124.171.208.74]) by smtp.gmail.com with ESMTPSA id 128sm16497119pgi.49.2017.04.17.01.53.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Apr 2017 01:53:15 -0700 (PDT) From: Daniel Axtens To: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Daniel Axtens , Laurent Pinchart , Dave Stevenson , Greg KH Subject: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect Date: Mon, 17 Apr 2017 18:52:39 +1000 Message-Id: <20170417085240.12930-1-dja@axtens.net> X-Mailer: git-send-email 2.9.3 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Currently, disconnecting a USB webcam while it is in use prints out a number of warnings, such as: WARNING: CPU: 2 PID: 3118 at /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237 sysfs_remove_group+0x8b/0x90 sysfs group ffffffffa7cd0780 not found for kobject 'event13' This has been noticed before. [0] This is because of the order in which things are torn down. If there are no streams active during a USB disconnect: - uvc_disconnect() is invoked via device_del() through the bus notifier mechanism. - this calls uvc_unregister_video(). - uvc_unregister_video() unregisters the video device for each stream, - because there are no streams open, it calls uvc_delete() - uvc_delete() calls uvc_status_cleanup(), which cleans up the status input device. - uvc_delete() calls media_device_unregister(), which cleans up the media device - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all return, and we end up back in device_del(). - device_del() then cleans up the sysfs folder for the camera with dpm_sysfs_remove(). Because uvc_status_cleanup() and media_device_unregister() have already been called, this all works nicely. If, on the other hand, there *are* streams active during a USB disconnect: - uvc_disconnect() is invoked - this calls uvc_unregister_video() - uvc_unregister_video() unregisters the video device for each stream, - uvc_unregister_video() and uvc_disconnect() return, and we end up back in device_del(). - device_del() then cleans up the sysfs folder for the camera with dpm_sysfs_remove(). Because the status input device and the media device are children of the USB device, this also deletes their sysfs folders. - Sometime later, the final stream is closed, invoking uvc_release(). - uvc_release() calls uvc_delete() - uvc_delete() calls uvc_status_cleanup(), which cleans up the status input device. Because the sysfs directory has already been removed, this causes a WARNing. - uvc_delete() calls media_device_unregister(), which cleans up the media device. Because the sysfs directory has already been removed, this causes another WARNing. To fix this, we need to make sure the devices are always unregistered before the end of uvc_disconnect(). To this, move the unregistration into the disconnect path: - split uvc_status_cleanup() into two parts, one on disconnect that unregisters and one on delete that frees. - move media_device_unregister() into the disconnect path. [0]: https://lkml.org/lkml/2016/12/8/657 Cc: Laurent Pinchart Cc: Dave Stevenson Cc: Greg KH Signed-off-by: Daniel Axtens --- Tested with cheese and yavta. --- drivers/media/usb/uvc/uvc_driver.c | 8 ++++++-- drivers/media/usb/uvc/uvc_status.c | 8 ++++++-- drivers/media/usb/uvc/uvcvideo.h | 1 + 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 46d6be0bb316..2390592f78e0 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev) if (dev->vdev.dev) v4l2_device_unregister(&dev->vdev); #ifdef CONFIG_MEDIA_CONTROLLER - if (media_devnode_is_registered(dev->mdev.devnode)) - media_device_unregister(&dev->mdev); media_device_cleanup(&dev->mdev); #endif @@ -1884,6 +1882,12 @@ static void uvc_unregister_video(struct uvc_device *dev) uvc_debugfs_cleanup_stream(stream); } + uvc_status_unregister(dev); +#ifdef CONFIG_MEDIA_CONTROLLER + if (media_devnode_is_registered(dev->mdev.devnode)) + media_device_unregister(&dev->mdev); +#endif + /* Decrement the stream count and call uvc_delete explicitly if there * are no stream left. */ diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index f552ab997956..95709b23d3b4 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev) return 0; } -void uvc_status_cleanup(struct uvc_device *dev) +void uvc_status_unregister(struct uvc_device *dev) { usb_kill_urb(dev->int_urb); + uvc_input_cleanup(dev); +} + +void uvc_status_cleanup(struct uvc_device *dev) +{ usb_free_urb(dev->int_urb); kfree(dev->status); - uvc_input_cleanup(dev); } int uvc_status_start(struct uvc_device *dev, gfp_t flags) diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 15e415e32c7f..4b4814df35cd 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, /* Status */ extern int uvc_status_init(struct uvc_device *dev); +extern void uvc_status_unregister(struct uvc_device *dev); extern void uvc_status_cleanup(struct uvc_device *dev); extern int uvc_status_start(struct uvc_device *dev, gfp_t flags); extern void uvc_status_stop(struct uvc_device *dev);