Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月19日 23:39, Alan Stern wrote:
> On Wed, 19 Dec 2012, lantianyu wrote:
>
I just find busy_bits is set or clear in the usb_port_resume() and
usb_reset_and_verify_device(). So currently we should keep my changes
mutually exclusive with them, right?
>>>
>>> Don't forget about what happens when a device is removed.
>> I am not very clear about this since busy_bits is not changed during device
>> being
>> removed. Could you elaborate? Thanks.
>
> What happens if the device is unplugged while some thread is using
> busy_bits? Will the hub driver realize that the device is gone?
>
> Alan Stern
>
Hi Alan:
Ok. I get. The port resume/suspend only take place when device is
suspending, suspended and resuming. If the device was unplugged during
busy_bits is set, it would be disconnected when resuming the device
since the resume operation would be failed.
I refresh my patch according to our previous discussion. The
needs_debounce flag is not useful because we move debouce to port's
resume. I also add a check of "port_dev->power_is_on" before set
change_bits in the hub_activate(). When device is power off, the port
status and port change will be 0 and change_bits will be set in the
original hub_activate() code which will cause device to be disconnected.
So add check to keep change_bits unset during device have been power off.
Index: usb/drivers/usb/core/hub.c
===
--- usb.orig/drivers/usb/core/hub.c
+++ usb/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
@@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
-#define HUB_DEBOUNCE_TIMEOUT 1500
+#define HUB_DEBOUNCE_TIMEOUT 2000
#define HUB_DEBOUNCE_STEP25
#define HUB_DEBOUNCE_STABLE 100
@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb
}
/* Note that hdev or one of its children must be locked! */
-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
+struct usb_hub *hdev_to_hub(struct usb_device *hdev)
{
if (!hdev || !hdev->actconfig || !hdev->maxchild)
return NULL;
@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_
/*
* USB 2.0 spec Section 11.24.2.2
*/
-static int clear_port_feature(struct usb_device *hdev, int port1, int
feature)
+int clear_port_feature(struct usb_device *hdev, int port1, int feature)
{
return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
@@ -718,6 +719,9 @@ int usb_hub_set_port_power(struct usb_de
bool set)
{
int ret;
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ struct usb_port *port_dev
+ = hub->ports[port1 - 1];
if (set)
ret = set_port_feature(hdev, port1,
@@ -725,6 +729,9 @@ int usb_hub_set_port_power(struct usb_de
else
ret = clear_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
+
+ if (!ret)
+ port_dev->power_is_on = set;
return ret;
}
@@ -804,7 +811,11 @@ static unsigned hub_power_on(struct usb_
dev_dbg(hub->intfdev, "trying to enable port power on "
"non-switchable hub\n");
for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
- set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+ if (hub->ports[port1 - 1]->power_is_on)
+ set_port_feature(hub->hdev, port1,
USB_PORT_FEAT_POWER);
+ else
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_POWER);
/* Wait at least 100 msec for power to become stable */
delay = max(pgood_delay, (unsigned) 100);
@@ -1136,10 +1147,14 @@ static void hub_activate(struct usb_hub
set_bit(port1, hub->change_bits);
} else if (udev->persist_enabled) {
+ struct usb_port *port_dev = hub->ports[port1 - 1];
+
#ifdef CONFIG_PM
udev->reset_resume = 1;
#endif
- set_bit(port1, hub->change_bits);
+ /* Don't set the change_bits when the device was
power off */
+ if (port_dev->power_is_on)
+ set_bit(port1, hub->change_bits);
} else {
/* The power session is gone; tell khubd */
@@ -2835,6 +2850,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
+ struct usb_port *port_dev = hub->ports[udev->portnum - 1];
int port1 = udev->
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Wed, 19 Dec 2012, lantianyu wrote: > >> I just find busy_bits is set or clear in the usb_port_resume() and > >> usb_reset_and_verify_device(). So currently we should keep my changes > >> mutually exclusive with them, right? > > > > Don't forget about what happens when a device is removed. > I am not very clear about this since busy_bits is not changed during device > being > removed. Could you elaborate? Thanks. What happens if the device is unplugged while some thread is using busy_bits? Will the hub driver realize that the device is gone? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
于 2012/12/18 23:58, Alan Stern 写道:
On Tue, 18 Dec 2012, lantianyu wrote:
What you want here is sort of an alternate debounce routine. The
normal routine waits until the connect status has been stable for 100
ms. But you want to wait until the status has stable in the
"connected" state for 100 ms.
Yesh.
Maybe it would be better to do this by passing an extra argument to the
regular debounce routine.
How about this?
It would be easier to read a patch, particularly if it wasn't
whitespace-damaged.
Oh. Sorry. I didn't notice this. I replied via a newly installed
thunder bird and forgot to unset html format.
static int hub_port_debounce(struct usb_hub *hub, int port1, bool
must_connected)
This should be "must_be_connected".
{
int ret;
int total_time, stable_time = 0;
u16 portchange, portstatus;
unsigned connection = 0x;
for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) {
ret = hub_port_status(hub, port1, &portstatus, &portchange);
if (ret < 0)
return ret;
if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
(portstatus & USB_PORT_STAT_CONNECTION) == connection){
if (!must_connected || (connection == USB_PORT_STAT_CONNECTION))
stable_time += HUB_DEBOUNCE_STEP;
if (stable_time >= HUB_DEBOUNCE_STABLE)
break;
} else {
stable_time = 0;
connection = portstatus & USB_PORT_STAT_CONNECTION;
}
if (portchange & USB_PORT_STAT_C_CONNECTION) {
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
}
if (total_time >= HUB_DEBOUNCE_TIMEOUT)
break;
msleep(HUB_DEBOUNCE_STEP);
}
dev_dbg (hub->intfdev,
"debounce: port %d: total %dms stable %dms status 0x%x\n",
port1, total_time, stable_time, portstatus);
if (stable_time < HUB_DEBOUNCE_STABLE)
return -ETIMEDOUT;
return portstatus;
}
Yes, that seems okay. You'll want to increase HUB_DEBOUNCE_TIMEOUT.
Yeah.
No, no, not at all. The set_bit and clear_bit operations don't need
any protection -- they are atomic. What we need to do is make sure
that two different threads don't try to manage the same port at the
same time. For example, it should not be possible for one thread to
reset the port while another thread is trying to suspend it.
I'm pretty sure that this can't happen with the existing code. The
only places where a port gets used are:
when a device is connected;
when a device is disconnected;
when a device is reset;
when a device is suspended or resumed.
We should check that these things really are mutually exclusive, and
that they remain mutually exclusive when your changes are added.
I just find busy_bits is set or clear in the usb_port_resume() and
usb_reset_and_verify_device(). So currently we should keep my changes
mutually exclusive with them, right?
Don't forget about what happens when a device is removed.
I am not very clear about this since busy_bits is not changed during device
being
removed. Could you elaborate? Thanks.
My changes add two new places: when port is resumed and when is suspended.
Port's resume/suspend may occur in device's resume/suspend and Pm Qos
change by
pm_runtime_get_sync/put(port_dev).
The case in device's resume/suspend will not conflict with
usb_port_resume() and
usb_reset_and_verify_device() since they are all in the same thread or
will not occur at
the same time.
Yes.
For the case of Pm Qos change, actually it only happens when user set
NO_POWER_OFF flag
after the device being power off. The port will be resumed at that time.
One case is
that device resume and Pm Qos change take place at the same time. The
usb_port_resume()
is calling pm_runtime_get_sync(port_dev) and PM core also is doing the
same thing. So two
port resume will be executed in the parallel. Assuming that the one
triggered by usb_port_resume()
is finished firstly and it goes back to usb_port_resume() position where
just before set busy_bits. The
second, port resume operation will be executed. Will this happen? If
yes, there will be a race problem.
Just one case I have found.
The PM core guarantees that runtime PM callbacks are mutually
exclusive. It also guarantees that the runtime_resume routine won't be
called if the device is currently active (and the runtime_suspend
routine won't be called if the device is currently suspended).
Yeah. I misread the code.
Therefore it suffices to make sure that usb_port_resume does
pm_runtime_get_sync(port_dev) before it touches busy_bits.
The other race, against usb_reset_and_verify_device, should also be
okay. Aside from the resume pathway, the only place that routine gets
called is from usb_reset_device, which is careful to prevent the device
from being suspended during the reset.
Is it possble to add a lock to prevent the busy_bits from being cleared
by other thread?
It looks like we don't need it.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Tue, 18 Dec 2012, lantianyu wrote:
> > What you want here is sort of an alternate debounce routine. The
> > normal routine waits until the connect status has been stable for 100
> > ms. But you want to wait until the status has stable in the
> > "connected" state for 100 ms.
> Yesh.
> > Maybe it would be better to do this by passing an extra argument to the
> > regular debounce routine.
> How about this?
It would be easier to read a patch, particularly if it wasn't
whitespace-damaged.
> static int hub_port_debounce(struct usb_hub *hub, int port1, bool
> must_connected)
This should be "must_be_connected".
> {
> int ret;
> int total_time, stable_time = 0;
> u16 portchange, portstatus;
> unsigned connection = 0x;
>
> for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) {
> ret = hub_port_status(hub, port1, &portstatus, &portchange);
> if (ret < 0)
> return ret;
>
> if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
> (portstatus & USB_PORT_STAT_CONNECTION) == connection){
>
> if (!must_connected || (connection == USB_PORT_STAT_CONNECTION))
> stable_time += HUB_DEBOUNCE_STEP;
>
> if (stable_time >= HUB_DEBOUNCE_STABLE)
> break;
> } else {
> stable_time = 0;
> connection = portstatus & USB_PORT_STAT_CONNECTION;
> }
>
> if (portchange & USB_PORT_STAT_C_CONNECTION) {
> clear_port_feature(hub->hdev, port1,
> USB_PORT_FEAT_C_CONNECTION);
> }
>
> if (total_time >= HUB_DEBOUNCE_TIMEOUT)
> break;
> msleep(HUB_DEBOUNCE_STEP);
> }
>
> dev_dbg (hub->intfdev,
> "debounce: port %d: total %dms stable %dms status 0x%x\n",
> port1, total_time, stable_time, portstatus);
>
> if (stable_time < HUB_DEBOUNCE_STABLE)
> return -ETIMEDOUT;
> return portstatus;
> }
Yes, that seems okay. You'll want to increase HUB_DEBOUNCE_TIMEOUT.
> > No, no, not at all. The set_bit and clear_bit operations don't need
> > any protection -- they are atomic. What we need to do is make sure
> > that two different threads don't try to manage the same port at the
> > same time. For example, it should not be possible for one thread to
> > reset the port while another thread is trying to suspend it.
> >
> > I'm pretty sure that this can't happen with the existing code. The
> > only places where a port gets used are:
> >
> > when a device is connected;
> >
> > when a device is disconnected;
> >
> > when a device is reset;
> >
> > when a device is suspended or resumed.
> >
> > We should check that these things really are mutually exclusive, and
> > that they remain mutually exclusive when your changes are added.
> I just find busy_bits is set or clear in the usb_port_resume() and
> usb_reset_and_verify_device(). So currently we should keep my changes
> mutually exclusive with them, right?
Don't forget about what happens when a device is removed.
> My changes add two new places: when port is resumed and when is suspended.
>
> Port's resume/suspend may occur in device's resume/suspend and Pm Qos
> change by
> pm_runtime_get_sync/put(port_dev).
>
> The case in device's resume/suspend will not conflict with
> usb_port_resume() and
> usb_reset_and_verify_device() since they are all in the same thread or
> will not occur at
> the same time.
Yes.
> For the case of Pm Qos change, actually it only happens when user set
> NO_POWER_OFF flag
> after the device being power off. The port will be resumed at that time.
> One case is
> that device resume and Pm Qos change take place at the same time. The
> usb_port_resume()
> is calling pm_runtime_get_sync(port_dev) and PM core also is doing the
> same thing. So two
> port resume will be executed in the parallel. Assuming that the one
> triggered by usb_port_resume()
> is finished firstly and it goes back to usb_port_resume() position where
> just before set busy_bits. The
> second, port resume operation will be executed. Will this happen? If
> yes, there will be a race problem.
> Just one case I have found.
The PM core guarantees that runtime PM callbacks are mutually
exclusive. It also guarantees that the runtime_resume routine won't be
called if the device is currently active (and the runtime_suspend
routine won't be called if the device is currently suspended).
Therefore it suffices to make sure that usb_port_resume does
pm_runtime_get_sync(port_dev) before it touches busy_bits.
The other race, against usb_reset_and_verify_device, should also be
okay. Aside from the resume pathway, the only place that routine gets
called is from usb_reset_device, which is careful to prevent the device
from being suspended during the reset.
> Is it possble to add a lock to prevent the busy_bits from being cleared
> by other thread?
It looks like we don't need it.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
于 2012/12/17 0:25, Alan Stern 写道:
On Sun, 16 Dec 2012, Lan Tianyu wrote:
On 2012/12/14 23:44, Alan Stern wrote:
On Fri, 14 Dec 2012, Lan Tianyu wrote:
Hi Alan:
debounce is still needed. If connect status was not stable, resume
operation will fail. So how about following?
Actually, I'm not sure that a debounce is needed.
Debouncing is used when a plug is physically inserted or removed,
because the electrical terminals make fleeting contact with one another
(they literally bounce on and off each other for a brief time). But in
this case there is no physical insertion. The terminals are in stable
physical contact with each other, so they can't bounce.
I have done a test without debounce but resume failed due to port status
bounce. So I add debounce again.
Okay. That's not a physical bounce but an electrical problem in the
device.
What you want here is sort of an alternate debounce routine. The
normal routine waits until the connect status has been stable for 100
ms. But you want to wait until the status has stable in the
"connected" state for 100 ms.
Yesh.
Maybe it would be better to do this by passing an extra argument to the
regular debounce routine.
How about this?
static int hub_port_debounce(struct usb_hub *hub, int port1, bool
must_connected)
{
int ret;
int total_time, stable_time = 0;
u16 portchange, portstatus;
unsigned connection = 0x;
for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) {
ret = hub_port_status(hub, port1, &portstatus, &portchange);
if (ret < 0)
return ret;
if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
(portstatus & USB_PORT_STAT_CONNECTION) == connection){
if (!must_connected || (connection == USB_PORT_STAT_CONNECTION))
stable_time += HUB_DEBOUNCE_STEP;
if (stable_time >= HUB_DEBOUNCE_STABLE)
break;
} else {
stable_time = 0;
connection = portstatus & USB_PORT_STAT_CONNECTION;
}
if (portchange & USB_PORT_STAT_C_CONNECTION) {
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
}
if (total_time >= HUB_DEBOUNCE_TIMEOUT)
break;
msleep(HUB_DEBOUNCE_STEP);
}
dev_dbg (hub->intfdev,
"debounce: port %d: total %dms stable %dms status 0x%x\n",
port1, total_time, stable_time, portstatus);
if (stable_time < HUB_DEBOUNCE_STABLE)
return -ETIMEDOUT;
return portstatus;
}
Is there a case that the port status bounced and its status maintained
unconnected for a period which caused debounce returned un-connected
status? And wait for more time, the port's status would become connected
and stable.
That can happen. It doesn't cause any problems, because we will detect
the device when it connects the second time. It will then look like a
normal new device connection.
But actually we don't want the device to be disconnected, this will affect
some user space. E.g a usb key, the disc dev node /dev/sd* will be recreated
during this procedure. I think the new debouce routine can resolve the problem.
We're going to have to take a little more care with busy_bits in the
future. It's not protected by any lock, and there's not much to
prevent two different threads from using the same bit at the same time.
Only the fact that this would mean they were trying to control the same
port at the same time.
Ok. So we need to add lock for busy_bits or a routine to change the
busy_bits with a lock protecting. Something likes this.
usb_hub_set_busy_bits(hub, port, set) {
up(lock);
if (set)
set_bit(port1, hub->busy_bits);
else
clear_bit(port1, hub->busy_bits);
down(lock);
}
No, no, not at all. The set_bit and clear_bit operations don't need
any protection -- they are atomic. What we need to do is make sure
that two different threads don't try to manage the same port at the
same time. For example, it should not be possible for one thread to
reset the port while another thread is trying to suspend it.
I'm pretty sure that this can't happen with the existing code. The
only places where a port gets used are:
when a device is connected;
when a device is disconnected;
when a device is reset;
when a device is suspended or resumed.
We should check that these things really are mutually exclusive, and
that they remain mutually exclusive when your changes are added.
I just find busy_bits is set or clear in the usb_port_resume() and
usb_reset_and_verify_device(). So currently we should keep my changes
mutually exclusive with them, right?
My changes add two new places: when port is resumed and when is suspended.
Port's resume/suspend may occur in device's resume/suspend and Pm Qos
change by
pm_runtime_get_sync/put(port_dev).
The case in device's resume/suspend will not conflict with
usb_port_resume() and
usb_reset_and_verify_device() since they are all in the same thread or
will not occur at
the same time.
For the case of Pm Qos change, actually it only happens when user set
NO_POWER_OFF flag
after the device being power off. The port will be resumed
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Sun, 16 Dec 2012, Lan Tianyu wrote:
> On 2012/12/14 23:44, Alan Stern wrote:
> > On Fri, 14 Dec 2012, Lan Tianyu wrote:
> >
> >> Hi Alan:
> >>debounce is still needed. If connect status was not stable, resume
> >> operation will fail. So how about following?
> >
> > Actually, I'm not sure that a debounce is needed.
> >
> > Debouncing is used when a plug is physically inserted or removed,
> > because the electrical terminals make fleeting contact with one another
> > (they literally bounce on and off each other for a brief time). But in
> > this case there is no physical insertion. The terminals are in stable
> > physical contact with each other, so they can't bounce.
>
> I have done a test without debounce but resume failed due to port status
> bounce. So I add debounce again.
Okay. That's not a physical bounce but an electrical problem in the
device.
What you want here is sort of an alternate debounce routine. The
normal routine waits until the connect status has been stable for 100
ms. But you want to wait until the status has stable in the
"connected" state for 100 ms.
Maybe it would be better to do this by passing an extra argument to the
regular debounce routine.
> Is there a case that the port status bounced and its status maintained
> unconnected for a period which caused debounce returned un-connected
> status? And wait for more time, the port's status would become connected
> and stable.
That can happen. It doesn't cause any problems, because we will detect
the device when it connects the second time. It will then look like a
normal new device connection.
> > We're going to have to take a little more care with busy_bits in the
> > future. It's not protected by any lock, and there's not much to
> > prevent two different threads from using the same bit at the same time.
> > Only the fact that this would mean they were trying to control the same
> > port at the same time.
> Ok. So we need to add lock for busy_bits or a routine to change the
> busy_bits with a lock protecting. Something likes this.
>
> usb_hub_set_busy_bits(hub, port, set) {
>
> up(lock);
> if (set)
> set_bit(port1, hub->busy_bits);
> else
> clear_bit(port1, hub->busy_bits);
> down(lock);
> }
No, no, not at all. The set_bit and clear_bit operations don't need
any protection -- they are atomic. What we need to do is make sure
that two different threads don't try to manage the same port at the
same time. For example, it should not be possible for one thread to
reset the port while another thread is trying to suspend it.
I'm pretty sure that this can't happen with the existing code. The
only places where a port gets used are:
when a device is connected;
when a device is disconnected;
when a device is reset;
when a device is suspended or resumed.
We should check that these things really are mutually exclusive, and
that they remain mutually exclusive when your changes are added.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012/12/14 23:44, Alan Stern wrote:
On Fri, 14 Dec 2012, Lan Tianyu wrote:
Hi Alan:
debounce is still needed. If connect status was not stable, resume
operation will fail. So how about following?
Actually, I'm not sure that a debounce is needed.
Debouncing is used when a plug is physically inserted or removed,
because the electrical terminals make fleeting contact with one another
(they literally bounce on and off each other for a brief time). But in
this case there is no physical insertion. The terminals are in stable
physical contact with each other, so they can't bounce.
I have done a test without debounce but resume failed due to port status
bounce. So I add debounce again.
int usb_port_wait_for_connected(struct usb_hub *hub, int port1)
{
u16 portchange, portstatus;
int total_time, ret;
for (total_time = 0; total_time < 2000; total_time += 20) {
ret = hub_port_status(hub, port1, &portstatus, &portchange);
if (ret < 0)
return ret;
if ((portstatus & USB_PORT_STAT_CONNECTION)
&& (hub_port_debounce(hub, port1) &
USB_PORT_STAT_CONNECTION)) {
/*
* just clear enable-change feature since debounce
* has cleared connect-change feature.
*/
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_ENABLE);
return 0;
}
mdelay(20);
}
return -ETIMEDOUT;
}
Why do you want to continue the loop when hub_port_debounce fails?
Is there a case that the port status bounced and its status maintained
unconnected for a period which caused debounce returned un-connected
status? And wait for more time, the port's status would become connected
and stable.
If you get a connect change immediately after turning off the port
power, there's no way to know whether it was because the power is now
off or because the device really disconnected. So the best you can do
is turn on the busy_bits entry, turn off the power, clear the
connect-change and enable-change statuses, and turn off the busy_bits
entry.
You mean this?
static int usb_port_runtime_suspend(struct device *dev)
{
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev =
to_usb_device(dev->parent->parent);
struct usb_hub *hub = hdev_to_hub(hdev);
int port1 = port_dev->portnum;
int retval;
if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
== PM_QOS_FLAGS_ALL)
return -EAGAIN;
set_bit(port1, hub->busy_bits);
retval = usb_hub_set_port_power(hdev, port1, false);
clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_ENABLE);
clear_bit(port1, hub->busy_bits);
return retval;
}
Yes, something like that.
We're going to have to take a little more care with busy_bits in the
future. It's not protected by any lock, and there's not much to
prevent two different threads from using the same bit at the same time.
Only the fact that this would mean they were trying to control the same
port at the same time.
Ok. So we need to add lock for busy_bits or a routine to change the
busy_bits with a lock protecting. Something likes this.
usb_hub_set_busy_bits(hub, port, set) {
up(lock);
if (set)
set_bit(port1, hub->busy_bits);
else
clear_bit(port1, hub->busy_bits);
down(lock);
}
Alan Stern
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Fri, 14 Dec 2012, Lan Tianyu wrote:
> Hi Alan:
> debounce is still needed. If connect status was not stable, resume
> operation will fail. So how about following?
Actually, I'm not sure that a debounce is needed.
Debouncing is used when a plug is physically inserted or removed,
because the electrical terminals make fleeting contact with one another
(they literally bounce on and off each other for a brief time). But in
this case there is no physical insertion. The terminals are in stable
physical contact with each other, so they can't bounce.
> int usb_port_wait_for_connected(struct usb_hub *hub, int port1)
> {
> u16 portchange, portstatus;
> int total_time, ret;
>
> for (total_time = 0; total_time < 2000; total_time += 20) {
> ret = hub_port_status(hub, port1, &portstatus, &portchange);
> if (ret < 0)
> return ret;
>
> if ((portstatus & USB_PORT_STAT_CONNECTION)
> && (hub_port_debounce(hub, port1) &
> USB_PORT_STAT_CONNECTION)) {
> /*
>* just clear enable-change feature since debounce
>* has cleared connect-change feature.
>*/
> clear_port_feature(hub->hdev, port1,
> USB_PORT_FEAT_C_ENABLE);
> return 0;
> }
> mdelay(20);
> }
> return -ETIMEDOUT;
> }
Why do you want to continue the loop when hub_port_debounce fails?
> > If you get a connect change immediately after turning off the port
> > power, there's no way to know whether it was because the power is now
> > off or because the device really disconnected. So the best you can do
> > is turn on the busy_bits entry, turn off the power, clear the
> > connect-change and enable-change statuses, and turn off the busy_bits
> > entry.
> >
> You mean this?
> static int usb_port_runtime_suspend(struct device *dev)
> {
> struct usb_port *port_dev = to_usb_port(dev);
> struct usb_device *hdev =
> to_usb_device(dev->parent->parent);
> struct usb_hub *hub = hdev_to_hub(hdev);
> int port1 = port_dev->portnum;
> int retval;
>
> if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
> == PM_QOS_FLAGS_ALL)
> return -EAGAIN;
>
> set_bit(port1, hub->busy_bits);
> retval = usb_hub_set_port_power(hdev, port1, false);
> clear_port_feature(hdev, port1,
> USB_PORT_FEAT_C_CONNECTION);
> clear_port_feature(hdev, port1,
> USB_PORT_FEAT_C_ENABLE);
> clear_bit(port1, hub->busy_bits);
> return retval;
> }
Yes, something like that.
We're going to have to take a little more care with busy_bits in the
future. It's not protected by any lock, and there's not much to
prevent two different threads from using the same bit at the same time.
Only the fact that this would mean they were trying to control the same
port at the same time.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月12日 23:56, Alan Stern wrote:
> On Wed, 12 Dec 2012, Lan Tianyu wrote:
>
I tested a usb ssd which consumes about 1s to makes usb port status
enter into connect status after powering off and powering on the port.
So I set the tries 20 and the longest latency is larger than 2s.
The debounce routine only guarantees port status stable rather than
enter into connect status.
>>>
>>> Then a better solution would be to first wait (up to 2 seconds) for a
>>> connect status and then call the debounce routine.
>> But some devices don't need to wait 2s such long time, 200ms is enough
>> for them. So I try to check status everytime after debounce. If it's
>> connected, go away.
>
> You should test the connect status in a loop. Each time through the
> loop, if it's not connected wait another 20 or so.
>
Hi Alan:
debounce is still needed. If connect status was not stable, resume
operation will fail. So how about following?
int usb_port_wait_for_connected(struct usb_hub *hub, int port1)
{
u16 portchange, portstatus;
int total_time, ret;
for (total_time = 0; total_time < 2000; total_time += 20) {
ret = hub_port_status(hub, port1, &portstatus, &portchange);
if (ret < 0)
return ret;
if ((portstatus & USB_PORT_STAT_CONNECTION)
&& (hub_port_debounce(hub, port1) &
USB_PORT_STAT_CONNECTION)) {
/*
* just clear enable-change feature since debounce
* has cleared connect-change feature.
*/
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_ENABLE);
return 0;
}
mdelay(20);
}
return -ETIMEDOUT;
}
>
> You need to store somewhere the fact that you made this call, so that
> you will know whether or not to make the corresponding
> pm_runtime_get_sync call in usb_port_resume.
You mean I should add a new flag to keep the
pm_runtime_put_sync/put(port_dev) being called paired, right?
How about "needs_resume"?
>>>
>>> What you need isn't a resume; it's pm_runtime_get_sync.
>> How about "needs_runtime_get"?
>
> Or maybe "did_runtime_put". Something like that.
>
>
Even the power is still on but the PORT_POWER feature has been cleared.
So there should be no event from port, right?
>>>
>>> "should be" -- but buggy hardware might send an event anyway. Also,
>>> many root hubs don't pay attention to the power feature. If this
>>> happens, you probably should handle the connect change properly. I
>>> don't see any point in ignoring it.
>> How to deal with these connect change event or how to identify whether
>> the connect change event is trigger by real power off or not?
>
> If you get a connect change immediately after turning off the port
> power, there's no way to know whether it was because the power is now
> off or because the device really disconnected. So the best you can do
> is turn on the busy_bits entry, turn off the power, clear the
> connect-change and enable-change statuses, and turn off the busy_bits
> entry.
>
You mean this?
static int usb_port_runtime_suspend(struct device *dev)
{
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev =
to_usb_device(dev->parent->parent);
struct usb_hub *hub = hdev_to_hub(hdev);
int port1 = port_dev->portnum;
int retval;
if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
== PM_QOS_FLAGS_ALL)
return -EAGAIN;
set_bit(port1, hub->busy_bits);
retval = usb_hub_set_port_power(hdev, port1, false);
clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_ENABLE);
clear_bit(port1, hub->busy_bits);
return retval;
}
> Alan Stern
>
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Wed, 12 Dec 2012, Alan Stern wrote: > You should test the connect status in a loop. Each time through the > loop, if it's not connected wait another 20 or so. Typo; I meant wait another 20 ms or so. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Wed, 12 Dec 2012, Lan Tianyu wrote: > >> I tested a usb ssd which consumes about 1s to makes usb port status > >> enter into connect status after powering off and powering on the port. > >> So I set the tries 20 and the longest latency is larger than 2s. > >> The debounce routine only guarantees port status stable rather than > >> enter into connect status. > > > > Then a better solution would be to first wait (up to 2 seconds) for a > > connect status and then call the debounce routine. > But some devices don't need to wait 2s such long time, 200ms is enough > for them. So I try to check status everytime after debounce. If it's > connected, go away. You should test the connect status in a loop. Each time through the loop, if it's not connected wait another 20 or so. > >>> You need to store somewhere the fact that you made this call, so that > >>> you will know whether or not to make the corresponding > >>> pm_runtime_get_sync call in usb_port_resume. > >> You mean I should add a new flag to keep the > >> pm_runtime_put_sync/put(port_dev) being called paired, right? > >> How about "needs_resume"? > > > > What you need isn't a resume; it's pm_runtime_get_sync. > How about "needs_runtime_get"? Or maybe "did_runtime_put". Something like that. > >> Even the power is still on but the PORT_POWER feature has been cleared. > >> So there should be no event from port, right? > > > > "should be" -- but buggy hardware might send an event anyway. Also, > > many root hubs don't pay attention to the power feature. If this > > happens, you probably should handle the connect change properly. I > > don't see any point in ignoring it. > How to deal with these connect change event or how to identify whether > the connect change event is trigger by real power off or not? If you get a connect change immediately after turning off the port power, there's no way to know whether it was because the power is now off or because the device really disconnected. So the best you can do is turn on the busy_bits entry, turn off the power, clear the connect-change and enable-change statuses, and turn off the busy_bits entry. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月12日 00:35, Alan Stern wrote:
> On Tue, 11 Dec 2012, Lan Tianyu wrote:
>
@@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
+#define HUB_PORT_RECONNECT_TRIES 20
>>>
>>> 20 is an awful lot. Do you really need any more than one try? The
>>> debounce routine already does its own looping.
>> I tested a usb ssd which consumes about 1s to makes usb port status
>> enter into connect status after powering off and powering on the port.
>> So I set the tries 20 and the longest latency is larger than 2s.
>> The debounce routine only guarantees port status stable rather than
>> enter into connect status.
>
> Then a better solution would be to first wait (up to 2 seconds) for a
> connect status and then call the debounce routine.
But some devices don't need to wait 2s such long time, 200ms is enough
for them. So I try to check status everytime after debounce. If it's
connected, go away.
>
@@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
*hdev, int port1,
bool set)
{
int ret;
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ struct usb_port *port_dev
+ = hub->ports[port1 - 1];
+
+ if (set) {
+ if (port_dev->power_is_on)
+ return 0;
- if (set)
ret = set_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
- else
+ } else {
+ if (!port_dev->power_is_on)
+ return 0;
+
ret = clear_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
+ }
>>>
>>> Do we need these shortcuts? How often will this routine be called when
>>> the port is already in the right state
>> When the PM Qos NO_POWER_OFF is changed,
>> pm_runtime_get_sync/put(port_dev) will be invoked. This routine will be
>> called during port resume and suspend. If one device was suspended and
>> power off, the port's usage_count would be 0. After then, the port will
>> be resumed and suspend every time pm qos NO_POWER_OFF flag being
>> changed. So this depends on the user space.
>
> You did not understand my question. When usb_hub_set_port_power is
> called, won't the Set-Feature request almost always be necessary?
>
Oh. Sorry. Thanks for reminders. :)
> For example, how often will it happen that set and
> port_dev->power_is_on are both true? Or both false? It seems to me
> that this will almost never happen. So why bother testing for it?
Ok. I get. You are right and will remove the check.
>
@@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
pm_message_t msg)
udev->port_is_suspended = 1;
msleep(10);
}
+
+ /*
+ * Check whether current status is meeting requirement of
+ * usb port power off mechanism
+ */
+ if (!udev->do_remote_wakeup
+ && (dev_pm_qos_flags(&port_dev->dev,
+ PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
+ && udev->persist_enabled
+ && !status)
+ pm_runtime_put_sync(&port_dev->dev);
>>>
>>> You need to store somewhere the fact that you made this call, so that
>>> you will know whether or not to make the corresponding
>>> pm_runtime_get_sync call in usb_port_resume.
>> You mean I should add a new flag to keep the
>> pm_runtime_put_sync/put(port_dev) being called paired, right?
>> How about "needs_resume"?
>
> What you need isn't a resume; it's pm_runtime_get_sync.
How about "needs_runtime_get"?
>
@@ -2982,10 +3035,39 @@ static int finish_port_resume(struct usb_device
*udev)
int usb_port_resume(struct usb_device *udev, pm_message_t msg)
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
+ struct usb_port *port_dev = hub->ports[udev->portnum - 1];
int port1 = udev->portnum;
int status;
u16 portchange, portstatus;
+
+ set_bit(port1, hub->busy_bits);
+
+ if (!port_dev->power_is_on) {
>>>
>>> This test is wrong. It's possible that the port power is still on even
>>> though you called pm_runtime_put_sync.
>> Above, we need to check the new flag, right?
>
> Yes.
>
+ status = pm_runtime_get_sync(&port_dev->dev);
+ if (status < 0) {
+ dev_dbg(&udev->dev, "can't resume usb port, status
%d\n",
+ status);
+ clear_bit(port1, hub->busy_bits);
+ return status;
+ }
>>>
>>> Don't you want to call usb_port_wait_for_connected right here?
>>>
+ }
+
+
+ /*
+ * Wait for usb hub port to be reconnected in order to make
+ * the resume pro
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Tue, 11 Dec 2012, Lan Tianyu wrote:
> >> @@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
> >> DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
> >> EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
> >>
> >> +#define HUB_PORT_RECONNECT_TRIES 20
> >
> > 20 is an awful lot. Do you really need any more than one try? The
> > debounce routine already does its own looping.
> I tested a usb ssd which consumes about 1s to makes usb port status
> enter into connect status after powering off and powering on the port.
> So I set the tries 20 and the longest latency is larger than 2s.
> The debounce routine only guarantees port status stable rather than
> enter into connect status.
Then a better solution would be to first wait (up to 2 seconds) for a
connect status and then call the debounce routine.
> >> @@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
> >> *hdev, int port1,
> >>bool set)
> >> {
> >>int ret;
> >> + struct usb_hub *hub = hdev_to_hub(hdev);
> >> + struct usb_port *port_dev
> >> + = hub->ports[port1 - 1];
> >> +
> >> + if (set) {
> >> + if (port_dev->power_is_on)
> >> + return 0;
> >>
> >> - if (set)
> >>ret = set_port_feature(hdev, port1,
> >>USB_PORT_FEAT_POWER);
> >> - else
> >> + } else {
> >> + if (!port_dev->power_is_on)
> >> + return 0;
> >> +
> >>ret = clear_port_feature(hdev, port1,
> >>USB_PORT_FEAT_POWER);
> >> + }
> >
> > Do we need these shortcuts? How often will this routine be called when
> > the port is already in the right state
> When the PM Qos NO_POWER_OFF is changed,
> pm_runtime_get_sync/put(port_dev) will be invoked. This routine will be
> called during port resume and suspend. If one device was suspended and
> power off, the port's usage_count would be 0. After then, the port will
> be resumed and suspend every time pm qos NO_POWER_OFF flag being
> changed. So this depends on the user space.
You did not understand my question. When usb_hub_set_port_power is
called, won't the Set-Feature request almost always be necessary?
For example, how often will it happen that set and
port_dev->power_is_on are both true? Or both false? It seems to me
that this will almost never happen. So why bother testing for it?
> >> @@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
> >> pm_message_t msg)
> >>udev->port_is_suspended = 1;
> >>msleep(10);
> >>}
> >> +
> >> + /*
> >> + * Check whether current status is meeting requirement of
> >> + * usb port power off mechanism
> >> + */
> >> + if (!udev->do_remote_wakeup
> >> + && (dev_pm_qos_flags(&port_dev->dev,
> >> + PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
> >> + && udev->persist_enabled
> >> + && !status)
> >> + pm_runtime_put_sync(&port_dev->dev);
> >
> > You need to store somewhere the fact that you made this call, so that
> > you will know whether or not to make the corresponding
> > pm_runtime_get_sync call in usb_port_resume.
> You mean I should add a new flag to keep the
> pm_runtime_put_sync/put(port_dev) being called paired, right?
> How about "needs_resume"?
What you need isn't a resume; it's pm_runtime_get_sync.
> >> @@ -2982,10 +3035,39 @@ static int finish_port_resume(struct usb_device
> >> *udev)
> >> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> >> {
> >>struct usb_hub *hub = hdev_to_hub(udev->parent);
> >> + struct usb_port *port_dev = hub->ports[udev->portnum - 1];
> >>int port1 = udev->portnum;
> >>int status;
> >>u16 portchange, portstatus;
> >>
> >> +
> >> + set_bit(port1, hub->busy_bits);
> >> +
> >> + if (!port_dev->power_is_on) {
> >
> > This test is wrong. It's possible that the port power is still on even
> > though you called pm_runtime_put_sync.
> Above, we need to check the new flag, right?
Yes.
> >> + status = pm_runtime_get_sync(&port_dev->dev);
> >> + if (status < 0) {
> >> + dev_dbg(&udev->dev, "can't resume usb port, status
> >> %d\n",
> >> + status);
> >> + clear_bit(port1, hub->busy_bits);
> >> + return status;
> >> + }
> >
> > Don't you want to call usb_port_wait_for_connected right here?
> >
> >> + }
> >> +
> >> +
> >> + /*
> >> + * Wait for usb hub port to be reconnected in order to make
> >> + * the resume procedure successful.
> >> + */
> >> + if (port_dev->needs_debounce) {
> >> + status = usb_port_wait_for_connected(hub, port1);
> >
> > If you move this call earlier then you won't have to test
> > needs_debounce.
> The port may have been power on when device is resumed. One case, after
> device being suspended and power off, user may set the NO_POWER_OFF fla
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月11日 00:53, Alan Stern wrote:
> On Mon, 10 Dec 2012, Lan Tianyu wrote:
>
>> Hi Alan:
>> I write a patch based on the needs_debounce flag. The flag will be set
>> when port has child device and power on successfully. Otherwise, I
>> separate resume port and wait for connect in the usb_port_resume().
>> Device will not be disconnected when power_is_on is false or
>> needs_debounce is set. Welcome comments.
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 86e595c..aa41d3a 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
>> DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
>> EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>>
>> +#define HUB_PORT_RECONNECT_TRIES20
>
> 20 is an awful lot. Do you really need any more than one try? The
> debounce routine already does its own looping.
I tested a usb ssd which consumes about 1s to makes usb port status
enter into connect status after powering off and powering on the port.
So I set the tries 20 and the longest latency is larger than 2s.
The debounce routine only guarantees port status stable rather than
enter into connect status.
>
>> @@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
>> *hdev, int port1,
>> bool set)
>> {
>> int ret;
>> +struct usb_hub *hub = hdev_to_hub(hdev);
>> +struct usb_port *port_dev
>> += hub->ports[port1 - 1];
>> +
>> +if (set) {
>> +if (port_dev->power_is_on)
>> +return 0;
>>
>> -if (set)
>> ret = set_port_feature(hdev, port1,
>> USB_PORT_FEAT_POWER);
>> -else
>> +} else {
>> +if (!port_dev->power_is_on)
>> +return 0;
>> +
>> ret = clear_port_feature(hdev, port1,
>> USB_PORT_FEAT_POWER);
>> +}
>
> Do we need these shortcuts? How often will this routine be called when
> the port is already in the right state
When the PM Qos NO_POWER_OFF is changed,
pm_runtime_get_sync/put(port_dev) will be invoked. This routine will be
called during port resume and suspend. If one device was suspended and
power off, the port's usage_count would be 0. After then, the port will
be resumed and suspend every time pm qos NO_POWER_OFF flag being
changed. So this depends on the user space.
>
>> @@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
>> pm_message_t msg)
>> udev->port_is_suspended = 1;
>> msleep(10);
>> }
>> +
>> +/*
>> + * Check whether current status is meeting requirement of
>> + * usb port power off mechanism
>> + */
>> +if (!udev->do_remote_wakeup
>> +&& (dev_pm_qos_flags(&port_dev->dev,
>> +PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
>> +&& udev->persist_enabled
>> +&& !status)
>> +pm_runtime_put_sync(&port_dev->dev);
>
> You need to store somewhere the fact that you made this call, so that
> you will know whether or not to make the corresponding
> pm_runtime_get_sync call in usb_port_resume.
You mean I should add a new flag to keep the
pm_runtime_put_sync/put(port_dev) being called paired, right?
How about "needs_resume"?
>
>> @@ -2982,10 +3035,39 @@ static int finish_port_resume(struct usb_device
>> *udev)
>> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>> {
>> struct usb_hub *hub = hdev_to_hub(udev->parent);
>> +struct usb_port *port_dev = hub->ports[udev->portnum - 1];
>> int port1 = udev->portnum;
>> int status;
>> u16 portchange, portstatus;
>>
>> +
>> +set_bit(port1, hub->busy_bits);
>> +
>> +if (!port_dev->power_is_on) {
>
> This test is wrong. It's possible that the port power is still on even
> though you called pm_runtime_put_sync.
Above, we need to check the new flag, right?
>
>> +status = pm_runtime_get_sync(&port_dev->dev);
>> +if (status < 0) {
>> +dev_dbg(&udev->dev, "can't resume usb port, status
>> %d\n",
>> +status);
>> +clear_bit(port1, hub->busy_bits);
>> +return status;
>> +}
>
> Don't you want to call usb_port_wait_for_connected right here?
>
>> +}
>> +
>> +
>> +/*
>> + * Wait for usb hub port to be reconnected in order to make
>> + * the resume procedure successful.
>> + */
>> +if (port_dev->needs_debounce) {
>> +status = usb_port_wait_for_connected(hub, port1);
>
> If you move this call earlier then you won't have to test
> needs_debounce.
The port may have been power on when device is resumed. One case, after
device being suspended and power off, user may set the NO_POWER_OFF flag
and port will be power on before device being resumed. For this c
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Mon, 10 Dec 2012, Lan Tianyu wrote:
> Hi Alan:
> I write a patch based on the needs_debounce flag. The flag will be set
> when port has child device and power on successfully. Otherwise, I
> separate resume port and wait for connect in the usb_port_resume().
> Device will not be disconnected when power_is_on is false or
> needs_debounce is set. Welcome comments.
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 86e595c..aa41d3a 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
> DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
> EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>
> +#define HUB_PORT_RECONNECT_TRIES 20
20 is an awful lot. Do you really need any more than one try? The
debounce routine already does its own looping.
> @@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
> *hdev, int port1,
> bool set)
> {
> int ret;
> + struct usb_hub *hub = hdev_to_hub(hdev);
> + struct usb_port *port_dev
> + = hub->ports[port1 - 1];
> +
> + if (set) {
> + if (port_dev->power_is_on)
> + return 0;
>
> - if (set)
> ret = set_port_feature(hdev, port1,
> USB_PORT_FEAT_POWER);
> - else
> + } else {
> + if (!port_dev->power_is_on)
> + return 0;
> +
> ret = clear_port_feature(hdev, port1,
> USB_PORT_FEAT_POWER);
> + }
Do we need these shortcuts? How often will this routine be called when
the port is already in the right state?
> @@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
> pm_message_t msg)
> udev->port_is_suspended = 1;
> msleep(10);
> }
> +
> + /*
> + * Check whether current status is meeting requirement of
> + * usb port power off mechanism
> + */
> + if (!udev->do_remote_wakeup
> + && (dev_pm_qos_flags(&port_dev->dev,
> + PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
> + && udev->persist_enabled
> + && !status)
> + pm_runtime_put_sync(&port_dev->dev);
You need to store somewhere the fact that you made this call, so that
you will know whether or not to make the corresponding
pm_runtime_get_sync call in usb_port_resume.
> @@ -2982,10 +3035,39 @@ static int finish_port_resume(struct usb_device
> *udev)
> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> {
> struct usb_hub *hub = hdev_to_hub(udev->parent);
> + struct usb_port *port_dev = hub->ports[udev->portnum - 1];
> int port1 = udev->portnum;
> int status;
> u16 portchange, portstatus;
>
> +
> + set_bit(port1, hub->busy_bits);
> +
> + if (!port_dev->power_is_on) {
This test is wrong. It's possible that the port power is still on even
though you called pm_runtime_put_sync.
> + status = pm_runtime_get_sync(&port_dev->dev);
> + if (status < 0) {
> + dev_dbg(&udev->dev, "can't resume usb port, status
> %d\n",
> + status);
> + clear_bit(port1, hub->busy_bits);
> + return status;
> + }
Don't you want to call usb_port_wait_for_connected right here?
> + }
> +
> +
> + /*
> + * Wait for usb hub port to be reconnected in order to make
> + * the resume procedure successful.
> + */
> + if (port_dev->needs_debounce) {
> + status = usb_port_wait_for_connected(hub, port1);
If you move this call earlier then you won't have to test
needs_debounce.
> @@ -4175,6 +4256,11 @@ static void hub_port_connect_change(struct
> usb_hub *hub, int port1,
> }
> }
>
> + if (!port_dev->power_is_on || port_dev->needs_debounce) {
> + clear_bit(port1, hub->change_bits);
> + return;
> + }
Doesn't the busy_bits flag take care of this for you already?
Also, what if the port is ganged, so even though you think you turned
off the power, it really is still on? When that happens you probably
don't want to ignore port events.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月07日 23:22, Alan Stern wrote:
> On Fri, 7 Dec 2012, Lan Tianyu wrote:
>
>>> Maybe you really need two flags. Do whatever is best; I'm sure you can
>>> figure out a good scheme.
>> Yeah. Two flags maybe good. In this situation, it should be call
>> "power_is_on", right? power_is_on can be used to avoid to sending
>> redundant PORT_POWER request. The other flag is dedicated to prevent
>> device from being disconnected. Something like hub->busy_bits. We can
>> can "child_busy", I am not very good at giving a name. So I'd like to
>> see your opinion. :)
>
> How about "needs_debounce"?
>
> Alan Stern
>
Hi Alan:
I write a patch based on the needs_debounce flag. The flag will be set
when port has child device and power on successfully. Otherwise, I
separate resume port and wait for connect in the usb_port_resume().
Device will not be disconnected when power_is_on is false or
needs_debounce is set. Welcome comments.
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 86e595c..aa41d3a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
@@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
+#define HUB_PORT_RECONNECT_TRIES 20
+
#define HUB_DEBOUNCE_TIMEOUT 1500
#define HUB_DEBOUNCE_STEP25
#define HUB_DEBOUNCE_STABLE 100
static int usb_reset_and_verify_device(struct usb_device *udev);
+static int hub_port_debounce(struct usb_hub *hub, int port1);
static inline char *portspeed(struct usb_hub *hub, int portstatus)
{
@@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
*hdev, int port1,
bool set)
{
int ret;
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ struct usb_port *port_dev
+ = hub->ports[port1 - 1];
+
+ if (set) {
+ if (port_dev->power_is_on)
+ return 0;
- if (set)
ret = set_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
- else
+ } else {
+ if (!port_dev->power_is_on)
+ return 0;
+
ret = clear_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
+ }
+
+ if (!ret)
+ port_dev->power_is_on = set;
return ret;
}
@@ -804,7 +821,11 @@ static unsigned hub_power_on(struct usb_hub *hub,
bool do_delay)
dev_dbg(hub->intfdev, "trying to enable port power on "
"non-switchable hub\n");
for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
- set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+ if (hub->ports[port1 - 1]->power_is_on)
+ set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+ else
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_POWER);
/* Wait at least 100 msec for power to become stable */
delay = max(pgood_delay, (unsigned) 100);
@@ -2768,6 +2789,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
+ struct usb_port *port_dev = hub->ports[udev->portnum - 1];
int port1 = udev->portnum;
int status;
@@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
pm_message_t msg)
udev->port_is_suspended = 1;
msleep(10);
}
+
+ /*
+* Check whether current status is meeting requirement of
+* usb port power off mechanism
+*/
+ if (!udev->do_remote_wakeup
+ && (dev_pm_qos_flags(&port_dev->dev,
+ PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
+ && udev->persist_enabled
+ && !status)
+ pm_runtime_put_sync(&port_dev->dev);
+
usb_mark_last_busy(hub->hdev);
return status;
}
@@ -2945,6 +2979,25 @@ static int finish_port_resume(struct usb_device
*udev)
return status;
}
+static int usb_port_wait_for_connected(struct usb_hub *hub, int port1)
+{
+ int status, i;
+
+ for (i = 0; i < HUB_PORT_RECONNECT_TRIES; i++) {
+ status = hub_port_debounce(hub, port1);
+ if (status & USB_PORT_STAT_CONNECTION) {
+ /*
+* just clear enable-change feature since debounce
+* has cleared connect-change feature.
+*/
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_ENABLE);
+ return 0;
+ }
+
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Fri, 7 Dec 2012, Lan Tianyu wrote: > > Maybe you really need two flags. Do whatever is best; I'm sure you can > > figure out a good scheme. > Yeah. Two flags maybe good. In this situation, it should be call > "power_is_on", right? power_is_on can be used to avoid to sending > redundant PORT_POWER request. The other flag is dedicated to prevent > device from being disconnected. Something like hub->busy_bits. We can > can "child_busy", I am not very good at giving a name. So I'd like to > see your opinion. :) How about "needs_debounce"? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年12月06日 23:43, Alan Stern wrote: > On Thu, 6 Dec 2012, Lan Tianyu wrote: > >> Hi Alan: >> Doing port_dev->power_on = true in usb_hub_set_port_power() just after >> set PORT_POWER feature will cause device to be disconnected. If user set >> PM Qos NO_POWER_OFF flag after the device was power off, the port would >> be power on and do port_dev->power_on = true. But the port connect >> change event couldn't be ignored and the device would be disconnected. > > The problem is that you chose a bad name for this flag. Does > "power_on" mean that the power _is_ on, that the power _was_ on, that > the power _will be_ on, or that the power _should be_ on? > > Maybe you really need two flags. Do whatever is best; I'm sure you can > figure out a good scheme. Yeah. Two flags maybe good. In this situation, it should be call "power_is_on", right? power_is_on can be used to avoid to sending redundant PORT_POWER request. The other flag is dedicated to prevent device from being disconnected. Something like hub->busy_bits. We can can "child_busy", I am not very good at giving a name. So I'd like to see your opinion. :) > > Alan Stern > -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Thu, 6 Dec 2012, Lan Tianyu wrote: > Hi Alan: > Doing port_dev->power_on = true in usb_hub_set_port_power() just after > set PORT_POWER feature will cause device to be disconnected. If user set > PM Qos NO_POWER_OFF flag after the device was power off, the port would > be power on and do port_dev->power_on = true. But the port connect > change event couldn't be ignored and the device would be disconnected. The problem is that you chose a bad name for this flag. Does "power_on" mean that the power _is_ on, that the power _was_ on, that the power _will be_ on, or that the power _should be_ on? Maybe you really need two flags. Do whatever is best; I'm sure you can figure out a good scheme. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On 2012年11月29日 03:37, Alan Stern wrote:
> On Sat, 17 Nov 2012, Lan Tianyu wrote:
>
>> This patch is to add usb port auto power off mechanism.
>> When usb device is suspending, usb core will suspend usb port and
>> usb port runtime pm callback will clear PORT_POWER feature to
>> power off port if all conditions were met. These conditions are
>> remote wakeup disable, pm qos NO_POWER_OFF flag clear and persist
>> enable. When device is suspended and power off, usb core
>> will ignore port's connect and enable change event to keep the device
>> not being disconnected. When it resumes, power on port again.
>
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -2861,6 +2869,19 @@ int usb_port_suspend(struct usb_device *udev,
>> pm_message_t msg)
>> udev->port_is_suspended = 1;
>> msleep(10);
>> }
>> +
>> +/*
>> + * Check whether current status is meeting requirement of
>> + * usb port power off mechanism
>> + */
>> +if (!udev->do_remote_wakeup
>> +&& (dev_pm_qos_flags(&port_dev->dev,
>> +PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
>> +&& udev->persist_enabled
>> +&& !status)
>> +if (!pm_runtime_put_sync(&port_dev->dev))
>> +port_dev->power_on = false;
>
> You shouldn't need to change port_dev->power_on here.
>
>> @@ -2981,10 +3021,36 @@ static int finish_port_resume(struct usb_device
>> *udev)
>> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>> {
>> struct usb_hub *hub = hdev_to_hub(udev->parent);
>> +struct usb_port *port_dev = hub->ports[udev->portnum - 1];
>> int port1 = udev->portnum;
>> int status;
>> u16 portchange, portstatus;
>>
>> +
>> +set_bit(port1, hub->busy_bits);
>> +
>> +if (!port_dev->power_on) {
>> +status = pm_runtime_get_sync(&port_dev->dev);
>> +if (status < 0) {
>> +dev_dbg(&udev->dev, "can't resume usb port, status
>> %d\n",
>> +status);
>> +return status;
>
> You must not return without clearing hub->busy_bits. Same thing
> applies later on.
>
>> +}
>> +
>> +port_dev->power_on = true;
>
> This isn't necessary.
Hi Alan:
Doing port_dev->power_on = true in usb_hub_set_port_power() just after
set PORT_POWER feature will cause device to be disconnected. If user set
PM Qos NO_POWER_OFF flag after the device was power off, the port would
be power on and do port_dev->power_on = true. But the port connect
change event couldn't be ignored and the device would be disconnected.
>
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -72,6 +72,9 @@ static int usb_port_runtime_resume(struct device *dev)
>> struct usb_device *hdev =
>> to_usb_device(dev->parent->parent);
>>
>> +if (port_dev->power_on)
>> +return 0;
>> +
>
> Move these lines into usb_hub_set_port_power, and have that routine set
> port_dev->power_on when the Set-Feature request succeeds.
>
>> return usb_hub_set_port_power(hdev, port_dev->portnum,
>> true);
>> }
>> @@ -81,13 +84,21 @@ static int usb_port_runtime_suspend(struct device *dev)
>> struct usb_port *port_dev = to_usb_port(dev);
>> struct usb_device *hdev =
>> to_usb_device(dev->parent->parent);
>> +int retval;
>>
>> if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>> == PM_QOS_FLAGS_ALL)
>> return -EAGAIN;
>>
>> -return usb_hub_set_port_power(hdev, port_dev->portnum,
>> +if (!port_dev->power_on)
>> +return 0;
>> +
>> +retval = usb_hub_set_port_power(hdev, port_dev->portnum,
>> false);
>> +if (!retval)
>> +port_dev->power_on = false;
>> +
>
> Move all this port_dev->power_on stuff into usb_hub_set_port_power.
> Then no changes will be needed here.
>
> Alan Stern
>
>
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] usb: add usb port auto power off mechanism
On Sat, 17 Nov 2012, Lan Tianyu wrote:
> This patch is to add usb port auto power off mechanism.
> When usb device is suspending, usb core will suspend usb port and
> usb port runtime pm callback will clear PORT_POWER feature to
> power off port if all conditions were met. These conditions are
> remote wakeup disable, pm qos NO_POWER_OFF flag clear and persist
> enable. When device is suspended and power off, usb core
> will ignore port's connect and enable change event to keep the device
> not being disconnected. When it resumes, power on port again.
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2861,6 +2869,19 @@ int usb_port_suspend(struct usb_device *udev,
> pm_message_t msg)
> udev->port_is_suspended = 1;
> msleep(10);
> }
> +
> + /*
> + * Check whether current status is meeting requirement of
> + * usb port power off mechanism
> + */
> + if (!udev->do_remote_wakeup
> + && (dev_pm_qos_flags(&port_dev->dev,
> + PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
> + && udev->persist_enabled
> + && !status)
> + if (!pm_runtime_put_sync(&port_dev->dev))
> + port_dev->power_on = false;
You shouldn't need to change port_dev->power_on here.
> @@ -2981,10 +3021,36 @@ static int finish_port_resume(struct usb_device *udev)
> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> {
> struct usb_hub *hub = hdev_to_hub(udev->parent);
> + struct usb_port *port_dev = hub->ports[udev->portnum - 1];
> int port1 = udev->portnum;
> int status;
> u16 portchange, portstatus;
>
> +
> + set_bit(port1, hub->busy_bits);
> +
> + if (!port_dev->power_on) {
> + status = pm_runtime_get_sync(&port_dev->dev);
> + if (status < 0) {
> + dev_dbg(&udev->dev, "can't resume usb port, status
> %d\n",
> + status);
> + return status;
You must not return without clearing hub->busy_bits. Same thing
applies later on.
> + }
> +
> + port_dev->power_on = true;
This isn't necessary.
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -72,6 +72,9 @@ static int usb_port_runtime_resume(struct device *dev)
> struct usb_device *hdev =
> to_usb_device(dev->parent->parent);
>
> + if (port_dev->power_on)
> + return 0;
> +
Move these lines into usb_hub_set_port_power, and have that routine set
port_dev->power_on when the Set-Feature request succeeds.
> return usb_hub_set_port_power(hdev, port_dev->portnum,
> true);
> }
> @@ -81,13 +84,21 @@ static int usb_port_runtime_suspend(struct device *dev)
> struct usb_port *port_dev = to_usb_port(dev);
> struct usb_device *hdev =
> to_usb_device(dev->parent->parent);
> + int retval;
>
> if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
> == PM_QOS_FLAGS_ALL)
> return -EAGAIN;
>
> - return usb_hub_set_port_power(hdev, port_dev->portnum,
> + if (!port_dev->power_on)
> + return 0;
> +
> + retval = usb_hub_set_port_power(hdev, port_dev->portnum,
> false);
> + if (!retval)
> + port_dev->power_on = false;
> +
Move all this port_dev->power_on stuff into usb_hub_set_port_power.
Then no changes will be needed here.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
