[PATCH] Revert "V4L/DVB: uvc: Enable USB autosuspend by default on uvcvideo"

[adam...@canonical.com]

From: Adam Lee <adam...@canonical.com>

This reverts commit 3dae8b41dc5651f8eb22cf310e8b116480ba25b7.

1, I do have a Chicony webcam, implements autosuspend in a broken way,
make `poweroff` performs rebooting when its autosuspend enabled.

2, There are other webcams which don't support autosuspend too, like
https://patchwork.kernel.org/patch/2356141/

3, kernel removed USB_QUIRK_NO_AUTOSUSPEND in
a691efa9888e71232dfb4088fb8a8304ffc7b0f9, because autosuspend is
disabled by default.

So, we need to disable autosuspend in uvcvideo, maintaining a quirk list
only for uvcvideo is not a good idea.

Signed-off-by: Adam Lee <adam...@canonical.com>
---
drivers/media/usb/uvc/uvc_driver.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 5dbefa6..8556f7c 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1914,7 +1914,6 @@ static int uvc_probe(struct usb_interface *intf,
}

uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
- usb_enable_autosuspend(udev);
return 0;

error:
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Laurent Pinchart]

Hi Adam,

Thanks for the patch.

On Wednesday 24 April 2013 15:57:19 adam...@canonical.com wrote:
> From: Adam Lee <adam...@canonical.com>
>
> This reverts commit 3dae8b41dc5651f8eb22cf310e8b116480ba25b7.
>
> 1, I do have a Chicony webcam, implements autosuspend in a broken way,
> make `poweroff` performs rebooting when its autosuspend enabled.
>
> 2, There are other webcams which don't support autosuspend too, like
> https://patchwork.kernel.org/patch/2356141/
>
> 3, kernel removed USB_QUIRK_NO_AUTOSUSPEND in
> a691efa9888e71232dfb4088fb8a8304ffc7b0f9, because autosuspend is
> disabled by default.
>
> So, we need to disable autosuspend in uvcvideo, maintaining a quirk list
> only for uvcvideo is not a good idea.
>
> Signed-off-by: Adam Lee <adam...@canonical.com>

I've received very few bug reports about broken auto-suspend support in UVC
devices. Most of them could be solved by setting the RESET_RESUME quirk in USB
core, only the Creative Live! Cam Optia AF required a quirk in the uvcvideo
driver. I would thus rather use the available quirks (USB_QUIRK_RESET_RESUME
if possible, UVC_QUIRK_DISABLE_AUTOSUSPEND otherwise) than killing power
management for the vast majority of webcams that behave correctly.

> ---
> drivers/media/usb/uvc/uvc_driver.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 5dbefa6..8556f7c 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1914,7 +1914,6 @@ static int uvc_probe(struct usb_interface *intf,
> }
>
> uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
> - usb_enable_autosuspend(udev);
> return 0;
>
> error:

--

Regards,

Laurent Pinchart

[Adam Lee]

On Wed, Apr 24, 2013 at 11:17:52AM +0200, Laurent Pinchart wrote:
> Hi Adam,
>
> Thanks for the patch.
>
> On Wednesday 24 April 2013 15:57:19 adam...@canonical.com wrote:
> > From: Adam Lee <adam...@canonical.com>
> >
> > This reverts commit 3dae8b41dc5651f8eb22cf310e8b116480ba25b7.
> >
> > 1, I do have a Chicony webcam, implements autosuspend in a broken way,
> > make `poweroff` performs rebooting when its autosuspend enabled.
> >
> > 2, There are other webcams which don't support autosuspend too, like
> > https://patchwork.kernel.org/patch/2356141/
> >
> > 3, kernel removed USB_QUIRK_NO_AUTOSUSPEND in
> > a691efa9888e71232dfb4088fb8a8304ffc7b0f9, because autosuspend is
> > disabled by default.
> >
> > So, we need to disable autosuspend in uvcvideo, maintaining a quirk list
> > only for uvcvideo is not a good idea.
>

> I've received very few bug reports about broken auto-suspend support in UVC
> devices. Most of them could be solved by setting the RESET_RESUME quirk in USB
> core, only the Creative Live! Cam Optia AF required a quirk in the uvcvideo
> driver. I would thus rather use the available quirks (USB_QUIRK_RESET_RESUME
> if possible, UVC_QUIRK_DISABLE_AUTOSUSPEND otherwise) than killing power
> management for the vast majority of webcams that behave correctly.

