Message ID | 1435012335-6055-3-git-send-email-mrkiko.rs@gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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.rs@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 = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16)); > + > + /* Frame signature: to be reworked in general. */ > + ndp16->dwSignature = cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN); > + > + ret = 0; > + break; > + > + case NCM_UPDATE_NDP: > + > + if (drvstate->skb_ndp) { > + ndp16 = drvstate->skb_ndp; > + } else { > + ndp16 = drvstate->ndp; > + > + /* Do we have enough space for the data? */ > + if (skb_out->len + ctx->max_ndp_size > ctx->tx_max) > + goto error; > + } > + > + /* Calculate frame number within this NDP */ > + ndplen = le16_to_cpu(ndp16->wLength); > + index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1; > + > + if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) > + goto error; > + > + /* tx_max shouldn't be exceeded after committing. */ > + if (ndplen + sizeof(struct usb_cdc_ncm_dpe16) > ctx->tx_max) > + goto error; > + > + /* Adding a DPT entry. */ > + ndp16->dpe16[index].wDatagramLength = cpu_to_le16(drvstate->data_len); > + ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(drvstate->data_start); > + ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16)); > + > + ret = 0; > + break; > + > + case NCM_COMMIT_NDP: > + > + if (drvstate->skb_ndp) > + goto error; /* Call this only once please. */ > + > + ndp16 = drvstate->ndp; > + > + nth16->wNdpIndex = cpu_to_le16(skb_out->len); > + > + /* "commit" NDP */ > + drvstate->skb_ndp = memcpy(skb_put(skb_out, ctx->max_ndp_size), ndp16, ctx->max_ndp_size); > + > + kfree(ndp16); > + ndp16 = NULL; > + drvstate->ndp = NULL; > + > + case NCM_FINALIZE_NTH: > + > + /* Finalize NTH16 header, setting it's block length */ > + nth16->wBlockLength = cpu_to_le16(skb_out->len); > + > + ret = 0; > + break; > + default: > + break; > + } > + > +error: The purpose of a label is kind of defeated by having a conditional after it. > + if (ret) > + kfree(drvstate->ndp); > + return ret; > +} 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
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <oneukum@suse.com> > To: Enrico Mioso <mrkiko.rs@gmail.com> > Cc: linux-usb@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.rs@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 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? > >> + 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. > True. Sorry - I knew that the "z" in kzalloc stood for ZERO and also looked at the code, but I forgot it at some point. >> + >> + /* Initial NDP length */ >> + ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16)); >> + >> + /* Frame signature: to be reworked in general. */ >> + ndp16->dwSignature = cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN); >> + >> + ret = 0; >> + break; >> + >> + case NCM_UPDATE_NDP: >> + >> + if (drvstate->skb_ndp) { >> + ndp16 = drvstate->skb_ndp; >> + } else { >> + ndp16 = drvstate->ndp; >> + >> + /* Do we have enough space for the data? */ >> + if (skb_out->len + ctx->max_ndp_size > ctx->tx_max) >> + goto error; >> + } >> + >> + /* Calculate frame number within this NDP */ >> + ndplen = le16_to_cpu(ndp16->wLength); >> + index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1; >> + >> + if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) >> + goto error; >> + >> + /* tx_max shouldn't be exceeded after committing. */ >> + if (ndplen + sizeof(struct usb_cdc_ncm_dpe16) > ctx->tx_max) >> + goto error; >> + >> + /* Adding a DPT entry. */ >> + ndp16->dpe16[index].wDatagramLength = cpu_to_le16(drvstate->data_len); >> + ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(drvstate->data_start); >> + ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16)); >> + >> + ret = 0; >> + break; >> + >> + case NCM_COMMIT_NDP: >> + >> + if (drvstate->skb_ndp) >> + goto error; /* Call this only once please. */ >> + >> + ndp16 = drvstate->ndp; >> + >> + nth16->wNdpIndex = cpu_to_le16(skb_out->len); >> + >> + /* "commit" NDP */ >> + drvstate->skb_ndp = memcpy(skb_put(skb_out, ctx->max_ndp_size), ndp16, ctx->max_ndp_size); >> + >> + kfree(ndp16); >> + ndp16 = NULL; >> + drvstate->ndp = NULL; >> + >> + case NCM_FINALIZE_NTH: >> + >> + /* Finalize NTH16 header, setting it's block length */ >> + nth16->wBlockLength = cpu_to_le16(skb_out->len); >> + >> + ret = 0; >> + break; >> + default: >> + break; >> + } >> + >> +error: > > The purpose of a label is kind of defeated by having a conditional after > it. > The purpose here was to be sure to deallocate everything could needed to, even reaching the label from multiple places. I'll look at it again now. >> + if (ret) >> + kfree(drvstate->ndp); >> + return ret; >> +} > > Regards > Oliver > > > Thank you very much 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
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Oliver! And thank you again. I like / recommend the usage of open messaging standards: my preferred XMPP ID (JID) is: mrkiko@jit.si. On Thu, 25 Jun 2015, Oliver Neukum wrote: > Date: Thu, 25 Jun 2015 15:38:46 > From: Oliver Neukum <oneukum@suse.de> > To: Enrico Mioso <mrkiko.rs@gmail.com> > Cc: linux-usb@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 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
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: mrkiko@jit.si. > > On Thu, 25 Jun 2015, Oliver Neukum wrote: > > > Date: Thu, 25 Jun 2015 15:38:46 > > From: Oliver Neukum <oneukum@suse.de> > > To: Enrico Mioso <mrkiko.rs@gmail.com> > > Cc: linux-usb@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <oneukum@suse.com> > To: Enrico Mioso <mrkiko.rs@gmail.com> > Cc: linux-usb@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: mrkiko@jit.si. >> >> On Thu, 25 Jun 2015, Oliver Neukum wrote: >> >>> Date: Thu, 25 Jun 2015 15:38:46 >>> From: Oliver Neukum <oneukum@suse.de> >>> To: Enrico Mioso <mrkiko.rs@gmail.com> >>> Cc: linux-usb@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); if (!skb) { /* triggered */ netif_dbg(dev, rx_err, dev->net, "no rx skb\n"); usbnet_defer_kevent (dev, EVENT_RX_MEMORY); usb_free_urb (urb); return -ENOMEM; } And genuinely, I can't understand why. To test what I was doing wrong, I started with the stock huawei_cdc_ncm.c that works fine. then I started allocating the temporary NDP in the bind function: and the problem started triggering, so I can't RX. Allocating and deallocating the temporary NDP as-neede as happens now, triggers the problem inevitably after some packets: ping works for a while. I was investigating on calls to obtain "private memory" as it happens in the hso.c driver and others, but wanted some feedback. What happens then is I get a worker thread using all the CPU and ksoftirqs/0 going mad. Googling around resulted mainly in people complaining about this same problem with the drivers/net/usb/smsc95xx.c driver, used in the Raspberry PI for the Ethernet card. I imagine in my case what's doing wrong could be terrible but I am not abe to tell right now. I tried thinking how to avoid allocating the temporary NDP somehow: without a solution. And nowI would like to understand what's going wrong even for my personal kwoledge. > Regards > Oliver > > > Thank you again, and have a good weekend. Enrico -- 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
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. + */ +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; + + /* Allocate a new NDP */ + ndp16 = kzalloc(ctx->max_ndp_size, GFP_NOIO); + if (!ndp16) + return ret; + + /* Prepare a new NDP to add data on subsequent calls. */ + drvstate->ndp = memset(ndp16, 0, ctx->max_ndp_size); + + /* Initial NDP length */ + ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16)); + + /* Frame signature: to be reworked in general. */ + ndp16->dwSignature = cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN); + + ret = 0; + break; + + case NCM_UPDATE_NDP: + + if (drvstate->skb_ndp) { + ndp16 = drvstate->skb_ndp; + } else { + ndp16 = drvstate->ndp; + + /* Do we have enough space for the data? */ + if (skb_out->len + ctx->max_ndp_size > ctx->tx_max) + goto error; + } + + /* Calculate frame number within this NDP */ + ndplen = le16_to_cpu(ndp16->wLength); + index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1; + + if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) + goto error; + + /* tx_max shouldn't be exceeded after committing. */ + if (ndplen + sizeof(struct usb_cdc_ncm_dpe16) > ctx->tx_max) + goto error; + + /* Adding a DPT entry. */ + ndp16->dpe16[index].wDatagramLength = cpu_to_le16(drvstate->data_len); + ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(drvstate->data_start); + ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16)); + + ret = 0; + break; + + case NCM_COMMIT_NDP: + + if (drvstate->skb_ndp) + goto error; /* Call this only once please. */ + + ndp16 = drvstate->ndp; + + nth16->wNdpIndex = cpu_to_le16(skb_out->len); + + /* "commit" NDP */ + drvstate->skb_ndp = memcpy(skb_put(skb_out, ctx->max_ndp_size), ndp16, ctx->max_ndp_size); + + kfree(ndp16); + ndp16 = NULL; + drvstate->ndp = NULL; + + case NCM_FINALIZE_NTH: + + /* Finalize NTH16 header, setting it's block length */ + nth16->wBlockLength = cpu_to_le16(skb_out->len); + + ret = 0; + break; + default: + break; + } + +error: + if (ret) + kfree(drvstate->ndp); + return ret; +} + +/* XXX rewrite, not multipacket */ +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); + + status = huawei_ncm_mgmt(dev, drvstate, skb_out, NCM_INIT_FRAME); + + cdc_ncm_align_tail(skb_out, ctx->tx_ndp_modulus, 0, ctx->tx_max); + status = huawei_ncm_mgmt(dev, drvstate, skb_out, NCM_COMMIT_NDP); + + cdc_ncm_align_tail(skb_out, ctx->tx_modulus, ctx->tx_remainder, ctx->tx_max); + drvstate->data_start = skb_out->len; + memcpy(skb_put(skb_out, skb_in->len), skb_in->data, skb_in->len); + drvstate->data_len = skb_out->len - drvstate->data_start; + + status = huawei_ncm_mgmt(dev, drvstate, skb_out, NCM_UPDATE_NDP); + + status = huawei_ncm_mgmt(dev, drvstate, skb_out, NCM_FINALIZE_NTH); + + kfree_skb(skb_in); + return skb_out; +} + static int huawei_cdc_ncm_wdm_manage_power(struct usb_interface *intf, int status) { @@ -175,12 +357,13 @@ err: static const struct driver_info huawei_cdc_ncm_info = { .description = "Huawei CDC NCM device", - .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN, + /* .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN, */ + .flags = FLAG_NO_SETINT | FLAG_WWAN, .bind = huawei_cdc_ncm_bind, .unbind = huawei_cdc_ncm_unbind, .manage_power = huawei_cdc_ncm_manage_power, .rx_fixup = cdc_ncm_rx_fixup, - .tx_fixup = cdc_ncm_tx_fixup, + .tx_fixup = huawei_ncm_tx_fixup, }; static const struct usb_device_id huawei_cdc_ncm_devs[] = {
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.rs@gmail.com> --- drivers/net/usb/huawei_cdc_ncm.c | 187 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 2 deletions(-)