All of lore.kernel.org
 help / color / mirror / Atom feed
* [cdc_ncm] guidance and help refactoring cdc_ncm
@ 2015-05-31 14:37 Enrico Mioso
       [not found] ` <alpine.LNX.2.20.1505311628420.16456-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Enrico Mioso @ 2015-05-31 14:37 UTC (permalink / raw)
  To: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	Oliver Neukum
  Cc: Greg KH

Hello guys.
I am writing to you all to ask for help and assistance in refactoring the 
cdc_ncm driver to support newer devices.
In particular - I would need step-by-step guidance in doing this: or any other 
kind of help would be anyway greatly apreciated.

1 - What we need:
   We would need to refactor the driver to be able to re-order parts of the NCM
   package itself.
   In particular, being a single NCM frame composed of different parts, we would
   need more flexibility in changing their order.
2 - What might be nice
   To do so, it would be nice to have the driver queue up frames, sending them
   out as needed. this already happens to a certain extent, but the NCM package
   is created in the process and updated in the while as I understood the code.
   The best thing would be to have the NCM package created only before sending
   it out, to achieve for best performance and code readability.

I already contactedprivately some of you to have some more insight on what 
needs to be done, and to understand better how to organize the effort. I 
unfortunately miss the time to do this right now: and infact I can't even be 
sure to be able to do this, due to various problems (my tesis, my life in 
general).
But gathering more informations and in general trying to get some help is the 
best thing I feel like doing right now.

The compelling reasons I find for trying to fix the situation are:
1 - The fact these drivers are used in different products integrating or
   interfacing with 3G/4G technologies.
2 - To gain more flexibility in the long term.

Thank you guys for reading this message and everything.
Please keep me in CC since I am not subscribed to this list.

Enrico Mioso
My Tox ID is: 7C593F402A3C8632D87AB4B948D492294C39A6A614464ECF843CA3429FB023284180472C7475

I like / reocmmend usage of open messaging standards: my preferred XMPP ID
(JID) is: mrkiko-aHS423dKhWw@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found] ` <alpine.LNX.2.20.1505311628420.16456-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2015-06-01  0:59   ` Greg KH
       [not found]     ` <20150601005917.GB5320-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2015-06-01  0:59 UTC (permalink / raw)
  To: Enrico Mioso
  Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	Oliver Neukum

On Sun, May 31, 2015 at 04:37:11PM +0200, Enrico Mioso wrote:
> Hello guys.
> I am writing to you all to ask for help and assistance in refactoring the
> cdc_ncm driver to support newer devices.
> In particular - I would need step-by-step guidance in doing this: or any
> other kind of help would be anyway greatly apreciated.
> 
> 1 - What we need:
>   We would need to refactor the driver to be able to re-order parts of the NCM
>   package itself.
>   In particular, being a single NCM frame composed of different parts, we would
>   need more flexibility in changing their order.

Do you have hardware that needs this now?  What exactly needs to be done
here that currently doesn't work?

> 2 - What might be nice
>   To do so, it would be nice to have the driver queue up frames, sending them
>   out as needed. this already happens to a certain extent, but the NCM package
>   is created in the process and updated in the while as I understood the code.
>   The best thing would be to have the NCM package created only before sending
>   it out, to achieve for best performance and code readability.

Would this really make things faster?

> I already contactedprivately some of you to have some more insight on what
> needs to be done, and to understand better how to organize the effort. I
> unfortunately miss the time to do this right now: and infact I can't even be
> sure to be able to do this, due to various problems (my tesis, my life in
> general).
> But gathering more informations and in general trying to get some help is
> the best thing I feel like doing right now.
> 
> The compelling reasons I find for trying to fix the situation are:
> 1 - The fact these drivers are used in different products integrating or
>   interfacing with 3G/4G technologies.

Is there hardware that has out-of-tree drivers that implement what you
are referring to here?  Or does someone just want this to make the
hardware work "better"?

