diff mbox

[net] net: cdc_ncm: workaround for missing CDC Union

Message ID 87mww2y4ni.fsf@nemi.mork.no
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Bjørn Mork Jan. 21, 2013, 2:47 p.m. UTC 
Bjørn Mork <bjorn@mork.no> writes:

> I have another issue with the Sierra firmware which I hope you can help
> me with: The MC7710 device requires at ZLP even if we send
> dwNtbOutMaxSize sized NTBs. This is a problem because the current code
> explicitly prevents this.

If anyone found this more than normally confused, then that is
correct...

What the NCM code does is that it sends a *short* packet whenever the
usbnet would normally have done so, except if the NTP length >=
dwNtbOutMaxSize.

The problem is that we do not set 

	urb->transfer_flags |= URB_ZERO_PACKET;

either in this special case, resulting in neither a short nor a zero
length packet being sent.  I believe this is what the Sierra firmware
chokes at. This seems to fix the issue, and is IMHO correct:




But I wonder if this isn't really a generic problem in usbnet.  The
FLAG_MULTI_PACKET test here seems completely bogus:

	if (length % dev->maxpacket == 0) {
		if (!(info->flags & FLAG_SEND_ZLP)) {
			if (!(info->flags & FLAG_MULTI_PACKET)) {
				urb->transfer_buffer_length++;
				if (skb_tailroom(skb)) {
					skb->data[skb->len] = 0;
					__skb_put(skb, 1);
				}
			}
		} else
			urb->transfer_flags |= URB_ZERO_PACKET;
	}

Either the FLAG_MULTI_PACKET minidriver will have already padded the
buffer so that we do not hit (length % dev->maxpacket == 0), or we
should choose one of the alternatives: ZLP or padding.

Minidrivers not wanting the short packets (like cdc_ncm) must set
FLAG_SEND_ZLP.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Oliver Neukum Jan. 21, 2013, 2:55 p.m. UTC | #1 
On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
> But I wonder if this isn't really a generic problem in usbnet.  The
> FLAG_MULTI_PACKET test here seems completely bogus:
> 
>         if (length % dev->maxpacket == 0) {
>                 if (!(info->flags & FLAG_SEND_ZLP)) {
>                         if (!(info->flags & FLAG_MULTI_PACKET)) {
>                                 urb->transfer_buffer_length++;
>                                 if (skb_tailroom(skb)) {
>                                         skb->data[skb->len] = 0;
>                                         __skb_put(skb, 1);
>                                 }
>                         }
>                 } else
>                         urb->transfer_flags |= URB_ZERO_PACKET;
>         }
> 
> Either the FLAG_MULTI_PACKET minidriver will have already padded the
> buffer so that we do not hit (length % dev->maxpacket == 0), or we
> should choose one of the alternatives: ZLP or padding.

But we cannot simply call __skb_put for a complicated data frame.
Besides you may want the current behavior.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Jan. 21, 2013, 3:28 p.m. UTC | #2 
Oliver Neukum <oneukum@suse.de> writes:
> On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
>> But I wonder if this isn't really a generic problem in usbnet.  The
>> FLAG_MULTI_PACKET test here seems completely bogus:
>> 
>>         if (length % dev->maxpacket == 0) {
>>                 if (!(info->flags & FLAG_SEND_ZLP)) {
>>                         if (!(info->flags & FLAG_MULTI_PACKET)) {
>>                                 urb->transfer_buffer_length++;
>>                                 if (skb_tailroom(skb)) {
>>                                         skb->data[skb->len] = 0;
>>                                         __skb_put(skb, 1);
>>                                 }
>>                         }
>>                 } else
>>                         urb->transfer_flags |= URB_ZERO_PACKET;
>>         }
>> 
>> Either the FLAG_MULTI_PACKET minidriver will have already padded the
>> buffer so that we do not hit (length % dev->maxpacket == 0), or we
>> should choose one of the alternatives: ZLP or padding.
>
> But we cannot simply call __skb_put for a complicated data frame.

Agreed.  But I believe the condition should be 

 if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
     ..
 } else {
     urb->transfer_flags |= URB_ZERO_PACKET;
 }

to ensure that we send the ZLP in this case.

> Besides you may want the current behavior.

