[PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Jim Wylder]

When calling pm_runtime_get, usb_autopm_get_interface_async
treats a return value of -EINPROGRESS as a success and
increments the usage count.  Since the interface is resuming,
it is safe for usbnet_start_xmit to submit the urb.  If instead,
usbnet_start_xmit treats this as an error the packet will be
dropped.  Additionally, a corresponding tx_complete will not
run to offset the earlier increment of the usage count from the
call to usb_autopm_get_interface_async.

Signed-off-by: James Wylder <james.wylder@motorola.com>
---
 drivers/net/usb/usbnet.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index ce395fe..90849d6 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1105,7 +1105,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,

 	spin_lock_irqsave(&dev->txq.lock, flags);
 	retval = usb_autopm_get_interface_async(dev->intf);
-	if (retval < 0) {
+	if (retval < 0 && retval != -EINPROGRESS) {
 		spin_unlock_irqrestore(&dev->txq.lock, flags);
 		goto drop;
 	}
-- 
1.7.6

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Oliver Neukum]

Am Samstag, 3. September 2011, 16:21:00 schrieb Jim Wylder:
> When calling pm_runtime_get, usb_autopm_get_interface_async
> treats a return value of -EINPROGRESS as a success and
> increments the usage count.  Since the interface is resuming,
> it is safe for usbnet_start_xmit to submit the urb.

usbnet_start_xmit() is exported, so simply stating that it is
called when the interface is resumingisn't enough. It seems to
me that the later check for DEV_ASLEEP will save as, but have
you checked for that?

	Regards
		Oliver

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Jim Wylder]

Oliver,

Thanks for the quick feedback.  True usbnet_start_xmit() could be
running at anytime, but the usb_autopm_get_interface_async() will only
return -EINPROGRESS when rpm_resume detects that
dev->power.runtime_status == RPM_RESUMING.  My understanding is that
for an asynchronous request, the promise that the device is resuming
would be equivalent to cases where usb_autopm_get_interface_async()
returns success.

In all other cases, when we are not attempting to resume an already
resuming interface, this change should have no impact.

Are you recommending that I add an additional check for DEV_ASLEEP,
possibly to decide whether to drop the or continue on?  Or am I
missing your point?  I had not done anything similar because my
understanding was that knowing that the device is in fact resuming
would be sufficient.

Jim

On Sat, Sep 3, 2011 at 11:37 AM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Samstag, 3. September 2011, 16:21:00 schrieb Jim Wylder:
>> When calling pm_runtime_get, usb_autopm_get_interface_async
>> treats a return value of -EINPROGRESS as a success and
>> increments the usage count.  Since the interface is resuming,
>> it is safe for usbnet_start_xmit to submit the urb.
>
> usbnet_start_xmit() is exported, so simply stating that it is
> called when the interface is resumingisn't enough. It seems to
> me that the later check for DEV_ASLEEP will save as, but have
> you checked for that?
>
>        Regards
>                Oliver
>

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Oliver Neukum]

Am Samstag, 3. September 2011, 19:32:44 schrieb Jim Wylder:

Hi,

> Thanks for the quick feedback.  True usbnet_start_xmit() could be
> running at anytime, but the usb_autopm_get_interface_async() will only
> return -EINPROGRESS when rpm_resume detects that
> dev->power.runtime_status == RPM_RESUMING.  My understanding is that
> for an asynchronous request, the promise that the device is resuming
> would be equivalent to cases where usb_autopm_get_interface_async()
> returns success.

If CONFIG_PM is set.

> In all other cases, when we are not attempting to resume an already
> resuming interface, this change should have no impact.
> 
> Are you recommending that I add an additional check for DEV_ASLEEP,

The check is already there but depending on CONFIG_PM.

> possibly to decide whether to drop the or continue on?  Or am I
> missing your point?  I had not done anything similar because my
> understanding was that knowing that the device is in fact resuming
> would be sufficient.

At that point it is sufficient. Upon further thought it looks like your check
is correct.
We just need to make very sure that for the decision to queue or
to submit EVENT_DEV_ASLEEP is relevant.
Would you consider defining a macro with a nice name for the
==0 || == -EINPROGRESS check, so that any reader knows what
this is about? This is because I doubt only usbnet is affected.

	Regards
		Oliver

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Jim Wylder]

