lkml.org 
[lkml]   [2013]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: usb_wwan_write() called while device still being resumed
On 02/15/2013 08:05 PM, Bjørn Mork wrote:
> Alex Courbot <acourbot@nvidia.com> writes:
>
>> Unfortunately it does not, and fails the same way. On the other hand,
>> I do not see the issue when doing the following:
>>
>> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
>> index e4fad5e..1490029 100644
>> --- a/drivers/usb/serial/usb_wwan.c
>> +++ b/drivers/usb/serial/usb_wwan.c
>> @@ -238,8 +238,6 @@ int usb_wwan_write(struct tty_struct *tty, struct
>> usb_serial_port *port,
>> usb_pipeendpoint(this_urb->pipe), i);
>>
>> err =
>> usb_autopm_get_interface_async(port->serial->interface);
>> - if (err < 0)
>> - break;
>>
>> /* send the data */
>> memcpy(this_urb->transfer_buffer, buf, todo);
>>
>> After doing this I don't see this issue anymore. It looks wrong
>> though. But it seems to work despite the obvious unbalance in autopm
>> calls that results.
>>
>> If I understand you correctly, usb_wwan_write() failing here is not a
>> problem in itself, and the ack should just be sent again later?
>
> That was what I thought looking (obviously too) briefly through this.
>
> Most errors from usb_autopm_get_interface_async will be translated to
> EIO before being returned by serial_write. I believe the userspace
> application should deal with that. But maybe it just gives up? Should
> we return EAGAIN or something instead?
>
> I don't know. I am pretty clueless about these things...

Obviously not as much as I am. :) Checking what userspace is doing could
indeed be another trail.

> But looking again, trying to guess why it works fine if you just ignore
> the error. I believe that is because you then end up hitting this until
> the interface is fully resumed:
>
> if (intfdata->suspended) {
> usb_anchor_urb(this_urb, &portdata->delayed);
> spin_unlock_irqrestore(&intfdata->susp_lock, flags);
> }

Yes, this seems to be exactly what is happening.

> I am way out of my league here, but I wonder if pm_runtime_get()
> shouldn't return -EINPROGRESS instead if there is a queued resume
> request or an ongoing resume, regardless of disable_depth?
>
> Maybe something like the completely untested:
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 3148b10..38e19ba 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -512,6 +512,9 @@ static int rpm_resume(struct device *dev, int rpmflags)
> else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> && dev->power.runtime_status == RPM_ACTIVE)
> retval = 1;
> + else if (rpmflags & RPM_ASYNC && dev->power.request_pending &&
> + dev->power.request == RPM_REQ_RESUME)
> + retval = -EINPROGRESS;
> else if (dev->power.disable_depth > 0)
> retval = -EACCES;
> if (retval)
> ---
> usb_autopm_get_interface_async() will interprete EINPROGRESS as success,
> so that would prevent this problem.

That sounds sensefull indeed. If the interface is soon to be resumed,
there should be no reason for usb_autopm_get_interface_async() to fail.
Let's give this a try and bring the idea to the PM people if it works.

In any case thanks a lot for the help, it is extremely useful.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2013-02-17 12:41    [W:0.349 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site