Here comes another one, integrated Chicony webcam 04f2:b39f, its
autosuspend makes `poweroff` performs rebooting at the laptop I'm
working on. I tried USB_QUIRK_RESET_RESUME, not helping.

The quirks list will go longer and longer absolutely, do uvcvideo wanna
maintain that? And why only uvcvideo do this in kernel space which
against general USB module?

I still suggest we disable it by default, people can enable it in udev
just like almost all distroes do for udisks. Please consider about it.

--
Regards,
Adam Lee
Hardware Enablement

[Adam Lee]

Hi, Laurent

Any comment of this? I still think it's a risk to enable autosuspend in
uvcvideo by default, there must be users meeting weird issues but didn't
report to you becaue they didn't figured out the cause.

[Laurent Pinchart]

Hi Adam,

That's a pretty bad one :-/ Do you have any idea why it occurs ?

> > I tried USB_QUIRK_RESET_RESUME, not helping.
> >
> > The quirks list will go longer and longer absolutely, do uvcvideo wanna
> > maintain that? And why only uvcvideo do this in kernel space which
> > against general USB module?
> >
> > I still suggest we disable it by default, people can enable it in udev
> > just like almost all distroes do for udisks. Please consider about it.
>

> Any comment of this? I still think it's a risk to enable autosuspend in
> uvcvideo by default, there must be users meeting weird issues but didn't
> report to you becaue they didn't figured out the cause.

I've discussed this issue during LPC last week, and I still believe we should
enable auto-suspend. The feature really saves power, without it my C910
Logitech webcam gets hot even when unused.

If we disable auto-suspend by default and enable it from userspace only a
handful of devices will get auto-suspended. Unless we can get distros to
automatically test auto-suspend on unknown webcam models and report the
results to update a central data base (which would grow much bigger than a
quirks list in the driver in my opinion), disabling auto-suspend would be a
serious regression.

Any comment from other developers from the mailing lists ?

--
Regards,

Laurent Pinchart

[VDR User]

On Tue, Sep 24, 2013 at 4:34 AM, Laurent Pinchart
<laurent....@ideasonboard.com> wrote:
> I've discussed this issue during LPC last week, and I still believe we should
> enable auto-suspend. The feature really saves power, without it my C910
> Logitech webcam gets hot even when unused.
>
> If we disable auto-suspend by default and enable it from userspace only a
> handful of devices will get auto-suspended. Unless we can get distros to
> automatically test auto-suspend on unknown webcam models and report the
> results to update a central data base (which would grow much bigger than a
> quirks list in the driver in my opinion), disabling auto-suspend would be a
> serious regression.

Setting defaults which knowingly cause problems is a horrible idea.
Just because it works for you and your setup is no justification to
force it upon everyone. This is certainly a feature that, if wanted,
can be enabled by the user. I don't see any reasonable argument
against letting the user enable it if he/she wants it.

Ps. Sorry for the double-post if anyone already received this. The
first posting wasn't using plain text and was therefore rejected so
this corrects that.

[Laurent Pinchart]

Hi,

On Tuesday 24 September 2013 08:55:19 VDR User wrote:

> On Tue, Sep 24, 2013 at 4:34 AM, Laurent Pinchart wrote:
> > I've discussed this issue during LPC last week, and I still believe we
> > should enable auto-suspend. The feature really saves power, without it my
> > C910 Logitech webcam gets hot even when unused.
> >
> > If we disable auto-suspend by default and enable it from userspace only a
> > handful of devices will get auto-suspended. Unless we can get distros to
> > automatically test auto-suspend on unknown webcam models and report the
> > results to update a central data base (which would grow much bigger than a
> > quirks list in the driver in my opinion), disabling auto-suspend would be
> > a serious regression.
>

> Setting defaults which knowingly cause problems is a horrible idea. Just
> because it works for you and your setup is no justification to force it upon
> everyone. This is certainly a feature that, if wanted, can be enabled by the
> user.

It's not just my setup, auto-suspend works for the vast majority of webcams.
It has been enabled three years ago, with a report that Fedora had enabled it
by carrying a kernel patch for a while, without any user complaint.

> I don't see any reasonable argument against letting the user enable it if
> he/she wants it.

USB autosuspend is an important power saving feature. I would be fine with
enabling it in userspace if we could find a reasonable, cross-distro way to
create, maintain and distribute the list of devices that support USB
autosuspend properly.