From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D78A7C433ED for ; Tue, 13 Apr 2021 14:44:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A6470613AB for ; Tue, 13 Apr 2021 14:44:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244378AbhDMOom (ORCPT ); Tue, 13 Apr 2021 10:44:42 -0400 Received: from netrider.rowland.org ([192.131.102.5]:47843 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1345247AbhDMOoh (ORCPT ); Tue, 13 Apr 2021 10:44:37 -0400 Received: (qmail 1456110 invoked by uid 1000); 13 Apr 2021 10:44:16 -0400 Date: Tue, 13 Apr 2021 10:44:16 -0400 From: Alan Stern To: Chris Chiu Cc: gregkh@linuxfoundation.org, m.v.b@runbox.com, hadess@hadess.net, linux-usb@vger.kernel.org, Linux Kernel Subject: Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub Message-ID: <20210413144416.GB1454681@rowland.harvard.edu> References: <20210412150006.53909-1-chris.chiu@canonical.com> <20210412151205.GB1420451@rowland.harvard.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 13, 2021 at 03:52:14PM +0800, Chris Chiu wrote: > On Mon, Apr 12, 2021 at 11:12 PM Alan Stern wrote: > > > > On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@canonical.com wrote: > > > The USB_PORT_FEAT_SUSPEND is not really necessary due to the > > > "global suspend" in USB 2.0 spec. It's only for many hub devices > > > which don't relay wakeup requests from the devices connected to > > > downstream ports. For this realtek hub, there's no problem waking > > > up the system from connected keyboard. > > > > What about runtime suspend? That _does_ require USB_PORT_FEAT_SUSPEND. > > It's hard to reproduce the same thing with runtime PM. I also don't > know the aggressive > way to trigger runtime suspend. So I'm assuming the same thing will happen in > runtime PM case because they both go the same usb_port_resume path. Could > you please suggest a better way to verify this for runtime PM? To put a USB device into runtime suspend, do this: echo 0 >/sys/bus/usb/devices/.../bConfigurationValue echo auto >/sys/bus/usb/devices/.../power/control where ... is the pathname for the device you want to suspend. (Note that this will unbind the device from its driver, so make sure there's no possibility of data loss before you do it.) To resume the device, write "on" to the power/control file. You can verify the runtime-PM status by reading the files in the power/ subdirectory. > > > This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub. > > > > > > Signed-off-by: Chris Chiu > > > --- > > > > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > > index 7f71218cc1e5..8478d49bba77 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > > > * descendants is enabled for remote wakeup. > > > */ > > > else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0) > > > - status = set_port_feature(hub->hdev, port1, > > > - USB_PORT_FEAT_SUSPEND); > > > + if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND) > > > > You should test hub->hdev->quirks, here, not udev->quirks. The quirk > > belongs to the Realtek hub, not to the device that's plugged into the > > hub. > > > > Thanks for pointing that out. I'll verify again and propose a V2 after > it's done. Another thing to consider: You shouldn't return 0 from usb_port_suspend if the port wasn't actually suspended. We don't want to kernel to have a false idea of the hardware's current state. Alan Stern