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-usb@vger.kernel.org, net...@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 usbnet from submittinx RX'd packets, I should be doing something
nasty here. And unfortunately don't understand why.
Infact, this problem manifested itself even more aggressively when I kzallocìed
in the _bind() function and kfreed in _unbind. I searched around this "kevent 2
may have been dropped" (rx memory exhaustion) problem, but without finding
significant hints.
Any idea would be welcome.
Thank you Oliver,
everyone.
Enrico