When calling pm_runtime_get, usb_autopm_get_interface_async
treats a return value of -EINPROGRESS as a success and
increments the usage count.  Since the interface is resuming,
it is safe for usbnet_start_xmit to submit the urb.  If instead,
usbnet_start_xmit treats this as an error the packet will be
dropped.  Additionally, a corresponding tx_complete will not
run to offset the earlier increment of the usage count from the
call to usb_autopm_get_interface_async.

Signed-off-by: James Wylder <james.wylder@motorola.com>
---
 drivers/net/usb/usbnet.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index ce395fe..7dcef05 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -81,6 +81,19 @@

 /*-------------------------------------------------------------------------*/

+/* When power management is configured, usb_autopm_get_interface_async()
+ * will return -EINPROGRESS to signify that the interface was already
+ * resuming at the time that the function was called.  In our case
+ * (all cases?) this not an error condition.
+ * */
+#ifdef CONFIG_PM
+#define USBNET_PM_INTF_ALREADY_RESUMING(x) (x == -EINPROGRESS)
+#else
+#define USBNET_PM_INTF_ALREADY_RESUMING(x) 0
+#endif
+
+/*-------------------------------------------------------------------------*/
+
 // randomly generated ethernet address
 static u8	node_id [ETH_ALEN];

@@ -1042,6 +1055,7 @@ EXPORT_SYMBOL_GPL(usbnet_tx_timeout);

 /*-------------------------------------------------------------------------*/

+
 netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 				     struct net_device *net)
 {
@@ -1105,7 +1119,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,

 	spin_lock_irqsave(&dev->txq.lock, flags);
 	retval = usb_autopm_get_interface_async(dev->intf);
-	if (retval < 0) {
+	if (retval < 0 && !USBNET_PM_INTF_ALREADY_RESUMING(retval)) {
 		spin_unlock_irqrestore(&dev->txq.lock, flags);
 		goto drop;
 	}
-- 
1.7.6

On Sun, Sep 4, 2011 at 5:16 AM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Samstag, 3. September 2011, 19:32:44 schrieb Jim Wylder:
>
> Hi,
>
>> Thanks for the quick feedback.  True usbnet_start_xmit() could be
>> running at anytime, but the usb_autopm_get_interface_async() will only
>> return -EINPROGRESS when rpm_resume detects that
>> dev->power.runtime_status == RPM_RESUMING.  My understanding is that
>> for an asynchronous request, the promise that the device is resuming
>> would be equivalent to cases where usb_autopm_get_interface_async()
>> returns success.
>
> If CONFIG_PM is set.
>
>> In all other cases, when we are not attempting to resume an already
>> resuming interface, this change should have no impact.
>>
>> Are you recommending that I add an additional check for DEV_ASLEEP,
>
> The check is already there but depending on CONFIG_PM.
>
>> possibly to decide whether to drop the or continue on?  Or am I
>> missing your point?  I had not done anything similar because my
>> understanding was that knowing that the device is in fact resuming
>> would be sufficient.
>
> At that point it is sufficient. Upon further thought it looks like your check
> is correct.
> We just need to make very sure that for the decision to queue or
> to submit EVENT_DEV_ASLEEP is relevant.
> Would you consider defining a macro with a nice name for the
> ==0 || == -EINPROGRESS check, so that any reader knows what
> this is about? This is because I doubt only usbnet is affected.
>
>        Regards
>                Oliver
>

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Jim Wylder]

Oliver,

I have a couple of questions about this:

- Does the macro as implemented properly communicate the intent as you
requested?

- You made a comment about this being useful outside of usbnet.  The
only appropriate place that I could identify for this would be all the
way up in usb.h.  Do you think something at that level is more
appropriate?

Jim

Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

[Oliver Neukum]

Am Montag, 5. September 2011, 18:36:54 schrieb Jim Wylder:
> Oliver,
> 
> I have a couple of questions about this:
> 
> - Does the macro as implemented properly communicate the intent as you
> requested?

Yes.

> - You made a comment about this being useful outside of usbnet.  The
> only appropriate place that I could identify for this would be all the
> way up in usb.h.  Do you think something at that level is more
> appropriate?

It is specific to usb. It belongs in the core device code.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - -