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

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

[adam.lee]

From: Adam Lee <adam.lee@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.lee@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

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

[Laurent Pinchart]

Hi Adam,

Thanks for the patch.

On Wednesday 24 April 2013 15:57:19 adam.lee@canonical.com wrote:
> From: Adam Lee <adam.lee@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.lee@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

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

[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.lee@canonical.com wrote:
> > From: Adam Lee <adam.lee@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

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

[Adam Lee]

On Thu, Apr 25, 2013 at 02:33:06PM +0800, Adam Lee wrote:
> 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.lee@canonical.com wrote:
> > > From: Adam Lee <adam.lee@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.

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.

-- 
Regards,
Adam Lee
Hardware Enablement

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

[Laurent Pinchart]

Hi Adam,

On Thursday 13 June 2013 17:56:47 Adam Lee wrote:
> On Thu, Apr 25, 2013 at 02:33:06PM +0800, Adam Lee wrote:
> > On Wed, Apr 24, 2013 at 11:17:52AM +0200, Laurent Pinchart wrote:
> > > On Wednesday 24 April 2013 15:57:19 adam.lee@canonical.com wrote:
> > > > From: Adam Lee <adam.lee@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.

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

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

[VDR User]

On Tue, Sep 24, 2013 at 4:34 AM, Laurent Pinchart
<laurent.pinchart@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.

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

[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.

-- 
Regards,

Laurent Pinchart