I think we need more specifics before being able to determine exactly
what needs to be done.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found]     ` <20150601005917.GB5320-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
@ 2015-06-01  6:53       ` Enrico Mioso
       [not found]         ` <alpine.LNX.2.20.1506010818110.3158-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Enrico Mioso @ 2015-06-01  6:53 UTC (permalink / raw)
  To: Greg KH
  Cc: Enrico Mioso, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, Oliver Neukum,
	youtux-Re5JQEeQqe8AvxtiuMwx3w

Hello Greg, hello everyone reading.
I am sorry If I wasn't specific enough - I'll be this time! :) Or I try at 
least.

On Mon, 1 Jun 2015, Greg KH wrote:

==Date: Mon, 1 Jun 2015 02:59:17
==From: Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
==To: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
==Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
==    Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Sun, May 31, 2015 at 04:37:11PM +0200, Enrico Mioso wrote:
==> Hello guys.
==> I am writing to you all to ask for help and assistance in refactoring the
==> cdc_ncm driver to support newer devices.
==> In particular - I would need step-by-step guidance in doing this: or any
==> other kind of help would be anyway greatly apreciated.
==> 
==> 1 - What we need:
==>   We would need to refactor the driver to be able to re-order parts of the NCM
==>   package itself.
==>   In particular, being a single NCM frame composed of different parts, we would
==>   need more flexibility in changing their order.
==
==Do you have hardware that needs this now?  What exactly needs to be done
==here that currently doesn't work?

yes - there is hardware that curently doesn't work with the actual code.
In particular, I am referring to Huawei 3G / 4G modems:
- Huawei E3131: will not work with some firmware versions, works with others / 
  olders ones
- Huawei E3372 (LTE modem): will not work.

I received various mail messages from people trying to configure different 
devices that aren't working: and partly the situation is confusing since 
sometimes devices with very similar product names are pretty different, or 
derive from different hardware branches.

Regarding what needs to be done: it's important to note that those devices 
follow an USB specification.
the network control Model spec as found here:
http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip
aims to provide more efficient netowkring over USB solutions, batching frames 
for example.
The fundamental packet unit here is the "NCM Package": which can hold more 
ethernet-style frames in it. The device and the host will negotiate once 
appropriate some frame characteristics.

The specification doesn't mandate where some parts of the package should be 
placed: infact, they can be put in somewhat arbitrary places.

This is true for the NDP part: we actually as I understand it, are putting it 
near the beginning of the frame.
Some Huawei devices, started to mandate it to be at the end of it, ignoring the 
frame otherwise.

==
==> 2 - What might be nice
==>   To do so, it would be nice to have the driver queue up frames, sending them
==>   out as needed. this already happens to a certain extent, but the NCM package
==>   is created in the process and updated in the while as I understood the code.
==>   The best thing would be to have the NCM package created only before sending
==>   it out, to achieve for best performance and code readability.
==
==Would this really make things faster?

Probably it would, depending on the setup we are considering. Considering a 
standard setup where those devices are being connected to a laptop or a PC with 
relatively high resources, this would not make so much difference.
But it's not so unusual to see these devices running coupled with small 
devices: and here this could make the difference in some cases.
But this would not be my main goal: getting things working faster is good, but 
I would like just to see them working now: and so I am trying to gather help / 
information / guidance / code in general so in case I might try if needed in 
the future.

==
==> I already contactedprivately some of you to have some more insight on what
==> needs to be done, and to understand better how to organize the effort. I
==> unfortunately miss the time to do this right now: and infact I can't even be
==> sure to be able to do this, due to various problems (my tesis, my life in
==> general).
==> But gathering more informations and in general trying to get some help is
==> the best thing I feel like doing right now.
==> 
==> The compelling reasons I find for trying to fix the situation are:
==> 1 - The fact these drivers are used in different products integrating or
==>   interfacing with 3G/4G technologies.
==
==Is there hardware that has out-of-tree drivers that implement what you
==are referring to here?  Or does someone just want this to make the
==hardware work "better"?
==
==I think we need more specifics before being able to determine exactly
==what needs to be done.
==
==thanks,
==
==greg k-h
==
Thank you Greg for your precious help.

Once again - some devices will just not work.

There is an out-of-tree vendor driver implementing what I am referring to: it 
contains code to work with many different devices from Huawei, but only the NCM 
related parts would be of use in this scenario. Other devices are already 
supported and working in the kernel.
the driver can be found here:
https://dl.dropboxusercontent.com/u/15043728/ArchLinuxArm/huawei_ndis/ndis_src.tar.xz

it works with kernels up to 3.18. then it doesn't, since there are some 
problems with the power management (PM) code in here.
As can be seen, there is various code coming from usbnet and the linu ncm 
headers.

All the rest as I can see it, isn't so important: the specification talks about 
different possibilities we are not implementing currently, but I think we can 
live without them if no other devices requiring them gest created. It remains 
to be seen if at some point we'll need additional features. Actually we are 
anyway deviating from the Huawei Windows driver, because we use 16-bit NCM 
while it uses the 32-bit version, we are not supporting CRC on frames and other 
things.
I think we can leave them out.
A 32-bit version of the driver (talking 32-bit NCM) is here:
http://www.gstorm.eu/cdc_ncm.c
I modified the original driver with the help of a very talented friend.
It works: but there seem to be no real reasons to implement this properly. We 
did this in a consistent effort to understand what was wrong, and here it is.
Thank you again guys for reading this messae.

Enrico
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found]         ` <alpine.LNX.2.20.1506010818110.3158-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2015-06-01  7:48           ` Oliver Neukum
       [not found]             ` <1433144906.1557.2.camel-IBi9RG/b67k@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Oliver Neukum @ 2015-06-01  7:48 UTC (permalink / raw)
  To: Enrico Mioso
  Cc: Greg KH, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, youtux-Re5JQEeQqe8AvxtiuMwx3w

