Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Ok, for now I should let go of this - might be it was a little bit too much for the time constaints I am having. But thank you for your effort Oliver. Anyway, trying to write this new TX engine helped me in understand better things and conceive the patch I sent you affecting cdc_ncm.c, which triggers some OOPSes but not in a qemu vm, so I can't read them actually. Any hint on my code is always apreciated, regardless. Enrico -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function
will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become
very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to
become
a little bit more convoluted.
You cannot become any more convoluted even if you try.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx-tx_rem_skb);
swap(sign, ctx-tx_rem_sign);
} else {
ready2send = 1;
}
The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.
I doubt you can can come up with anything more convoluted
but it simplifies locking.
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
Sounds like a plan.
But to prevent usbnet from submittinx RX'd packets, I should be doing
something
nasty here. And unfortunately don't understand why.
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info-tx_fixup) {
skb = info-tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info-flags FLAG_MULTI_PACKET)
goto not_drop;
So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver!
I wish sincerely to thank you again for your time and patience.
On Fri, 26 Jun 2015, Oliver Neukum wrote:
Date: Fri, 26 Jun 2015 10:14:02
From: Oliver Neukum oneu...@suse.com
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.
You cannot become any more convoluted even if you try.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx-tx_rem_skb);
swap(sign, ctx-tx_rem_sign);
} else {
ready2send = 1;
}
The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.
I doubt you can can come up with anything more convoluted
but it simplifies locking.
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
Sounds like a plan.
But to prevent usbnet from submittinx RX'd packets, I should be doing something
nasty here. And unfortunately don't understand why.
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info-tx_fixup) {
skb = info-tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info-flags FLAG_MULTI_PACKET)
goto not_drop;
So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.
Unfortuantely I might have not explained myself better.
I should thank you for this illumination on the timer part: since I know I'll
need this.
But what's stopping me right now is actually the RX, not the TX part.
Sure, the RX function I am using comes from cdc_ncm.c and works fine:
unfortunately, usbnet will not call it.
Infact, my dmesg ends up FULL of
kevent 2 may have been dropped messages.
And looking around I discovered I end up triggering a memory check in usbnet.c,
at line 475
skb = __netdev_alloc_skb_ip_align(dev-net, size, flags
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
This is something done in cdc_ncm.c for example.
But here I have a question: by reading the comment in file
drivers/net/usb/rndis_host.c at line 572, there seem to be different opinions
in this matter.
That is a very old comment written for much slower devices.
rndis_host doesn't get much love nowadays.
What to do ?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length are known a-priori. */
+ nth16-dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+ nth16-wHeaderLength = cpu_to_le16(sizeof(struct
usb_cdc_ncm_nth16));
+
+ /* TX sequence numbering */
+ nth16-wSequence = cpu_to_le16(ctx-tx_seq++);
+
+ /* Forget about previous SKB NDP */
+ drvstate-skb_ndp = NULL;
This is probably better done after you know you cannot fail.
Sure. Thank you.
+
+ /* Allocate a new NDP */
+ ndp16 = kzalloc(ctx-max_ndp_size, GFP_NOIO);
Where is this freed?
The intention wqas to free it in the NCM_COMMIT_NDP case.
Infact after allocating the pointer, I make a copy of it in the driver state
(drvstate) variable, and get back to it later.
Is this wrong?
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver.
Thank you for your patience, and review. I apreciated it very much.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 11:49:29
From: Oliver Neukum oneu...@suse.com
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Tue, 2015-06-23 at 00:32 +0200, Enrico Mioso wrote:
This patch introduces a new NCM tx engine, able to operate in standard-
and huawei-style mode.
In the first case, the NDP is disposed after the initial headers and
before any datagram.
What works:
- is able to communicate with compliant NCM devices:
I tested this with a board running the Linux g_ncm gadget driver.
What doesn't work:
- After some packets I start gettint LOTS of EVENT_RX_MEMORY from usbnet,
which fails to allocate an RX SKB in rx_submit(). Don't understand why,
any suggestion would be very welcome.
The tx_fixup function given here, even if actually working, should be
considered as an example: the NCM manager is used here simulating the
cdc_ncm.c behaviour.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/huawei_cdc_ncm.c | 187 ++-
1 file changed, 185 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index 735f7da..217802a 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -29,6 +29,35 @@
#include linux/usb/cdc-wdm.h
#include linux/usb/cdc_ncm.h
+/* NCM management operations: */
+
+/* NCM_INIT_FRAME: prepare for a new frame.
+ * NTH16 header is written to output SKB, NDP data is reset and last
+ * committed NDP pointer set to NULL.
+ * Now, data may be added to this NCM package.
+ */
+#define NCM_INIT_FRAME 1
+
+/* NCM_UPDATE_NDP: adds data to an NDP structure, hence updating it.
+ * Some checks are performed to be sure data fits in, respecting device and
+ * spec constrains.
+ * Normally the NDP is kept in memory and committed to the SKB only when
+ * requested. However, calling this method after NCM_COMMIT_NDP, causes it to
+ * work directly on the already committed SKB copy. this allows for flexibility
+ * in frame ordering.
+ */
+#define NCM_UPDATE_NDP 2
+
+/* Write NDP: commits NDP to output SKB.
+ * This method should be called only once per frame.
+ */
+#define NCM_COMMIT_NDP 3
+
+/* Finalizes NTH16 header: to be called when working in
+ * update-already-committed mode.
+ */
+#define NCM_FINALIZE_NTH 5
+
/* Driver data */
struct huawei_cdc_ncm_state {
struct cdc_ncm_ctx *ctx;
@@ -36,6 +65,16 @@ struct huawei_cdc_ncm_state {
struct usb_driver *subdriver;
struct usb_interface *control;
struct usb_interface *data;
+
+ /* Keeps track of where data starts and ends in SKBs. */
+ int data_start;
+ int data_len;
+
+ /* Last committed NDP for post-commit operations. */
+ struct usb_cdc_ncm_ndp16 *skb_ndp;
+
+ /* Non-committed NDP */
+ struct usb_cdc_ncm_ndp16 *ndp;
};
static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
@@ -53,6 +92,149 @@ static int huawei_cdc_ncm_manage_power(struct usbnet
*usbnet_dev, int on)
return 0;
}
+/* huawei_ncm_mgmt: flexible TX NCM manager.
+ *
+ * Once a non-zero status value is rturned, current frame should be discarded
+ * and operations restarted from scratch.
+ */
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
OR
- we have enough data in the aggregate, and cannot add more.
This is something done in cdc_ncm.c for example.
But here I have a question: by reading the comment in file
drivers/net/usb/rndis_host.c at line 572, there seem to be different opinions
in this matter.
What to do ?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Tue, 2015-06-23 at 00:32 +0200, Enrico Mioso wrote:
This patch introduces a new NCM tx engine, able to operate in standard-
and huawei-style mode.
In the first case, the NDP is disposed after the initial headers and
before any datagram.
What works:
- is able to communicate with compliant NCM devices:
I tested this with a board running the Linux g_ncm gadget driver.
What doesn't work:
- After some packets I start gettint LOTS of EVENT_RX_MEMORY from usbnet,
which fails to allocate an RX SKB in rx_submit(). Don't understand why,
any suggestion would be very welcome.
The tx_fixup function given here, even if actually working, should be
considered as an example: the NCM manager is used here simulating the
cdc_ncm.c behaviour.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/huawei_cdc_ncm.c | 187
++-
1 file changed, 185 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/huawei_cdc_ncm.c
b/drivers/net/usb/huawei_cdc_ncm.c
index 735f7da..217802a 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -29,6 +29,35 @@
#include linux/usb/cdc-wdm.h
#include linux/usb/cdc_ncm.h
+/* NCM management operations: */
+
+/* NCM_INIT_FRAME: prepare for a new frame.
+ * NTH16 header is written to output SKB, NDP data is reset and last
+ * committed NDP pointer set to NULL.
+ * Now, data may be added to this NCM package.
+ */
+#define NCM_INIT_FRAME 1
+
+/* NCM_UPDATE_NDP: adds data to an NDP structure, hence updating it.
+ * Some checks are performed to be sure data fits in, respecting device and
+ * spec constrains.
+ * Normally the NDP is kept in memory and committed to the SKB only when
+ * requested. However, calling this method after NCM_COMMIT_NDP, causes it
to
+ * work directly on the already committed SKB copy. this allows for
flexibility
+ * in frame ordering.
+ */
+#define NCM_UPDATE_NDP 2
+
+/* Write NDP: commits NDP to output SKB.
+ * This method should be called only once per frame.
+ */
+#define NCM_COMMIT_NDP 3
+
+/* Finalizes NTH16 header: to be called when working in
+ * update-already-committed mode.
+ */
+#define NCM_FINALIZE_NTH 5
+
/* Driver data */
struct huawei_cdc_ncm_state {
struct cdc_ncm_ctx *ctx;
@@ -36,6 +65,16 @@ struct huawei_cdc_ncm_state {
struct usb_driver *subdriver;
struct usb_interface *control;
struct usb_interface *data;
+
+ /* Keeps track of where data starts and ends in SKBs. */
+ int data_start;
+ int data_len;
+
+ /* Last committed NDP for post-commit operations. */
+ struct usb_cdc_ncm_ndp16 *skb_ndp;
+
+ /* Non-committed NDP */
+ struct usb_cdc_ncm_ndp16 *ndp;
};
static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
@@ -53,6 +92,149 @@ static int huawei_cdc_ncm_manage_power(struct usbnet
*usbnet_dev, int on)
return 0;
}
+/* huawei_ncm_mgmt: flexible TX NCM manager.
+ *
+ * Once a non-zero status value is rturned, current frame should be discarded
+ * and operations restarted from scratch.
+ */
Is there any advantage in keeping this in a single function?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length are known a-priori. */
+ nth16-dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+ nth16-wHeaderLength = cpu_to_le16(sizeof(struct
usb_cdc_ncm_nth16));
+
+ /* TX sequence numbering */
+ nth16-wSequence = cpu_to_le16(ctx-tx_seq++);
+
+ /* Forget about previous SKB NDP */
+ drvstate-skb_ndp = NULL;
This is probably better done after you know you cannot fail.
+
+ /* Allocate a new NDP */
+ ndp16 = kzalloc(ctx-max_ndp_size, GFP_NOIO);
Where is this freed?
+ if (!ndp16)
+ return ret;
+
+ /* Prepare a new NDP to add data on subsequent calls. */
+ drvstate-ndp = memset(ndp16, 0, ctx-max_ndp_size);
Either kzalloc() or memset(). Using both never makes sense.
+
+ /* Initial NDP length */
+ ndp16-wLength =
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Tue, 2015-06-23 at 00:32 +0200, Enrico Mioso wrote:
+/* XXX rewrite, not multipacket */
Can you explain what you want to do here?
+struct sk_buff *
+huawei_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb_in, gfp_t flags)
{
+ struct huawei_cdc_ncm_state *drvstate = (void *)dev-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct sk_buff *skb_out;
+ int status;
+
+ skb_out = alloc_skb(ctx-tx_max, GFP_ATOMIC);
You must test this for NULL
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
And sure, the interface goes down case is important: didn't think about it.
Thanks for the point.
the only other additional goal is to use the manager in such a way that NDP
will be disposed after frames.
I think that this split between NCM management and tx_fixup makes things more
flexible in general: this is the reason for re-writing it.
This is something done in cdc_ncm.c for example.
But here I have a question: by reading the comment in file
drivers/net/usb/rndis_host.c at line 572, there seem to be different opinions
in this matter.
That is a very old comment written for much slower devices.
rndis_host doesn't get much love nowadays.
Fine.
What to do ?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length are known a-priori. */
+ nth16-dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+ nth16-wHeaderLength = cpu_to_le16(sizeof(struct
usb_cdc_ncm_nth16));
+
+ /* TX sequence numbering */
+ nth16-wSequence = cpu_to_le16(ctx-tx_seq++);
+
+ /* Forget about previous SKB NDP */
+ drvstate-skb_ndp = NULL;
This is probably better done after you know you cannot fail.
Sure. Thank you.
+
+ /* Allocate a new NDP */
+ ndp16 = kzalloc(ctx-max_ndp_size, GFP_NOIO);
Where is this freed?
The intention wqas to free it in the NCM_COMMIT_NDP case.
Infact after allocating the pointer, I make a copy of it in the driver state
(drvstate) variable, and get back to it later.
Is this wrong?
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
But to prevent
