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