Why? Does it ever make sense to prevent both the short packet and the
ZLP?


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexey ORISHKO Jan. 21, 2013, 5:59 p.m. UTC | #3 
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.de]
> On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
> > But I wonder if this isn't really a generic problem in usbnet.  The
> > FLAG_MULTI_PACKET test here seems completely bogus:
> >
> >         if (length % dev->maxpacket == 0) {
> >                 if (!(info->flags & FLAG_SEND_ZLP)) {
> >                         if (!(info->flags & FLAG_MULTI_PACKET)) {
> >                                 urb->transfer_buffer_length++;
> >                                 if (skb_tailroom(skb)) {
> >                                         skb->data[skb->len] = 0;
> >                                         __skb_put(skb, 1);
> >                                 }
> >                         }
> >                 } else
> >                         urb->transfer_flags |= URB_ZERO_PACKET;
> >         }
> >
> > Either the FLAG_MULTI_PACKET minidriver will have already padded the
> > buffer so that we do not hit (length % dev->maxpacket == 0), or we
> > should choose one of the alternatives: ZLP or padding.
> 
> But we cannot simply call __skb_put for a complicated data frame.
> Besides you may want the current behavior.
> 

Specification says: 
NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.

The problem is: 
dwNtbOutMaxSize value is negotiated between host and device NCM/MBIM
entities and usbnet has no knowledge about it.

Adding one byte to make buffer looking like a short packet was most
simple approach instead of inventing a way to communicate 
dwNtbOutMaxSize to usbnet.

You could drop short packet approach if dwNtbOutMaxSize is provided
to usbnet and decision is made accordingly to NCM/MBIM spec (with
exception to faulty devices).

Regards,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum Jan. 22, 2013, 8:50 a.m. UTC | #4 
On Monday 21 January 2013 16:28:48 Bjørn Mork wrote:

> Agreed.  But I believe the condition should be 
> 
>  if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
>      ..
>  } else {
>      urb->transfer_flags |= URB_ZERO_PACKET;
>  }
> 
> to ensure that we send the ZLP in this case.

Why? If a driver wants ZLP, it can set FLAG_SEND_ZLP. Your proposed change
would take away an option from drivers without any gain.

> > Besides you may want the current behavior.
> 
> Why? Does it ever make sense to prevent both the short packet and the
> ZLP?

Why not? It is possible and a driver may want it, so why forbid it?
Especially, as in theory it takes least bandwidth as a solution.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum Jan. 22, 2013, 9:07 a.m. UTC | #5 
On Monday 21 January 2013 18:59:05 Alexey ORISHKO wrote:
> Specification says: 
> NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.
> 
> The problem is: 
> dwNtbOutMaxSize value is negotiated between host and device NCM/MBIM
> entities and usbnet has no knowledge about it.
> 
> Adding one byte to make buffer looking like a short packet was most
> simple approach instead of inventing a way to communicate 
> dwNtbOutMaxSize to usbnet.
> 
> You could drop short packet approach if dwNtbOutMaxSize is provided
> to usbnet and decision is made accordingly to NCM/MBIM spec (with
> exception to faulty devices).

We could do that. Feel free to send a patch. ;-)

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum Jan. 22, 2013, 9:32 a.m. UTC | #6 
On Monday 21 January 2013 18:59:05 Alexey ORISHKO wrote:

> > > Either the FLAG_MULTI_PACKET minidriver will have already padded the
> > > buffer so that we do not hit (length % dev->maxpacket == 0), or we
> > > should choose one of the alternatives: ZLP or padding.
> > 
> > But we cannot simply call __skb_put for a complicated data frame.
> > Besides you may want the current behavior.
> > 
> 
> Specification says: 
> NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.

Hi,

one thing on a generic level, which I hope you don't take wrong, but I need
to make it clear. Usbnet is for every driver. What a spec says
or what the reality of implementations of that spec looks like, determine
what usbnet needs to support.

But what is not needed to implement that spec doesn't matter in deciding
what usbnet needs not support. Drivers may or may not need ZLPs.

	Regards
		Oliver
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Jan. 22, 2013, 10:35 a.m. UTC | #7 
Oliver Neukum <oneukum@suse.de> writes:
> On Monday 21 January 2013 16:28:48 Bjørn Mork wrote:
>
>> Agreed.  But I believe the condition should be 
>> 
>>  if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
>>      ..
>>  } else {
>>      urb->transfer_flags |= URB_ZERO_PACKET;
>>  }
>> 
>> to ensure that we send the ZLP in this case.
>
> Why? If a driver wants ZLP, it can set FLAG_SEND_ZLP. Your proposed change
> would take away an option from drivers without any gain.

OK.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 42f51c7..3a5673a 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -366,7 +366,7 @@  err:
 
 static const struct driver_info cdc_mbim_info = {
        .description = "CDC MBIM",
-       .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
+       .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN | FLAG_SEND_ZLP,
        .bind = cdc_mbim_bind,
        .unbind = cdc_mbim_unbind,
        .manage_power = cdc_mbim_manage_power,