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