Subject: Re: [PATCH v1 2/2] usb: core: enable remote wakeup function for usb
 controller
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Jiri Kosina <jikos@kernel.org>, benjamin.tissoires@redhat.com,
        gregkh@linuxfoundation.org, Thinh.Nguyen@synopsys.com,
        mathias.nyman@linux.intel.com, rajatja@google.com,
        chris.chiu@canonical.com, linux-usb@vger.kernel.org,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhuyinbo@loongson.cn,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Mathias Nyman <mathias.nyman@linux.intel.com>,
        Rajat Jain <rajatja@google.com>,
        Chris Chiu <chris.chiu@canonical.com>,
        linux-usb@vger.kernel.org, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1638956391-20149-1-git-send-email-zhuyinbo@loongson.cn>
 <1638956391-20149-2-git-send-email-zhuyinbo@loongson.cn>
 <YbEsCSwYLgQefQxU@rowland.harvard.edu>
From:   zhuyinbo <zhuyinbo@loongson.cn>
Message-ID: <fbd46e52-054c-8aea-2f06-3af74c95e5e0@loongson.cn>
Date:   Fri, 10 Dec 2021 17:27:30 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <YbEsCSwYLgQefQxU@rowland.harvard.edu>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


?? 2021/12/9 ????6:04, Alan Stern д??:
> On Wed, Dec 08, 2021 at 05:39:51PM +0800, Yinbo Zhu wrote:
>> The remote wake up function is a regular function on usb device and
>> I think keeping it enabled by default will make the usb application
>> more convenient and usb device remote wake up function keep enabled
>> that ask usb controller remote wake up was enabled at first.
>>
>> This patch only enable wake up on usb root hub device, among which,
> 
> You say the patch only affects root hub devices, but this doesn't appear
> to be true.
> 
>> usb3.0 root hub doesn't be set wakeup node property but use command
>> USB_INTRF_FUNC_SUSPEND to enable remote wake up function.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   drivers/usb/core/hub.c | 20 ++++++++++++++++++--
>>   include/linux/usb.h    |  4 +++-
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 86658a8..cb4b956 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev)
>>    */
>>   int usb_new_device(struct usb_device *udev)
>>   {
>> +	struct usb_host_config *config;
>> +	int ncfg;
>>   	int err;
>>   
>>   	if (udev->parent) {
>> @@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev)
>>   	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
>>   			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
>>   
>> +	for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
>> +		config = &udev->config[ncfg];
>> +		if ((config->desc.bmAttributes & (1 << 5)) == 0)
>> +			break;
>> +		if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
>> +			err = usb_enable_remote_wakeup(udev);
>> +			if (err)
>> +				dev_dbg(&udev->dev,
>> +				      "won't remote wakeup, err %d\n", err);
>> +		}
>> +	}
> 
> I don't see anything in there which treats root hubs differently from
> other devices.
> 
Hi Alan Stern,

You can find following code, non-root-hub had removed Wakeup sysfs 
attributes and disabled wakeup and root-hub had added wakeup sysfs 
attibutes before call usb_new_device, so this patch was only enabled
remote wakeup for root-hub device.
int usb_new_device(struct usb_device *udev)
{
         if (udev->parent) {
                 /* Initialize non-root-hub device wakeup to disabled;
                  * device (un)configuration controls wakeup capable
                  * sysfs power/wakeup controls wakeup enabled/disabled
                  */
                 device_init_wakeup(&udev->dev, 0);
         }

> Besides, enabling wakeup for root hubs is generally a bad idea.  Suppose
> you closed a laptop's lid and then unplugged a USB device -- with wakeup
> enabled, the unplug would cause the laptop to wake up again without your
> knowledge.
> 
> Alan Stern
when closed laptop's lid and then unplugged a non-hid usb device it 
doesn't cause laptop to wakeup. and if that usb device is hid type and 
cause laptop into wakeup state then system will continue into suspend 
state becuase system ask that need accepted a acpi lid open event.
and for laptop usb wakeup that as general ask bios to enable usb wakeup 
then if need do more things to enable usb wakeup I think this usb wakeup 
function isn't friendly and inconveient, so enable it by default.
after add this patch, if want to use usb wakeup function it only need 
enable bios configure it think it is appropriate.

BRs,
Yinbo.
>