于 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 = 0xffff;
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 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.
Is it possble to add a lock to prevent the busy_bits from being cleared
by other thread?
Alan Stern
--
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
--
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