On Mon, 2015-06-01 at 08:53 +0200, Enrico Mioso wrote:

> A 32-bit version of the driver (talking 32-bit NCM) is here:
> http://www.gstorm.eu/cdc_ncm.c
> I modified the original driver with the help of a very talented friend.
> It works: but there seem to be no real reasons to implement this properly. We 
> did this in a consistent effort to understand what was wrong, and here it is.

Well, you are really talking about two different things here.
Do we ever fail because we only do 16 bit as opposed to 32 bit?
Your 32 bit driver looks good, but it raises the question of what to do
if this test:

        if (le16_to_cpu(ctx->ncm_parm.bmNtbFormatsSupported) &
                                                USB_CDC_NCM_NTB32_SUPPORTED) {

fails. Otherwise it looks ready for merging.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found]             ` <1433144906.1557.2.camel-IBi9RG/b67k@public.gmane.org>
@ 2015-06-01  8:24               ` Enrico Mioso
  2015-06-01 10:49                 ` Oliver Neukum
  0 siblings, 1 reply; 10+ messages in thread
From: Enrico Mioso @ 2015-06-01  8:24 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg KH, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, youtux-Re5JQEeQqe8AvxtiuMwx3w

Thank you Oliver for the reply.


On Mon, 1 Jun 2015, Oliver Neukum wrote:

==Date: Mon, 1 Jun 2015 09:48:26
==From: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
==To: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
==Cc: Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
==    netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, youtux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Mon, 2015-06-01 at 08:53 +0200, Enrico Mioso wrote:
==
==> A 32-bit version of the driver (talking 32-bit NCM) is here:
==> http://www.gstorm.eu/cdc_ncm.c
==> I modified the original driver with the help of a very talented friend.
==> It works: but there seem to be no real reasons to implement this properly. We 
==> did this in a consistent effort to understand what was wrong, and here it is.
==
==Well, you are really talking about two different things here.
==Do we ever fail because we only do 16 bit as opposed to 32 bit?
==Your 32 bit driver looks good, but it raises the question of what to do
==if this test:
==
==        if (le16_to_cpu(ctx->ncm_parm.bmNtbFormatsSupported) &
==                                                USB_CDC_NCM_NTB32_SUPPORTED) {
==
==fails. Otherwise it looks ready for merging.
==
==	Regards
==		Oliver
==
==
==
==

Oh - I am sorry. Infact, I am taling about two different things here. No, I've 
seen no device failing because of this (16 vs. 32 bit problem).
I was mentioning this only as an additional thing to consider, at least from my 
side.
We are failing on some cases only because of the position we put the NDP part 
of the NCM frame. Infact, this 32-bit driver will work when the 16 bit one 
does, and fail when the 16 bit one does.
Thank you for your review and consideration.
It would be nice to see this code merged - but I think we might need to merge 
the two drivers in a way that reduces code de-duplication. But this might be 
work for a second take in case.
Thank you again,
Enrico
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
  2015-06-01  8:24               ` Enrico Mioso
@ 2015-06-01 10:49                 ` Oliver Neukum
       [not found]                   ` <1433155778.1884.12.camel-l3A5Bk7waGM@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Oliver Neukum @ 2015-06-01 10:49 UTC (permalink / raw)
  To: Enrico Mioso; +Cc: youtux, Greg KH, linux-usb, netdev

