Re: [RFC PATCH] usb/acpi: Add support usb port power off mechanism for device fixed on the motherboard

[Lan Tianyu <tianyu.lan@intel.com>]

On 2012年05月12日 00:12, Lan Tianyu wrote:
> hi all:
> 	Great thanks for review.
> On 2012年05月11日 01:44, Alan Stern wrote:
>> On Thu, 10 May 2012, Sarah Sharp wrote:
>>
>>> On Thu, May 10, 2012 at 11:54:04AM -0400, Alan Stern wrote:
>>>> On Thu, 10 May 2012, Lan Tianyu wrote:
>>>>
>>>>> hi all:
>>>>> 	Currently, we are working on usb port power off mechanism. Our developing
>>>>> machine provides usb port power control (a vbus switch)via ACPI power resource.
>>>>> When the power resource turns off, usb port powers off and usb device loses
>>>>> power. From usb hub side, just like the device being unplugged.
>>>>>
>>>>> 	Since usb port power off will affect hot-plug and devices remote wakeup
>>>>> function, it should be careful to do that.
>>>>> 	We conclude three different situations for power off mechanism.
>>>>> 	(1) hard-wired port with device
>>>>> 	(2) hot-pluggable port without device
>>>>> 	(3) hot-pluggable port with device
>>>>>
>>>>> For hard-wired port, the device will not be removed physically. So we can
>>>>> power off it when device is suspended and remote wakeup is disabled without
>>>>> concerning with hot-plug. This patch is dedicated to this siutation.
>>>>>
>>>>> This patch is to provide usb acpi power control method and call them in the
>>>>> usb_port_suspend() and usb_port_resume() when port can be power off. When the
>>>>> usb port is in the power off state, usb core doesn't remove device which is
>>>>> attached to the port. The device is still on the system and user can access
>>>>> the device.
>>>>
>>>> Can you provide any examples where this would be useful?  It won't end
>>>> up saving very much power (although on a laptop even a little bit might
>>>> help).
>>>
>>> Every little bit of power savings helps for the particular system that
>>> implements the USB port power off.  For this particular platform, Intel
>>> is looking at the system as a whole and trying to eek out power savings
>>> where ever they can.  A little power savings in one particular subsystem
>>> may not seem like a big deal, but when you look at the overall picture,
>>> the long tail adds up.  Just trust me, I'm excited about this system. :)
>>
>> I'll take your word for it.  :-)
>>
>      The power saving depends on devices. I test a usb3.0 ssd. The power saving of
> power off is about 2.2w more than just selective suspend. In theory, power
> off can help to save remaining power after selective suspend.
>
>>> As for examples of where the port power off would be useful, think about
>>> a laptop with several internal ports.  The customer can save some money
>>> by choosing not to purchase an internal USB bluetooth device.  The OEM
>>> may have just one motherboard for those two choices, so the port that
>>> would have held the bluetooth device will be empty.  In that case, we'll
>>> never see a USB device connect on that empty port, so we may as well
>>> power it down.  If we can further power off the internal webcam port
>>> when the device is suspended, we can save more power.
>>
>> The patch did not address the case of powering down ports that have no
>> devices attached.  That might be a better place to start, because it's
>> simpler, even though it might not yield as much power savings.
>
> Do you mean internal ports?
>   From my opinion, ACPI will tell us whether the port is connectable or not.
> When the internal port is not connectable, this means the port is not used
> and this patch will power down the port.
>
> @@ -55,9 +81,14 @@ static int usb_acpi_check_port_connect_type(struct usb_device
> *hdev,
>                   pld.user_visible ?
>                       USB_PORT_CONNECT_TYPE_HOT_PLUG :
>                       USB_PORT_CONNECT_TYPE_HARD_WIRED);
> -   else if (!pld.user_visible)
> +   else if (!pld.user_visible) {
>           usb_set_hub_port_connect_type(hdev, port1, USB_PORT_NOT_USED);
>
> +       /* Power off the usb port which may not be used.*/
> +       if (usb_acpi_power_manageable(hdev, port1))
> +           usb_acpi_set_power_state(hdev, port1, false);
> +   }
> +
>
> For external ports, this should be associated with sys file control. The users
> need to determine when they should be power off.
>
> So I should work on the external ports without devices firstly and
> add the sys file for user to control?
>
>>
>> There's one more thing to consider, which was missing from the patch.
>> When you power the port back up and resume the device, it will
>> necessarily be a reset-resume.  You won't want to do this if any of the
>> drivers attached to the device's interfaces doesn't have reset-resume
>> support.
>>
>>> Another example is when a user walks away from their laptop with some
>>> USB devices plugged in.  If userspace can somehow know when the system
>>> is idle (e.g. the screen blanks, the bluetooth/NFC radio no longer
>>> detects the person's phone, etc), we can power off unconnected and
>>> suspended external ports.  The hypothesis is that some users may not
>>> care about USB device connects and disconnects when their system is
>>> idle.  They really will only care that the changes get noticed when
>>> they start using their system.
>>>
>>> This breaks down for some users, of course.  Arjan has several desktops
>>> under his desk that are always on, and he starts interacting with them
>>> by plugging in a USB mouse and keyboard.  So obviously the "port power
>>> off on screen blank" policy might not work for him.  It also won't work
>>> for servers, where no one connects a real monitor to the server for
>>> months, but server folks probably won't care about this mechanism
>>> because their power budget is so much larger.
>>>
>>> However, someone on an airplane, trying to eek out every mW out of their
>>> battery, would want to power off any external unconnected or suspended
>>> USB ports.
>>>
>>> The point is that whether a user wants to allow the ports to be powered
>>> off should be a userspace policy.  That's why we export the sysfs file,
>>> so that desktop software can implement whatever their customers want.
>>> Personally, I would want a checkbox in the Gnome display settings that
>>> says "Power off USB ports on screen blank".
>>
>> Makes sense.  (But I foresee a lot of confusion among users when this
>> box is checked and the ports don't get powered down -- many of
>> today's laptops are incapable of powering down their USB ports.)
>>
> How about to just provide sys files if the usb port can be power off
> and the checkbox only is selectable when sys files exit.
>>>>> "waiting for connection" state is to avoid device to being removed.
>>>>
>>>> Why would the device be removed?
>>>
>>> When we turn the power back on, we'll get a connect status change bit
>>> set.  We don't want the USB core to misinterpret that bit and try to
>>> logically disconnect the device attached to the port.
>>
>> All you have to do is turn off the connect-change status bit.  In fact,
>> the debounce routine does this already.  Just leave the port state set
>> to "off" until the connection is debounced.
> This is my original version. :)
> But after testing, I found that after the connection is debounced,
> hub_port_connect_change() may not to be invoked or not reach place
> "disconnect devices".
>
> hub_port_connect_change() {
> ...
> /* debounce may be completed before here and port's state has becomed
>    * "on". The device also will be removed.
>    */	
> if (hub->port_data[port1 - 1].power_state == USB_PORT_POWER_STATE_OFF) {
> 	clear_bit(port1, hub->change_bits);
> 	return;
> }
> 	
> /* Disconnect any existing devices under this port */
> if (udev)
> 	usb_disconnect(&hub->port_data[port1-1].child);
> clear_bit(port1, hub->change_bits);
>
> ...
> }
>
> I spent a lot of time resolving the problem. At last, I found to add
> "waiting for connection" state to resolve the problem.
>
> After power on the port, hub_irq() invokes kick_khubd() to deal with
> connect or enable change event. Connect debouncing does in the
> usb_port_resume() and  connect-change event is processed in hub_thread().
> It is hard to synchronize them and the port's state should be set to
> "on" after the connect-change event finishing.
>>
>> You may also have to turn off the enable-change status bit.
>>
> You mean clear_port_feature(connect or enable change)?
>   From my opinion, this will be done in the hub_event() since
> hub is active and hub->quiescing should be 0 when the usb_port_resume()
> is invoking. Every portIs that right?
>
>>> What if the external device was suspended, we power off the port, and
>>> then the user yanks the device while it was suspended?  A device may
>>> never connect in that case.
>>
>> Then the debounce loop in usb_port_wait_for_connected() will time out.
>> At that point you'll know the device has been disconnected.
>>
>>> Let me know if you have any more questions about the mechanisms or
>>> policy we're trying to set up.  Tianyu has been creating these patches,
>>> but I've tried to provide some advice to him along the way.
>>
>> I think I get the picture.  It's a tricky job, because you can easily
>> go too far and turn off something that needs to remain on.
> This is reason why
This is why I want to device driver to set can_power_off, driver may know
clear when the device can be power off.
>>
>> Alan Stern
>>
>

-- 
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html