* [PATCH RFC 0/2] huawei_cdc_ncm: new NCM TX stack for Huawei-style NCM
@ 2015-06-22 22:32 Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 1/2 cdc_ncm: export cdc_ncm_align_tail Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack Enrico Mioso
0 siblings, 2 replies; 11+ messages in thread
From: Enrico Mioso @ 2015-06-22 22:32 UTC (permalink / raw)
To: linux-usb, netdev; +Cc: Oliver Neukum, Enrico Mioso
This is an ALFA version of my new NCM TX engine, built with huawei-style NCM and flexibility in mind.
The main goal is to allow foreasily re-ordering NCM frame content.
Basic communication works - but an usbnet EVENT_RX_MEMORY will stop the process after some packets.
Any idea or comment about any aspect of this code is really apreciated -
including suggestions for style and over-long lines problematic to break down,
at least for me.
Lots of lines longer than 80 chars seemed not easily breakable to me: any
suggestion is welcome.
Enrico Mioso (2):
cdc_ncm: export cdc_ncm_align_tail
huawei_cdc_ncm: introduce new TX ncm stack
drivers/net/usb/cdc_ncm.c | 3 +-
drivers/net/usb/huawei_cdc_ncm.c | 187 ++++++++++++++++++++++++++++++++++++++-
include/linux/usb/cdc_ncm.h | 1 +
3 files changed, 188 insertions(+), 3 deletions(-)
--
2.4.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC] 1/2 cdc_ncm: export cdc_ncm_align_tail
2015-06-22 22:32 [PATCH RFC 0/2] huawei_cdc_ncm: new NCM TX stack for Huawei-style NCM Enrico Mioso
@ 2015-06-22 22:32 ` Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack Enrico Mioso
1 sibling, 0 replies; 11+ messages in thread
From: Enrico Mioso @ 2015-06-22 22:32 UTC (permalink / raw)
To: linux-usb, netdev; +Cc: Oliver Neukum, Enrico Mioso
Export this function to allow for reusing it in other drivers needing NCM
frames alignment, such as the huawei_cdc_ncm one.
Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
---
drivers/net/usb/cdc_ncm.c | 3 ++-
include/linux/usb/cdc_ncm.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..1a8b619 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -967,7 +967,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
return ret;
}
-static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max)
+void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max)
{
size_t align = ALIGN(skb->len, modulus) - skb->len + remainder;
@@ -976,6 +976,7 @@ static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remai
if (align && skb_tailroom(skb) >= align)
memset(skb_put(skb, align), 0, align);
}
+EXPORT_SYMBOL_GPL(cdc_ncm_align_tail);
/* return a pointer to a valid struct usb_cdc_ncm_ndp16 of type sign, possibly
* allocating a new one within skb
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 7c9b484..8fe7ef0 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -141,5 +141,6 @@ int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset);
struct sk_buff *
cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags);
int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in);
+void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max);
#endif /* __LINUX_USB_CDC_NCM_H */
--
2.4.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
2015-06-22 22:32 [PATCH RFC 0/2] huawei_cdc_ncm: new NCM TX stack for Huawei-style NCM Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 1/2 cdc_ncm: export cdc_ncm_align_tail Enrico Mioso
@ 2015-06-22 22:32 ` Enrico Mioso
[not found] ` <1435012335-6055-3-git-send-email-mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
1 sibling, 1 reply; 11+ messages in thread
From: Enrico Mioso @ 2015-06-22 22:32 UTC (permalink / raw)
To: linux-usb, netdev; +Cc: Oliver Neukum, Enrico Mioso
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.
+ */
+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[] = {
--
2.4.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
[not found] ` <1435012335-6055-3-git-send-email-mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2015-06-25 9:49 ` Oliver Neukum
2015-06-25 11:44 ` Enrico Mioso
2015-06-25 9:55 ` Oliver Neukum
1 sibling, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2015-06-25 9:49 UTC (permalink / raw)
To: Enrico Mioso
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> 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 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] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
[not found] ` <1435012335-6055-3-git-send-email-mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-06-25 9:49 ` Oliver Neukum
@ 2015-06-25 9:55 ` Oliver Neukum
1 sibling, 0 replies; 11+ messages in thread
From: Oliver Neukum @ 2015-06-25 9:55 UTC (permalink / raw)
To: Enrico Mioso
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
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 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] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
2015-06-25 9:49 ` Oliver Neukum
@ 2015-06-25 11:44 ` Enrico Mioso
[not found] ` <alpine.LNX.2.20.1506251327570.25021-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Enrico Mioso @ 2015-06-25 11:44 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-usb, netdev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
[not found] ` <alpine.LNX.2.20.1506251327570.25021-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2015-06-25 13:38 ` Oliver Neukum
2015-06-25 15:19 ` Enrico Mioso
0 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2015-06-25 13:38 UTC (permalink / raw)
To: Enrico Mioso
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
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 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] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
2015-06-25 13:38 ` Oliver Neukum
@ 2015-06-25 15:19 ` Enrico Mioso
[not found] ` <alpine.LNX.2.20.1506251543240.16582-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Enrico Mioso @ 2015-06-25 15:19 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-usb, netdev
[-- Attachment #1: Type: text/plain, Size: 5430 bytes --]
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
[not found] ` <alpine.LNX.2.20.1506251543240.16582-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2015-06-26 8:14 ` Oliver Neukum
2015-06-27 5:40 ` Enrico Mioso
[not found] ` <1435306442.2914.8.camel-IBi9RG/b67k@public.gmane.org>
0 siblings, 2 replies; 11+ messages in thread
From: Oliver Neukum @ 2015-06-26 8:14 UTC (permalink / raw)
To: Enrico Mioso
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
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-aHS423dKhWw@public.gmane.org
>
> On Thu, 25 Jun 2015, Oliver Neukum wrote:
>
> > Date: Thu, 25 Jun 2015 15:38:46
> > From: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
> > To: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 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] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
2015-06-26 8:14 ` Oliver Neukum
@ 2015-06-27 5:40 ` Enrico Mioso
[not found] ` <1435306442.2914.8.camel-IBi9RG/b67k@public.gmane.org>
1 sibling, 0 replies; 11+ messages in thread
From: Enrico Mioso @ 2015-06-27 5:40 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-usb, netdev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
[not found] ` <1435306442.2914.8.camel-IBi9RG/b67k@public.gmane.org>
@ 2015-06-30 7:45 ` Enrico Mioso
0 siblings, 0 replies; 11+ messages in thread
From: Enrico Mioso @ 2015-06-30 7:45 UTC (permalink / raw)
To: Oliver Neukum
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
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 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] 11+ messages in thread
end of thread, other threads:[~2015-06-30 7:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-22 22:32 [PATCH RFC 0/2] huawei_cdc_ncm: new NCM TX stack for Huawei-style NCM Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 1/2 cdc_ncm: export cdc_ncm_align_tail Enrico Mioso
2015-06-22 22:32 ` [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack Enrico Mioso
[not found] ` <1435012335-6055-3-git-send-email-mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-06-25 9:49 ` Oliver Neukum
2015-06-25 11:44 ` Enrico Mioso
[not found] ` <alpine.LNX.2.20.1506251327570.25021-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2015-06-25 13:38 ` Oliver Neukum
2015-06-25 15:19 ` Enrico Mioso
[not found] ` <alpine.LNX.2.20.1506251543240.16582-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2015-06-26 8:14 ` Oliver Neukum
2015-06-27 5:40 ` Enrico Mioso
[not found] ` <1435306442.2914.8.camel-IBi9RG/b67k@public.gmane.org>
2015-06-30 7:45 ` Enrico Mioso
2015-06-25 9:55 ` Oliver Neukum
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.