On Mon, 2015-06-01 at 10:24 +0200, Enrico Mioso wrote:
> We are failing on some cases only because of the position we put the
> NDP part 
> of the NCM frame. Infact, this 32-bit driver will work when the 16 bit
> one 
> does, and fail when the 16 bit one does.

I think the discussion would benefit from a clearer explanation
how those devices need an aggregate to look and how our aggregates
look like.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found]                   ` <1433155778.1884.12.camel-l3A5Bk7waGM@public.gmane.org>
@ 2015-06-01 11:41                     ` Enrico Mioso
  2015-06-01 12:00                       ` Oliver Neukum
  0 siblings, 1 reply; 10+ messages in thread
From: Enrico Mioso @ 2015-06-01 11:41 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: youtux-Re5JQEeQqe8AvxtiuMwx3w, Greg KH,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA

Thank you Oliver, thank you all for reading this thread and the attention.
For a more detailed discussion and how we got here, you might google for the 
thread:
"Is this 32-bit NCM?"
and
"Is this 32-bit NCM?y" (follow up).
Where the "y" letter comes from a mistake of mine.

The specification does only mandate the position of the NTH (header). The rest 
can be in any order and position in general. This will work with most devices: 
except, of course, those from Huawei.
Our aggregate looks something like this from my perspective (anyone correct me 
in case):
NTH: header
NDP: contains indexing informations
ethernet packet 1
ethernet packet 2
...
ethernet packet n;

While it should look like:
NTH: header
ethernet packet 1
ethernet packet 2
...
ethernet packet n;
NDP: contains indexing informations

but, when introducing such a change: you might break other devices now working. 
Infact, clearly there are multiple vendors producing NCM device, as you might 
also see by looking at the dirver's comments.
So in general, we should be able to dynamically change the way in which the 
driver order things in the package.
and that's why I initially proposed the "redesign".

thank you guys, for the patience and time.
Enrico
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
  2015-06-01 11:41                     ` Enrico Mioso
@ 2015-06-01 12:00                       ` Oliver Neukum
       [not found]                         ` <1433160022.1884.18.camel-IBi9RG/b67k@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Oliver Neukum @ 2015-06-01 12:00 UTC (permalink / raw)
  To: Enrico Mioso; +Cc: youtux, Greg KH, linux-usb, netdev

On Mon, 2015-06-01 at 13:41 +0200, Enrico Mioso wrote:
> Thank you Oliver, thank you all for reading this thread and the attention.
> For a more detailed discussion and how we got here, you might google for the 
> thread:
> "Is this 32-bit NCM?"
> and
> "Is this 32-bit NCM?y" (follow up).
> Where the "y" letter comes from a mistake of mine.

Having read them it looks like the issues of padding and
sequence numbers are open.

> The specification does only mandate the position of the NTH (header). The rest 
> can be in any order and position in general. This will work with most devices: 
> except, of course, those from Huawei.

Indeed. And a redesign for crap devices looks like
a bad idea.

> Our aggregate looks something like this from my perspective (anyone correct me 
> in case):
> NTH: header
> NDP: contains indexing informations
> ethernet packet 1
> ethernet packet 2
> ...
> ethernet packet n;
> 
> While it should look like:
> NTH: header
> ethernet packet 1
> ethernet packet 2
> ...
> ethernet packet n;
> NDP: contains indexing informations
> 
> but, when introducing such a change: you might break other devices now working. 
> Infact, clearly there are multiple vendors producing NCM device, as you might 
> also see by looking at the dirver's comments.
> So in general, we should be able to dynamically change the way in which the 
> driver order things in the package.
> and that's why I initially proposed the "redesign".

