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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
