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

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

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