OK, so the NDP needs to be at the end. However in the old thread
you state that this requires the NDP to be built between the
final aggregate and physically transmitting. I think this is a false
choice. You could just as well copy the NDP around provided you reserve
enough space at the end of the skb.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [cdc_ncm] guidance and help refactoring cdc_ncm
       [not found]                         ` <1433160022.1884.18.camel-IBi9RG/b67k@public.gmane.org>
@ 2015-06-01 12:08                           ` Enrico Mioso
  2015-06-02  5:46                           ` [RFC cdc_ncm] introducing allocation mode Enrico Mioso
  1 sibling, 0 replies; 10+ messages in thread
From: Enrico Mioso @ 2015-06-01 12:08 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: youtux-Re5JQEeQqe8AvxtiuMwx3w, Greg KH,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA


On Mon, 1 Jun 2015, Oliver Neukum wrote:

==Date: Mon, 1 Jun 2015 14:00:22
==From: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
==To: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
==Cc: youtux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
==    netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Mon, 2015-06-01 at 13:41 +0200, Enrico Mioso wrote:
==> Thank you Oliver, thank you all for reading this thread and the attention.
==> For a more detailed discussion and how we got here, you might google for the 
==> thread:
==> "Is this 32-bit NCM?"
==> and
==> "Is this 32-bit NCM?y" (follow up).
==> Where the "y" letter comes from a mistake of mine.
==
==Having read them it looks like the issues of padding and
==sequence numbers are open.
==
==> The specification does only mandate the position of the NTH (header). The rest 
==> can be in any order and position in general. This will work with most devices: 
==> except, of course, those from Huawei.
==
==Indeed. And a redesign for crap devices looks like
==a bad idea.
==
==> Our aggregate looks something like this from my perspective (anyone correct me 
==> in case):
==> NTH: header
==> NDP: contains indexing informations
==> ethernet packet 1
==> ethernet packet 2
==> ...
==> ethernet packet n;
==> 
==> While it should look like:
==> NTH: header
==> ethernet packet 1
==> ethernet packet 2
==> ...
==> ethernet packet n;
==> NDP: contains indexing informations
==> 
==> but, when introducing such a change: you might break other devices now working. 
==> Infact, clearly there are multiple vendors producing NCM device, as you might 
==> also see by looking at the dirver's comments.
==> So in general, we should be able to dynamically change the way in which the 
==> driver order things in the package.
==> and that's why I initially proposed the "redesign".
==
==OK, so the NDP needs to be at the end. However in the old thread
==you state that this requires the NDP to be built between the
==final aggregate and physically transmitting. I think this is a false
==choice. You could just as well copy the NDP around provided you reserve
==enough space at the end of the skb.
Yes, probably you can do so. I am not against anything at this moment.
==
==	Regards
==		Oliver
==
==
==
==
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC cdc_ncm] introducing allocation mode
       [not found]                         ` <1433160022.1884.18.camel-IBi9RG/b67k@public.gmane.org>
  2015-06-01 12:08                           ` Enrico Mioso
@ 2015-06-02  5:46                           ` Enrico Mioso
  1 sibling, 0 replies; 10+ messages in thread
From: Enrico Mioso @ 2015-06-02  5:46 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Enrico Mioso, youtux-Re5JQEeQqe8AvxtiuMwx3w, Greg KH,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA

First of all - hello everyone, and thank you for the help.
I am here to learn something - so I am happy about having the opportunity of 
discussing this with you. I am a newbie in development, sure.

On Mon, 1 Jun 2015, Oliver Neukum wrote:

==Date: Mon, 1 Jun 2015 14:00:22
==From: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
==To: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
==Cc: youtux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
==    netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Mon, 2015-06-01 at 13:41 +0200, Enrico Mioso wrote:
==> Thank you Oliver, thank you all for reading this thread and the attention.
==> For a more detailed discussion and how we got here, you might google for the 
==> thread:
==> "Is this 32-bit NCM?"
==> and
==> "Is this 32-bit NCM?y" (follow up).
==> Where the "y" letter comes from a mistake of mine.
==
==Having read them it looks like the issues of padding and
==sequence numbers are open.
==
No - we tested the sequence number thing - and it didn't influence the device 
behaviour.
We also tested the padding if I am not wrong: with the same results. Bjorn Mork 
did a great job in making the cdc_ncm driver more flexible and rendering 
debugging so much easier. I thank him.

==> The specification does only mandate the position of the NTH (header). The rest 
==> can be in any order and position in general. This will work with most devices: 
==> except, of course, those from Huawei.
==
==Indeed. And a redesign for crap devices looks like
==a bad idea.
==
==> Our aggregate looks something like this from my perspective (anyone correct me 
==> in case):
==> NTH: header
==> NDP: contains indexing informations
==> ethernet packet 1
==> ethernet packet 2
==> ...
==> ethernet packet n;
==> 
==> While it should look like:
==> NTH: header
==> ethernet packet 1
==> ethernet packet 2
==> ...
==> ethernet packet n;
==> NDP: contains indexing informations
==> 
==> but, when introducing such a change: you might break other devices now working. 
==> Infact, clearly there are multiple vendors producing NCM device, as you might 
==> also see by looking at the dirver's comments.
==> So in general, we should be able to dynamically change the way in which the 
==> driver order things in the package.
==> and that's why I initially proposed the "redesign".
==
==OK, so the NDP needs to be at the end. However in the old thread
==you state that this requires the NDP to be built between the
==final aggregate and physically transmitting. I think this is a false
==choice. You could just as well copy the NDP around provided you reserve
==enough space at the end of the skb.
==
==	Regards
==		Oliver
==
==
==
==

This might be true - so let's work on it. Reading the code I realized 
that the cdc_ncm_ndp() (which btw I would like to have renamed to cdc_ncm_ndp16 
as I did in a previous patchset), does something interesting here.
So - the function serches the already prepared frame for an appropriate NDP, 
as the comment suggests. It uses the signature to find one. Until now, moving 
the NDP at the end would pose no problems, even doing so dynamically (that's 
the final goal).
If the appropriate ndp is found, we return it, going ahead if this doesn't 
happen, updating our offset accordingly.
We return the ndp16 anyway for the function calling us to use it as reference.
Infact, the real copying of the ndp in the skb asI understand is happening 
here:
ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb, ctx->max_ndp_size), 0, 
ctx->max_ndp_size);

Infact we call cdc_ncm_align_tail (you can observe we call it here and also in 
cdc_ncm_fill_tx_frame()).

So I am trying to change this, having this function work in two mode:
- the direct allocation mode: that's how it works now
- the non direct allocation mode: where the caller should do this work

What do you think about this ? 

--
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..bcd919d 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -978,9 +978,9 @@ static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remai
 }
 
 /* return a pointer to a valid struct usb_cdc_ncm_ndp16 of type sign, possibly
- * allocating a new one within skb
+ * allocating a new one within skb if in direct allocation mode (normal mode of operation).
  */
-static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign, size_t reserve)
+static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign, size_t reserve, unsigned int direct_allocation)
 {
 	struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
 	struct usb_cdc_ncm_nth16 *nth16 = (void *)skb->data;
@@ -994,8 +994,9 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_
 		ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
 	}
 
-	/* align new NDP */
-	cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_max);
+	/* align new NDP in case we're going to allocate it */
+	if (direct_allocation)
+	  cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_max);
 
 	/* verify that there is room for the NDP and the datagram (reserve) */
 	if ((ctx->tx_max - skb->len - reserve) < ctx->max_ndp_size)
@@ -1008,10 +1009,12 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_
 		nth16->wNdpIndex = cpu_to_le16(skb->len);
 
 	/* push a new empty NDP */
-	ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb, ctx->max_ndp_size), 0, ctx->max_ndp_size);
-	ndp16->dwSignature = sign;
-	ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16));
-	return ndp16;
+	if (direct_allocation) {
+	  ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb, ctx->max_ndp_size), 0, ctx->max_ndp_size);
+  	ndp16->dwSignature = sign;
+  	ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16));
+  	return ndp16;
+  }
 }
 
 struct sk_buff *
@@ -1071,7 +1074,7 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 		}
 
 		/* get the appropriate NDP for this skb */
-		ndp16 = cdc_ncm_ndp(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
+		ndp16 = cdc_ncm_ndp(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder, 0);
 
 		/* align beginning of next frame */
 		cdc_ncm_align_tail(skb_out,  ctx->tx_modulus, ctx->tx_remainder, ctx->tx_max);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-06-02  5:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-31 14:37 [cdc_ncm] guidance and help refactoring cdc_ncm Enrico Mioso
     [not found] ` <alpine.LNX.2.20.1505311628420.16456-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2015-06-01  0:59   ` Greg KH
     [not found]     ` <20150601005917.GB5320-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2015-06-01  6:53       ` Enrico Mioso
     [not found]         ` <alpine.LNX.2.20.1506010818110.3158-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2015-06-01  7:48           ` Oliver Neukum
     [not found]             ` <1433144906.1557.2.camel-IBi9RG/b67k@public.gmane.org>
2015-06-01  8:24               ` Enrico Mioso
2015-06-01 10:49                 ` Oliver Neukum
     [not found]                   ` <1433155778.1884.12.camel-l3A5Bk7waGM@public.gmane.org>
2015-06-01 11:41                     ` Enrico Mioso
2015-06-01 12:00                       ` Oliver Neukum
     [not found]                         ` <1433160022.1884.18.camel-IBi9RG/b67k@public.gmane.org>
2015-06-01 12:08                           ` Enrico Mioso
2015-06-02  5:46                           ` [RFC cdc_ncm] introducing allocation mode Enrico Mioso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.