Compiler warnings in kernel 4.14.51
Hello! While compiling kernel 4.14.51 I got the following warnings: CC net/core/dev.o net/core/dev.c: In function 'validate_xmit_skb_list': net/core/dev.c:3121:15: warning: 'tail' may be used uninitialized in this function [-Wmaybe-uninitialized] ... CC net/ipv4/fib_trie.o net/ipv4/fib_trie.c: In function 'fib_trie_unmerge': net/ipv4/fib_trie.c:1749:8: warning: 'local_tp' may be used uninitialized in this function [-Wmaybe-uninitialized] The kernel has been compiled for the MIPS architecture, little-endian 32-bit. My /prov/version file content is: Linux version 4.14.51 (mrkiko@mStation) (gcc version 7.3.0 (OpenWrt GCC 7.3.0 r7360-e15565a01c)) #0 Fri Jun 29 05:54:17 2018 thank you very much to all of you for your great work. Enrico
Re: [PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix
Thank you very much Ben and bjorn, for the spot, the fix, and overall helping
me learn new things.
Acked-By: Enrico Mioso <mrkiko...@gmail.com>
On Wed, 15 Nov 2017, Bjørn Mork wrote:
Date: Wed, 15 Nov 2017 09:35:02
From: Bjørn Mork <bj...@mork.no>
To: netdev@vger.kernel.org
Cc: Oliver Neukum <oli...@neukum.org>,
Ben Hutchings <ben.hutchi...@codethink.co.uk>, linux-...@vger.kernel.org,
Bjørn Mork <bj...@mork.no>, Enrico Mioso <mrkiko...@gmail.com>,
Christian Panton <christ...@panton.org>
Subject: [PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix
The GetNtbFormat and SetNtbFormat requests operate on 16 bit little
endian values. We get away with ignoring this most of the time, because
we only care about USB_CDC_NCM_NTB16_FORMAT which is 0x. This
fails for USB_CDC_NCM_NTB32_FORMAT.
Fix comparison between LE value from device and constant by converting
the constant to LE.
Reported-by: Ben Hutchings <ben.hutchi...@codethink.co.uk>
Fixes: 2b02c20ce0c2 ("cdc_ncm: Set NTB format again after altsetting switch for
Huawei devices")
Cc: Enrico Mioso <mrkiko...@gmail.com>
Cc: Christian Panton <christ...@panton.org>
Signed-off-by: Bjørn Mork <bj...@mork.no>
---
drivers/net/usb/cdc_ncm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 47cab1bde065..9e1b74590682 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -771,7 +771,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
int err;
u8 iface_no;
struct usb_cdc_parsed_header hdr;
- u16 curr_ntb_format;
+ __le16 curr_ntb_format;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -889,7 +889,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
goto error2;
}
- if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
+ if (curr_ntb_format == cpu_to_le16(USB_CDC_NCM_NTB32_FORMAT)) {
dev_info(>dev, "resetting NTB format to 16-bit");
err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
USB_TYPE_CLASS | USB_DIR_OUT
--
2.11.0
[PATCH V3] Set NTB format again after altsetting switch for Huawei devices
Some firmwares in Huawei E3372H devices have been observed to switch back
to NTB 32-bit format after altsetting switch.
This patch implements a driver flag to check for the device settings and
set NTB format to 16-bit again if needed.
The flag has been activated for devices controlled by the huawei_cdc_ncm.c
driver.
V1->V2:
- fixed broken error checks
- some corrections to the commit message
V2->V3:
- variable name changes, to clarify what's happening
- check (and possibly set) the NTB format later in the common bind code path
Signed-off-by: Enrico Mioso <mrkiko...@gmail.com>
Reported-and-tested-by: Christian Panton <christ...@panton.org>
Reviewed-by: Bjørn Mork <bj...@mork.no>
CC: Bjørn Mork <bj...@mork.no>
CC: Christian Panton <christ...@panton.org>
CC: linux-...@vger.kernel.org
CC: netdev@vger.kernel.org
CC: Oliver Neukum <oli...@neukum.org>
---
drivers/net/usb/cdc_ncm.c| 28
drivers/net/usb/huawei_cdc_ncm.c | 6 ++
include/linux/usb/cdc_ncm.h | 1 +
3 files changed, 35 insertions(+)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index d103a1d4fb36..8f572b9f3625 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -768,8 +768,10 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
u8 *buf;
int len;
int temp;
+ int err;
u8 iface_no;
struct usb_cdc_parsed_header hdr;
+ u16 curr_ntb_format;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -874,6 +876,32 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
goto error2;
}
+ /*
+* Some Huawei devices have been observed to come out of reset in NDP32
mode.
+* Let's check if this is the case, and set the device to NDP16 mode
again if
+* needed.
+ */
+ if (ctx->drvflags & CDC_NCM_FLAG_RESET_NTB16) {
+ err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_FORMAT,
+ USB_TYPE_CLASS | USB_DIR_IN |
USB_RECIP_INTERFACE,
+ 0, iface_no, _ntb_format, 2);
+ if (err < 0) {
+ goto error2;
+ }
+
+ if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
+ dev_info(>dev, "resetting NTB format to 16-bit");
+ err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
+ USB_TYPE_CLASS | USB_DIR_OUT
+ | USB_RECIP_INTERFACE,
+ USB_CDC_NCM_NTB16_FORMAT,
+ iface_no, NULL, 0);
+
+ if (err < 0)
+ goto error2;
+ }
+ }
+
cdc_ncm_find_endpoints(dev, ctx->data);
cdc_ncm_find_endpoints(dev, ctx->control);
if (!dev->in || !dev->out || !dev->status) {
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index 2680a65cd5e4..63f28908afda 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -80,6 +80,12 @@ static int huawei_cdc_ncm_bind(struct usbnet *usbnet_dev,
* be at the end of the frame.
*/
drvflags |= CDC_NCM_FLAG_NDP_TO_END;
+
+ /* Additionally, it has been reported that some Huawei E3372H devices,
with
+* firmware version 21.318.01.00.541, come out of reset in NTB32 format
mode, hence
+* needing to be set to the NTB16 one again.
+*/
+ drvflags |= CDC_NCM_FLAG_RESET_NTB16;
ret = cdc_ncm_bind_common(usbnet_dev, intf, 1, drvflags);
if (ret)
goto err;
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 021f7a88f52c..1a59699cf82a 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -83,6 +83,7 @@
/* Driver flags */
#define CDC_NCM_FLAG_NDP_TO_END0x02/* NDP is
placed at end of frame */
#define CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE 0x04/* Avoid altsetting
toggle during init */
+#define CDC_NCM_FLAG_RESET_NTB16 0x08 /* set NDP16 one more time after
altsetting switch */
#define cdc_ncm_comm_intf_is_mbim(x) ((x)->desc.bInterfaceSubClass ==
USB_CDC_SUBCLASS_MBIM && \
(x)->desc.bInterfaceProtocol ==
USB_CDC_PROTO_NONE)
--
2.13.2
cdc_ncm driver padding problem (WAS: Question about CDC_NCM_FLAG_NDP_TO_END)
Hello guys. Some very good people managed to detect there is a problem with some Huawei firmwares and NCM padding. I actually don't think I have the hardware to test btw. On Wed, 14 Sep 2016, Marek Brudka wrote: Sorry Marek - I forwarded this message without asking for your consent. Let me know anyway if this is a problem. thank you all guys for everything, Enrico ==Date: Wed, 14 Sep 2016 19:31:50 ==From: Marek Brudka <mbru...@gmail.com> ==To: Enrico Mioso <mrkiko...@gmail.com> ==Subject: Re: Question about CDC_NCM_FLAG_NDP_TO_END == ==Hello Enrico, == ==As nobody at openwrt forum replied to my request on the way how to get ==the exact ==recompilation of OpenWrt 15.05.1 I decided to switch to the developement ==version ==(12/09/2016), which already contains your patch. == ==The nice thing is that I got my modem (E3372 HiLink reflashed to E398) ==working ==in ncm mode! == ==The bad thing is DHCP. It seems, that cdc_ncm driver somehow consumes DHCP ==replies. I had to manually setup wwan0 interface as well as routing ==using the result ==of Hayes command == ==AT^DHCP? ==^DHCP: ==EC684764,F8FF,E9684764,E9684764,356002D4,366002D4,4320,4320 ==OK == ==Certainly, I will modify connect scripts == ==https://github.com/zabbius/smarthome/tree/master/openwrt/huawei-ncm/files/usr/sbin ==for me to parse this response. However it seems, that the problem is on ==driver level and ==is related with padding. Do you know this issue which is nicely ==described in the thread ==https://forum.openwrt.org/viewtopic.php?pid=273099 ==of OpenWrt forum? == ==Thank you ==Marek Brudka == == ==W dniu 11.09.2016 o 15:19, Enrico Mioso pisze: ==> Hello Marek. ==> ==> First of all, thank you for your interest in this driver, and for writing. ==> ==> Unfortunately, I don't know the exact procedure to do that: you might be confortable putting those patches in generic-patches-kernel_version if I am not wrong, but I may well be wrong or imprecise, and recompile the whole Openwrt thing? ==> don't know. But yes, that message should appear in the dmesg. ==> NDPs need to be at end of NCM frames. Oh, I don't remember well what NDP stands for... ufh. Sorry. ==> ==> Anyway, let me know if I can do something for you. ==> Enrico ==> == == ==
Re: [PATCH net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372
Hello guys. I am sorry the way my feature is actually implemented is limiting MUX performances in mbim case, even with good devices. When I developed it, I also probably tought this wasn't affecting MBIM devices. Thank you Bjorn, great work in handling it. Thank you for all of you; Thank you for reporting this Sami. Acked-By: Enrico Mioso <mrkiko...@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cdc_ncm: update specs URL
Update referenced specs link to reflect actual file version and location. Signed-off-by: Enrico Mioso mrkiko...@gmail.com --- drivers/net/usb/cdc_ncm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 1991e4a..db40175 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -6,7 +6,7 @@ * Original author: Hans Petter Selasky hans.petter.sela...@stericsson.com * * USB Host Driver for Network Control Model (NCM) - * http://www.usb.org/developers/devclass_docs/NCM10.zip + * http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip * * The NCM encoding, decoding and initialization logic * derives from FreeBSD 8.x. if_cdce.c and if_cdcereg.h -- 2.4.5 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
Hi Oliver and everybody reading this message. So V3 of this patch fixed the issue I reported recently. I wasn't properly accounting for the NDP size when writing new packets to the SKB: so i ended up writing past the end of SKB buffer. Thank you for your patience and help. Enrico -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3] cdc_ncm: Add support for moving NDP to end of NCM frame
NCM specs are not actually mandating a specific position in the frame for
the NDP (Network Datagram Pointer). However, some Huawei devices will
ignore our aggregates if it is not placed after the datagrams it points
to. Add support for doing just this, in a per-device configurable way.
While at it, update NCM subdrivers, disabling this functionality in all of
them, except in huawei_cdc_ncm where it is enabled instead.
We aren't making any distinction between different Huawei NCM devices,
based on what the vendor driver does. Standard NCM devices are left
unaffected: if they are compliant, they should be always usable, still
stay on the safe side.
This change has been tested and working with a Huawei E3131 device (which
works regardless of NDP position), a Huawei E3531 (also working both
ways) and an E3372 (which mandates NDP to be after indexed datagrams).
V1-V2:
- corrected wrong NDP acronym definition
- fixed possible NULL pointer dereference
- patch cleanup
V2-V3:
- Properly account for the NDP size when writing new packets to SKB
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_mbim.c | 2 +-
drivers/net/usb/cdc_ncm.c| 61
drivers/net/usb/huawei_cdc_ncm.c | 7 +++--
include/linux/usb/cdc_ncm.h | 7 -
4 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index e4b7a47..efc18e0 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct
usb_interface *intf)
if (!cdc_ncm_comm_intf_is_mbim(intf-cur_altsetting))
goto err;
- ret = cdc_ncm_bind_common(dev, intf, data_altsetting);
+ ret = cdc_ncm_bind_common(dev, intf, data_altsetting, 0);
if (ret)
goto err;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..1991e4a 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -684,10 +684,12 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
ctx-tx_curr_skb = NULL;
}
+ kfree(ctx-delayed_ndp16);
+
kfree(ctx);
}
-int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting)
+int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting, int drvflags)
{
const struct usb_cdc_union_desc *union_desc = NULL;
struct cdc_ncm_ctx *ctx;
@@ -855,6 +857,17 @@ advance:
/* finish setting up the device specific data */
cdc_ncm_setup(dev);
+ /* Device-specific flags */
+ ctx-drvflags = drvflags;
+
+ /* Allocate the delayed NDP if needed. */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END) {
+ ctx-delayed_ndp16 = kzalloc(ctx-max_ndp_size, GFP_KERNEL);
+ if (!ctx-delayed_ndp16)
+ goto error2;
+ dev_info(intf-dev, NDP will be placed at end of frame for
this device.);
+ }
+
/* override ethtool_ops */
dev-net-ethtool_ops = cdc_ncm_ethtool_ops;
@@ -954,8 +967,11 @@ static int cdc_ncm_bind(struct usbnet *dev, struct
usb_interface *intf)
if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM)
return -ENODEV;
- /* The NCM data altsetting is fixed */
- ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM);
+ /* The NCM data altsetting is fixed, so we hard-coded it.
+* Additionally, generic NCM devices are assumed to accept arbitrarily
+* placed NDP.
+*/
+ ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
/*
* We should get an event when network connection is connected or
@@ -986,6 +1002,14 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
size_t ndpoffset = le16_to_cpu(nth16-wNdpIndex);
+ /* If NDP should be moved to the end of the NCM package, we can't
follow the
+ * NTH16 header as we would normally do. NDP isn't written to the SKB
yet, and
+ * the wNdpIndex field in the header is actually not consistent with
reality. It will be later.
+ */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END)
+ if (ctx-delayed_ndp16-dwSignature == sign)
+ return ctx-delayed_ndp16;
+
/* follow the chain of NDPs, looking for a match */
while (ndpoffset) {
ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data + ndpoffset);
@@ -995,7 +1019,8 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
}
/* align new NDP */
- cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
+ if (!(ctx-drvflags CDC_NCM_FLAG_NDP_TO_END))
+ cdc_ncm_align_tail(skb, ctx
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
So, here is what I tried so far: 1 - Check if the pointer gets corrupted somehow (address change): it seems this doesn't happen at all. 2 - Size problems: I tried setting higher values of the size just in case, with absolutely no changei n behaviour. The code that assigns the pointer the address returned by kzalloc is after all the other function invocations in _bind(), so I don't know where to look exactly. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
Hi Oliver, hello to who is reading this message. i was re-reading the code and the oops, without understanding what's the problem. Still: what impressed me is the fact that at some point you see NULL ptr dereference in unrelated code (fbcon). Is it possible that at some point the memory portion (172 bytes if device is affected by NCM errata, and mine is), that the portion of memory to which ctx-delayed_ndp16 points to is somehow moved / thrown away? It doesn't make sense, because otherwise even accesses to the ctx variable would give problems. And they don't. Looking around then, I see kzalloc() / kmalloc (kzalloc =kmalloc | __GFP_ZERO) are used to allocate any size of memory (with the only requirement for it to be small). In rndis_host.c 1025 bytes (not 1024) are allocated, so I am excluding any kind of alignment problem here. Thank you, Enrico -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
Just to be clear - this happens on the real machine as well, but here the trace is difficult to extract, because even with the help of someone seeing the screen, I noticed the screen doesn't get updated. I am using vesa right now. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
Sure Oliver! Here it is. And - I tried with various approach. I tired also kzallocating the needed memory inside the tx_fixup function using the GFP_ATOMIC flag due to the fact I am in an interrupt handler. At some point, the problem started manifesting in a memset call that whasn't in my patch, DOH. Tell me if I can do something and I'll try. No crashdump possible it seems, after this crash the system isn't able to kexec. Enrico Mioso Trace: from a 32-bit QEMU VM launched with parameters: qemu-system-i386 -drive file=dsksys.img,index=0,media=disk -boot d -m 512 -soundhw hda -cdrom torrent_ctl/archlinux-2015.06.01-dual.iso -usb -usbdevice host:12d1:1506 -redir tcp:2200::22 -machine accel=kvm,kernel_irqchip=on -serial stdio -display none -cpu host -watchdog i6300esb $@ Host is also a 32-bit system. All goes well until I start rtorrent so that it emits DHT traffic (udp, small packets, lots of them I think). [ 617.581100] EXT4-fs (sda): re-mounted. Opts: nobarrier,noauto_da_alloc [ 656.964399] BUG: unable to handle kernel paging request at d1402000 [ 656.966824] IP: [c12596f0] memset+0x10/0x20 [ 656.966824] *pde = 1e7c1067 *pte = 11402161 [ 656.966824] Oops: 0003 [#1] PREEMPT SMP [ 656.966824] Modules linked in: huawei_cdc_ncm cdc_ncm mousedev snd_hda_codec_generic ppdev bochs_drm ttm snd_hda_intel cfg80211 drm_kms_helper rfkill snd_hda_controller snd_hda_codec psmouse pcspkr serio_raw snd_hwdep drm snd_pcm option snd_timer usb_wwan syscopyarea usbserial snd sysfillrect sysimgblt soundcore i2c_piix4 i6300esb i2c_core parport_pc parport acpi_cpufreq e vdev processor mac_hid sch_fq_codel nfs lockd grace sunrpc fscache ext4 crc16 mbcache jbd2 dm_snapshot dm_bufio dm_mod squashfs loop uas cdc_wdm isofs usbnet mii usb_storage sr_mod cdrom sd_mod ata_generic pata_acpi atkbd libps2 ata_piix uhci_hcd ehci_hcd libata intel_agp intel_gtt usbcore e1000 scsi_mod usb_common agpgart floppy i8042 serio button [last unloaded: cdc_ncm] [ 656.966824] CPU: 0 PID: 1664 Comm: main Tainted: GF 4.0.4-2-ARCH #1 [ 656.966824] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150617_082717-anatol 04/01/2014 [ 656.966824] task: dd48c660 ti: d1722000 task.ti: d1722000 [ 656.966824] EIP: 0060:[c12596f0] EFLAGS: 00210246 CPU: 0 [ 656.966824] EIP is at memset+0x10/0x20 [ 656.966824] EAX: EBX: ced5b058 ECX: fd959000 EDX: [ 656.966824] ESI: dd216c00 EDI: d1402000 EBP: d1723aa8 ESP: d1723aa0 [ 656.966824] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [ 656.966824] CR0: 80050033 CR2: d1402000 CR3: 1173 CR4: 07c0 [ 656.966824] Stack: [ 656.966824] 0025 ffa8 d1723ae8 e0dff758 1000 ced6ad40 dea13500 0002 [ 656.966824] 006a 0004 0002 ced5a000 002500ff dd2bbd80 00ac dd216c94 [ 656.966824] dd2bbb40 ced6ad40 d1723afc e0dff9d4 dd2bbb40 e0dff9a0 ced6a800 d1723b48 [ 656.966824] Call Trace: [ 656.966824] [e0dff758] cdc_ncm_fill_tx_frame+0x4c8/0x690 [cdc_ncm] [ 656.966824] [e0dff9d4] cdc_ncm_tx_fixup+0x34/0x70 [cdc_ncm] [ 656.966824] [e0dff9a0] ? cdc_ncm_bind+0x80/0x80 [cdc_ncm] [ 656.966824] [e08f3a50] usbnet_start_xmit+0x60/0x7c0 [usbnet] [ 656.966824] [c13bce5b] ? netif_skb_features+0xcb/0x440 [ 656.966824] [c13ab87a] ? __alloc_skb+0x6a/0x1e0 [ 656.966824] [c13bd6b4] dev_hard_start_xmit+0x224/0x3b0 [ 656.966824] [c13bd1e5] ? validate_xmit_skb.isra.33.part.34+0x15/0x2c0 [ 656.966824] [c13da960] sch_direct_xmit+0x100/0x1f0 [ 656.966824] [c13bda12] __dev_queue_xmit+0x1d2/0x500 [ 656.966824] [c13d99b0] ? ether_setup+0x80/0x80 [ 656.966824] [c13bdd4f] dev_queue_xmit+0xf/0x20 [ 656.966824] [c13c744f] neigh_resolve_output+0xff/0x200 [ 656.966824] [c13f321a] ip_finish_output+0x2ba/0x980 [ 656.966824] [c13f5754] ? __ip_make_skb+0x2a4/0x3b0 [ 656.966824] [c13f4ec7] ip_output+0x87/0xd0 [ 656.966824] [c13f460c] ? __ip_local_out+0x2c/0x80 [ 656.966824] [c13f5a19] ? ip_make_skb+0xd9/0x100 [ 656.966824] [c13f4687] ip_local_out_sk+0x27/0x30 [ 656.966824] [c13f5874] ip_send_skb+0x14/0x80 [ 656.966824] [c141b0f1] udp_send_skb+0x101/0x260 [ 656.966824] [c141c656] udp_sendmsg+0x2e6/0x900 [ 656.966824] [c13f3a80] ? ip_reply_glue_bits+0x80/0x80 [ 656.966824] [c107f1c7] ? update_cfs_rq_blocked_load+0x157/0x1a0 [ 656.966824] [c1427525] inet_sendmsg+0x75/0xa0 [ 656.966824] [c13a213f] do_sock_sendmsg+0x4f/0x80 [ 656.966824] [c13a409f] SyS_sendto+0x18f/0x1d0 [ 656.966824] [c13a1feb] ? sock_poll+0xeb/0x100 [ 656.966824] [c11c5a40] ? ep_read_events_proc+0xb0/0xb0 [ 656.966824] [c11c5adf] ? ep_send_events_proc+0x9f/0x1b0 [ 656.966824] [c13a4c4c] SyS_socketcall+0x19c/0x300 [ 656.966824] [c14a0c97] sysenter_do_call+0x12/0x12 [ 656.966824] Code: 8a 0e 88 0f 8d b4 26 00 00 00 00 8b 45 f0 83 c4 04 5b 5e 5f 5d c3 90 8d 74 26 00 55 89 e5 57 53 3e 8d 74 26 00 89 c3 89 c7 89 d0 f3 aa 89 d8 5b 5f 5d c3 90 90 90 90 90 90 90 90 55 89
Re: [PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
When sending lots of small packets, this patch will generate an Unable to handle kernel paging request in the memset call: ndp16 = memset(ctx-delayed_ndp16, 0, ctx-max_ndp_size); And I don't know why. Any comment or suggestion would be greatly apreciated. This has been reproduced in a QEMU X86 VM, from kernel 4.0.4 to current git. Thanks, Enrico Mioso -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] cdc_ncm: Add support for moving NDP to end of NCM frame
NCM specs are not actually mandating a specific position in the frame for
the NDP (Network Datagram Pointer). However, some Huawei devices will
ignore our aggregates if it is not placed after the datagrams it points
to. Add support for doing just this, in a per-device configurable way.
While at it, update NCM subdrivers, disabling this functionality in all of
them, except in huawei_cdc_ncm where it is enabled instead.
We aren't making any distinction between different Huawei NCM devices,
based on what the vendor driver does. Standard NCM devices are left
unaffected: if they are compliant, they should be always usable, still
stay on the safe side.
This change has been tested and working with a Huawei E3131 device (which
works regardless of NDP position) and an E3372 device (which mandates NDP
to be after indexed datagrams).
V1-V2:
- corrected wrong NDP acronym definition
- fixed possible NULL pointer dereference
- patch cleanup
- rewrote description and commit subject to be more clear
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_mbim.c | 2 +-
drivers/net/usb/cdc_ncm.c| 50
drivers/net/usb/huawei_cdc_ncm.c | 7 --
include/linux/usb/cdc_ncm.h | 7 +-
4 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index e4b7a47..efc18e0 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct
usb_interface *intf)
if (!cdc_ncm_comm_intf_is_mbim(intf-cur_altsetting))
goto err;
- ret = cdc_ncm_bind_common(dev, intf, data_altsetting);
+ ret = cdc_ncm_bind_common(dev, intf, data_altsetting, 0);
if (ret)
goto err;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..4a27673 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -684,10 +684,12 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
ctx-tx_curr_skb = NULL;
}
+ kfree(ctx-delayed_ndp16);
+
kfree(ctx);
}
-int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting)
+int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting, int drvflags)
{
const struct usb_cdc_union_desc *union_desc = NULL;
struct cdc_ncm_ctx *ctx;
@@ -855,6 +857,17 @@ advance:
/* finish setting up the device specific data */
cdc_ncm_setup(dev);
+ /* Device-specific flags */
+ ctx-drvflags = drvflags;
+
+ /* Allocate the delayed NDP if needed. */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END) {
+ ctx-delayed_ndp16 = kzalloc(ctx-max_ndp_size, GFP_KERNEL);
+ if (!ctx-delayed_ndp16)
+ goto error2;
+ dev_info(intf-dev, NDP will be placed at end of frame for
this device.);
+ }
+
/* override ethtool_ops */
dev-net-ethtool_ops = cdc_ncm_ethtool_ops;
@@ -954,8 +967,11 @@ static int cdc_ncm_bind(struct usbnet *dev, struct
usb_interface *intf)
if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM)
return -ENODEV;
- /* The NCM data altsetting is fixed */
- ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM);
+ /* The NCM data altsetting is fixed, so we hard-coded it.
+* Additionally, generic NCM devices are assumed to accept arbitrarily
+* placed NDP.
+*/
+ ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
/*
* We should get an event when network connection is connected or
@@ -986,6 +1002,14 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
size_t ndpoffset = le16_to_cpu(nth16-wNdpIndex);
+ /* If NDP should be moved to the end of the NCM package, we can't
follow the
+ * NTH16 header as we would normally do. NDP isn't written to the SKB
yet, and
+ * the wNdpIndex field in the header is actually not consistent with
reality. It will be later.
+ */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END)
+ if (ctx-delayed_ndp16-dwSignature == sign)
+ return ctx-delayed_ndp16;
+
/* follow the chain of NDPs, looking for a match */
while (ndpoffset) {
ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data + ndpoffset);
@@ -995,7 +1019,8 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
}
/* align new NDP */
- cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
+ if (!(ctx-drvflags CDC_NCM_FLAG_NDP_TO_END))
+ cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
/* verify
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Ok, for now I should let go of this - might be it was a little bit too much for the time constaints I am having. But thank you for your effort Oliver. Anyway, trying to write this new TX engine helped me in understand better things and conceive the patch I sent you affecting cdc_ncm.c, which triggers some OOPSes but not in a qemu vm, so I can't read them actually. Any hint on my code is always apreciated, regardless. Enrico -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] cdc_ncm: add support for moving NDP to end of frames
This patch adds support for moving the NDP (Network Data Pointer) part of
an NCM frame to the end of it. This allows newer Huawei devices to work with
upstream Linux driver.
This functionality can be enabled by a cdc_ncm subdriver when binding to a
device. Hence this patch updates them.
This code works pretty well with an E3372 device I am testing it with.
The problem is - I observed my patch causing system failures when severe OOM
conditions are met. I am not sure the problem is cause by my code, but would
like to hear from you.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_mbim.c | 2 +-
drivers/net/usb/cdc_ncm.c| 51
drivers/net/usb/huawei_cdc_ncm.c | 7 --
include/linux/usb/cdc_ncm.h | 7 +-
4 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index e4b7a47..efc18e0 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct
usb_interface *intf)
if (!cdc_ncm_comm_intf_is_mbim(intf-cur_altsetting))
goto err;
- ret = cdc_ncm_bind_common(dev, intf, data_altsetting);
+ ret = cdc_ncm_bind_common(dev, intf, data_altsetting, 0);
if (ret)
goto err;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..60b147f 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -684,10 +684,12 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
ctx-tx_curr_skb = NULL;
}
+ kfree(ctx-delayed_ndp16);
+
kfree(ctx);
}
-int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting)
+int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting, int drvflags)
{
const struct usb_cdc_union_desc *union_desc = NULL;
struct cdc_ncm_ctx *ctx;
@@ -855,6 +857,17 @@ advance:
/* finish setting up the device specific data */
cdc_ncm_setup(dev);
+ /* Device-specific flags */
+ ctx-drvflags = drvflags;
+
+ /* Allocate the delayed NDP if needed. */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END) {
+ ctx-delayed_ndp16 = kzalloc(ctx-max_ndp_size, GFP_ATOMIC);
+ if (!ctx-delayed_ndp16)
+ goto error2;
+ dev_info(intf-dev, NDP will be placed at end of frame for
this device.);
+ }
+
/* override ethtool_ops */
dev-net-ethtool_ops = cdc_ncm_ethtool_ops;
@@ -954,8 +967,11 @@ static int cdc_ncm_bind(struct usbnet *dev, struct
usb_interface *intf)
if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM)
return -ENODEV;
- /* The NCM data altsetting is fixed */
- ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM);
+ /* The NCM data altsetting is fixed, so we hard-coded it.
+* Additionally, generic NCM devices are assumed to accept arbitrarily
+* placed NDP.
+*/
+ ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
/*
* We should get an event when network connection is connected or
@@ -986,6 +1002,14 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
size_t ndpoffset = le16_to_cpu(nth16-wNdpIndex);
+ /* If NDP should be moved to the end of the NCM package, we can't
follow the
+ * NTH16 header as we would normally do. NDP isn't written to the SKB
yet, and
+ * the wNdpIndex field in the header is actually not consistent with
reality. It will be later.
+ */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END)
+ if (ctx-delayed_ndp16-dwSignature == sign)
+ return ctx-delayed_ndp16;
+
/* follow the chain of NDPs, looking for a match */
while (ndpoffset) {
ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data + ndpoffset);
@@ -995,7 +1019,8 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
}
/* align new NDP */
- cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
+ if (!(ctx-drvflags CDC_NCM_FLAG_NDP_TO_END))
+ cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
/* verify that there is room for the NDP and the datagram (reserve) */
if ((ctx-tx_max - skb-len - reserve) ctx-max_ndp_size)
@@ -1008,7 +1033,12 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
nth16-wNdpIndex = cpu_to_le16(skb-len);
/* push a new empty NDP */
- ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
+ if (!(ctx-drvflags
Re: [RFC PATCH] cdc_ncm: add support for moving NDP to end of frames
Now the code is running all-day in a virtual machine, which unfortunately isn't
able to expose the problem.
And with my braille display I am not able to read the output of the panic, when
it happens, since execution of all the user-space processes isstopped somewhat
immediately.
The code seems to work pretty well here, and lot of data has been processed so
far.
On Sun, 28 Jun 2015, Enrico Mioso wrote:
Date: Sun, 28 Jun 2015 09:03:37
From: Enrico Mioso mrkiko...@gmail.com
To: Oliver Neukum oneu...@suse.com, linux-...@vger.kernel.org,
netdev@vger.kernel.org
Cc: Enrico Mioso mrkiko...@gmail.com
Subject: [RFC PATCH] cdc_ncm: add support for moving NDP to end of frames
This patch adds support for moving the NDP (Network Data Pointer) part of
an NCM frame to the end of it. This allows newer Huawei devices to work with
upstream Linux driver.
This functionality can be enabled by a cdc_ncm subdriver when binding to a
device. Hence this patch updates them.
This code works pretty well with an E3372 device I am testing it with.
The problem is - I observed my patch causing system failures when severe OOM
conditions are met. I am not sure the problem is cause by my code, but would
like to hear from you.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_mbim.c | 2 +-
drivers/net/usb/cdc_ncm.c| 51
drivers/net/usb/huawei_cdc_ncm.c | 7 --
include/linux/usb/cdc_ncm.h | 7 +-
4 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index e4b7a47..efc18e0 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct
usb_interface *intf)
if (!cdc_ncm_comm_intf_is_mbim(intf-cur_altsetting))
goto err;
- ret = cdc_ncm_bind_common(dev, intf, data_altsetting);
+ ret = cdc_ncm_bind_common(dev, intf, data_altsetting, 0);
if (ret)
goto err;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..60b147f 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -684,10 +684,12 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
ctx-tx_curr_skb = NULL;
}
+ kfree(ctx-delayed_ndp16);
+
kfree(ctx);
}
-int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting)
+int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8
data_altsetting, int drvflags)
{
const struct usb_cdc_union_desc *union_desc = NULL;
struct cdc_ncm_ctx *ctx;
@@ -855,6 +857,17 @@ advance:
/* finish setting up the device specific data */
cdc_ncm_setup(dev);
+ /* Device-specific flags */
+ ctx-drvflags = drvflags;
+
+ /* Allocate the delayed NDP if needed. */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END) {
+ ctx-delayed_ndp16 = kzalloc(ctx-max_ndp_size, GFP_ATOMIC);
+ if (!ctx-delayed_ndp16)
+ goto error2;
+ dev_info(intf-dev, NDP will be placed at end of frame for this
device.);
+ }
+
/* override ethtool_ops */
dev-net-ethtool_ops = cdc_ncm_ethtool_ops;
@@ -954,8 +967,11 @@ static int cdc_ncm_bind(struct usbnet *dev, struct
usb_interface *intf)
if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM)
return -ENODEV;
- /* The NCM data altsetting is fixed */
- ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM);
+ /* The NCM data altsetting is fixed, so we hard-coded it.
+* Additionally, generic NCM devices are assumed to accept arbitrarily
+* placed NDP.
+*/
+ ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
/*
* We should get an event when network connection is connected or
@@ -986,6 +1002,14 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
size_t ndpoffset = le16_to_cpu(nth16-wNdpIndex);
+ /* If NDP should be moved to the end of the NCM package, we can't
follow the
+ * NTH16 header as we would normally do. NDP isn't written to the SKB
yet, and
+ * the wNdpIndex field in the header is actually not consistent with
reality. It will be later.
+ */
+ if (ctx-drvflags CDC_NCM_FLAG_NDP_TO_END)
+ if (ctx-delayed_ndp16-dwSignature == sign)
+ return ctx-delayed_ndp16;
+
/* follow the chain of NDPs, looking for a match */
while (ndpoffset) {
ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data + ndpoffset);
@@ -995,7 +1019,8 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
}
/* align new NDP
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver!
I wish sincerely to thank you again for your time and patience.
On Fri, 26 Jun 2015, Oliver Neukum wrote:
Date: Fri, 26 Jun 2015 10:14:02
From: Oliver Neukum oneu...@suse.com
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.
You cannot become any more convoluted even if you try.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx-tx_rem_skb);
swap(sign, ctx-tx_rem_sign);
} else {
ready2send = 1;
}
The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.
I doubt you can can come up with anything more convoluted
but it simplifies locking.
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
Sounds like a plan.
But to prevent usbnet from submittinx RX'd packets, I should be doing something
nasty here. And unfortunately don't understand why.
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info-tx_fixup) {
skb = info-tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info-flags FLAG_MULTI_PACKET)
goto not_drop;
So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.
Unfortuantely I might have not explained myself better.
I should thank you for this illumination on the timer part: since I know I'll
need this.
But what's stopping me right now is actually the RX, not the TX part.
Sure, the RX function I am using comes from cdc_ncm.c and works fine:
unfortunately, usbnet will not call it.
Infact, my dmesg ends up FULL of
kevent 2 may have been dropped messages.
And looking around I discovered I end up triggering a memory check in usbnet.c,
at line 475
skb = __netdev_alloc_skb_ip_align(dev-net, size, flags
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver.
Thank you for your patience, and review. I apreciated it very much.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 11:49:29
From: Oliver Neukum oneu...@suse.com
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Tue, 2015-06-23 at 00:32 +0200, Enrico Mioso wrote:
This patch introduces a new NCM tx engine, able to operate in standard-
and huawei-style mode.
In the first case, the NDP is disposed after the initial headers and
before any datagram.
What works:
- is able to communicate with compliant NCM devices:
I tested this with a board running the Linux g_ncm gadget driver.
What doesn't work:
- After some packets I start gettint LOTS of EVENT_RX_MEMORY from usbnet,
which fails to allocate an RX SKB in rx_submit(). Don't understand why,
any suggestion would be very welcome.
The tx_fixup function given here, even if actually working, should be
considered as an example: the NCM manager is used here simulating the
cdc_ncm.c behaviour.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/huawei_cdc_ncm.c | 187 ++-
1 file changed, 185 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index 735f7da..217802a 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -29,6 +29,35 @@
#include linux/usb/cdc-wdm.h
#include linux/usb/cdc_ncm.h
+/* NCM management operations: */
+
+/* NCM_INIT_FRAME: prepare for a new frame.
+ * NTH16 header is written to output SKB, NDP data is reset and last
+ * committed NDP pointer set to NULL.
+ * Now, data may be added to this NCM package.
+ */
+#define NCM_INIT_FRAME 1
+
+/* NCM_UPDATE_NDP: adds data to an NDP structure, hence updating it.
+ * Some checks are performed to be sure data fits in, respecting device and
+ * spec constrains.
+ * Normally the NDP is kept in memory and committed to the SKB only when
+ * requested. However, calling this method after NCM_COMMIT_NDP, causes it to
+ * work directly on the already committed SKB copy. this allows for flexibility
+ * in frame ordering.
+ */
+#define NCM_UPDATE_NDP 2
+
+/* Write NDP: commits NDP to output SKB.
+ * This method should be called only once per frame.
+ */
+#define NCM_COMMIT_NDP 3
+
+/* Finalizes NTH16 header: to be called when working in
+ * update-already-committed mode.
+ */
+#define NCM_FINALIZE_NTH 5
+
/* Driver data */
struct huawei_cdc_ncm_state {
struct cdc_ncm_ctx *ctx;
@@ -36,6 +65,16 @@ struct huawei_cdc_ncm_state {
struct usb_driver *subdriver;
struct usb_interface *control;
struct usb_interface *data;
+
+ /* Keeps track of where data starts and ends in SKBs. */
+ int data_start;
+ int data_len;
+
+ /* Last committed NDP for post-commit operations. */
+ struct usb_cdc_ncm_ndp16 *skb_ndp;
+
+ /* Non-committed NDP */
+ struct usb_cdc_ncm_ndp16 *ndp;
};
static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
@@ -53,6 +92,149 @@ static int huawei_cdc_ncm_manage_power(struct usbnet
*usbnet_dev, int on)
return 0;
}
+/* huawei_ncm_mgmt: flexible TX NCM manager.
+ *
+ * Once a non-zero status value is rturned, current frame should be discarded
+ * and operations restarted from scratch.
+ */
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
OR
- we have enough data in the aggregate, and cannot add more.
This is something done in cdc_ncm.c for example.
But here I have a question: by reading the comment in file
drivers/net/usb/rndis_host.c at line 572, there seem to be different opinions
in this matter.
What to do ?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
And sure, the interface goes down case is important: didn't think about it.
Thanks for the point.
the only other additional goal is to use the manager in such a way that NDP
will be disposed after frames.
I think that this split between NCM management and tx_fixup makes things more
flexible in general: this is the reason for re-writing it.
This is something done in cdc_ncm.c for example.
But here I have a question: by reading the comment in file
drivers/net/usb/rndis_host.c at line 572, there seem to be different opinions
in this matter.
That is a very old comment written for much slower devices.
rndis_host doesn't get much love nowadays.
Fine.
What to do ?
+int
+huawei_ncm_mgmt(struct usbnet *dev,
+ struct huawei_cdc_ncm_state *drvstate, struct sk_buff *skb_out,
int mode) {
+ struct usb_cdc_ncm_nth16 *nth16 = (struct usb_cdc_ncm_nth16
*)skb_out-data;
+ struct cdc_ncm_ctx *ctx = drvstate-ctx;
+ struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+ int ret = -EINVAL;
+ u16 ndplen, index;
+
+ switch (mode) {
+ case NCM_INIT_FRAME:
+
+ /* Write a new NTH16 header */
+ nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+ if (!nth16) {
+ ret = -EINVAL;
+ goto error;
+ }
+
+ /* NTH16 signature and header length are known a-priori. */
+ nth16-dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+ nth16-wHeaderLength = cpu_to_le16(sizeof(struct
usb_cdc_ncm_nth16));
+
+ /* TX sequence numbering */
+ nth16-wSequence = cpu_to_le16(ctx-tx_seq++);
+
+ /* Forget about previous SKB NDP */
+ drvstate-skb_ndp = NULL;
This is probably better done after you know you cannot fail.
Sure. Thank you.
+
+ /* Allocate a new NDP */
+ ndp16 = kzalloc(ctx-max_ndp_size, GFP_NOIO);
Where is this freed?
The intention wqas to free it in the NCM_COMMIT_NDP case.
Infact after allocating the pointer, I make a copy of it in the driver state
(drvstate) variable, and get back to it later.
Is this wrong?
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
But to prevent
[PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
This patch introduces a new NCM tx engine, able to operate in standard-
and huawei-style mode.
In the first case, the NDP is disposed after the initial headers and
before any datagram.
What works:
- is able to communicate with compliant NCM devices:
I tested this with a board running the Linux g_ncm gadget driver.
What doesn't work:
- After some packets I start gettint LOTS of EVENT_RX_MEMORY from usbnet,
which fails to allocate an RX SKB in rx_submit(). Don't understand why,
any suggestion would be very welcome.
The tx_fixup function given here, even if actually working, should be
considered as an example: the NCM manager is used here simulating the
cdc_ncm.c behaviour.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/huawei_cdc_ncm.c | 187 ++-
1 file changed, 185 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index 735f7da..217802a 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -29,6 +29,35 @@
#include linux/usb/cdc-wdm.h
#include linux/usb/cdc_ncm.h
+/* NCM management operations: */
+
+/* NCM_INIT_FRAME: prepare for a new frame.
+ * NTH16 header is written to output SKB, NDP data is reset and last
+ * committed NDP pointer set to NULL.
+ * Now, data may be added to this NCM package.
+ */
+#define NCM_INIT_FRAME 1
+
+/* NCM_UPDATE_NDP: adds data to an NDP structure, hence updating it.
+ * Some checks are performed to be sure data fits in, respecting device and
+ * spec constrains.
+ * Normally the NDP is kept in memory and committed to the SKB only when
+ * requested. However, calling this method after NCM_COMMIT_NDP, causes it to
+ * work directly on the already committed SKB copy. this allows for flexibility
+ * in frame ordering.
+ */
+#define NCM_UPDATE_NDP 2
+
+/* Write NDP: commits NDP to output SKB.
+ * This method should be called only once per frame.
+ */
+#define NCM_COMMIT_NDP 3
+
+/* Finalizes NTH16 header: to be called when working in
+ * update-already-committed mode.
+ */
+#define NCM_FINALIZE_NTH 5
+
/* Driver data */
struct huawei_cdc_ncm_state {
struct cdc_ncm_ctx *ctx;
@@ -36,6 +65,16 @@ struct huawei_cdc_ncm_state {
struct usb_driver *subdriver;
struct usb_interface *control;
struct usb_interface *data;
+
+ /* Keeps track of where data starts and ends in SKBs. */
+ int data_start;
+ int data_len;
+
+ /* Last committed NDP for post-commit operations. */
+ struct usb_cdc_ncm_ndp16 *skb_ndp;
+
+ /* Non-committed NDP */
+ struct usb_cdc_ncm_ndp16 *ndp;
};
static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
@@ -53,6 +92,149 @@ static int huawei_cdc_ncm_manage_power(struct usbnet
*usbnet_dev, int on)
return 0;
}
+/* huawei_ncm_mgmt: flexible TX NCM manager.
+ *
+ * Once a non-zero status value is rturned, current frame should be discarded
+ * and operations restarted from scratch.
+ */
+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
[PATCH RFC] 1/2 cdc_ncm: export cdc_ncm_align_tail
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...@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
--
To unsubscribe from this list: send the line unsubscribe netdev in
[PATCH RFC 0/2] huawei_cdc_ncm: new NCM TX stack for Huawei-style NCM
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 -- To unsubscribe from this list: send the line unsubscribe netdev in
Re: [RFC PATCH] cdc_ncm: support moving the NDP part of the frame to the end of NCM package
Hello guys.
Sorry for the long delay / timing: but I arrived to the conclusion I had to
shut up and inform myself and write somecode.
:D
Yes, you madeit!
So here are my findings and answers.
On Tue, 9 Jun 2015, Oliver Neukum wrote:
==Date: Tue, 9 Jun 2015 12:38:59
==From: Oliver Neukum oneu...@suse.com
==To: Enrico Mioso mrkiko...@gmail.com
==Cc: netdev@vger.kernel.org
==Subject: Re: [RFC PATCH] cdc_ncm: support moving the NDP part of the frame to
==the end of NCM package
==
==On Tue, 2015-06-09 at 09:46 +0200, Enrico Mioso wrote:
==
== ==Not another parameter please. If the alternate frames are processed as
== ==well as the current frames, all is well and we can generally produces
== ==such frames. If not, we want a black list.
==
== I would agree on this point: and I was exploring different alternatives
also,
== such as having this option settable when invoking cdc_ncm_bind_common from
== huawei_cdc_ncm for example. Unfortunately, I don't feel confident we could
do
== that.
==
==Using a module parameter is simply wrong. A system can be connected to
==multiple devices (in turn). As a minimum the feature must be per device.
==
Sure.
== First of all: this driver supports more devices than Huawei ones, so I feel
we
== have chances to break them modifying the frame structure.
==
==In theory we must never break or impede compliant devices for
==uncompliant devices. Yet I doubt we can break any device doing what
==Windows does.
==Does it have a standard NCM driver that works with Huawei devices?
==
== Even more complicated is the fact that huawei NCM devices are not easily
== distinguishable one another from the driver perspective. Some heuristics
may be
== put in place probably, but I don't have access to old enough NCM devices to
== know best.
== The Huawei vendor driver put NDPs at end of frame - and does so for all
devices
== in my opinion, but I can't be really sure about that.
==
==Doesn't it have a list of supported devices?
I don't have a Windows box readily at disposal to look into - partly due to
accessiblity problems here. Still, Mobile Partner comes with it's own drivers
for NCM devices.
==
== Not now. Anyway I can change this as desired. Would something like a sysfs
knob
== be preferable? User-space surely has a good chance to tell us what to do
here.
==
==Not really. The answer will come from a list. The question is just
==whether easier updates are worth splitting up the fix.
==
The Linux huawei vendor driver defines various kinds of NDIS interfaces: but
doesn't threat them differently when it comes to NCM framing: it always writes
the NDP after the datagrams sequence, at least from what I understand reading
the code.
Some device work with both the cdc_ncm-way and huawei_cdc-way, others don't.
== == --- a/include/linux/usb/cdc_ncm.h
== == +++ b/include/linux/usb/cdc_ncm.h
== == @@ -93,6 +93,7 @@ struct cdc_ncm_ctx {
== == const struct usb_cdc_mbim_desc *mbim_desc;
== == const struct usb_cdc_mbim_extended_desc *mbim_extended_desc;
== == const struct usb_cdc_ether_desc *ether_desc;
== == + struct usb_cdc_ncm_ndp16 *delayed_ndp;
== ==
== ==What prevents you from embedding this in the structure?
== ==
== == Regards
== == Oliver
== ==
== ==
== ==
== Sorry - I think I didn't understand your ocmment here. Still, I am open to
any
== suggestion.
==
==Why don't you put struct usb_cdc_ncm_ndp16 delayed_ndp as opposed to a
==pointer to it into struct cdc_ncm_ctx. You never need more than one at a
==time.
==
== Regards
== Oliver
==
==
==
I guess this will be clear reading the code I am posting here.
Based on the situation and constrains, I decided to re-write the NCM tx stack:
and write a huawei specific one, in the huawei_cdc_ncm.c driver.
I can actually produce SKBs which are processed and seemigly received from the
Linux cdc_ncm rx_fixup function, still not by the device I am testing this on,
an E3131 3G dongle.
I don't know exactly why at the moment - still, I imagine two possibilities.
I might be aligning things wrong, or I might be doing some programming errors
and corrupting the data somewhere. Any suggestion on how to verify this are
welcome.
Si I post here the entire huawei_cdc_ncm.c file, since a patch would be
probably not so smaller.
The only things I changed are still huawei_ncm_mgmt, huawei_ncm_tx_fixup and
some defines got added.
I decided to split the NCM engine in two parts:
1 - The manager: which performs operations on frames based on what you ask.
2 - The packet processing path, which is in huawei_cdc_ncm_tx_fixup.
I expect all kinds of errors here: still hoping my work can show a serious
intention in trying to do my best.
Obviously, printks sparkled all-over, huawei_show_ndp and probably many checks
will go away. Some of them are redundant probably and make the code ugly.
I put them in place at least so I can avoid rebooting my virtual machine :D .
thank you,
Enrico
Re: [RFC PATCH] cdc_ncm: support moving the NDP part of the frame to the end of NCM package
First of all - thank you for your patience and time reading this message,
reviewing my code.
On Mon, 8 Jun 2015, Oliver Neukum wrote:
==Date: Mon, 8 Jun 2015 12:53:23
==From: Oliver Neukum oneu...@suse.com
==To: Enrico Mioso mrkiko...@gmail.com
==Cc: netdev@vger.kernel.org
==Subject: Re: [RFC PATCH] cdc_ncm: support moving the NDP part of the frame to
==the end of NCM package
==
==On Mon, 2015-06-08 at 12:44 +0200, Enrico Mioso wrote:
== The NCM specification as per
== http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip
== does not define a fixed position for different frame parts.
== The NCM header is effectively the head of a list of NDPs (NCM datagram
== pointers), each of them pointing to a set of ethernet frames.
== In general, the NDP can be anywhere after the header.
== Some devices however are not quite respecting the specs - as they mandate
== the NDP pointers to be after the payload, at the end of the NCM package.
== This patch aims to support this scenario, introducing a module parameter
== enabling this funcitonality.
==
== Is this approach acceptable?
==
== What does work:
== - the module compiles, and seems to cause no crash
== What doesn't:
== - the device for now will ignore our frames
==
==Why?
I will need to look trought wireshark to understand things better I think - and
I will do so once I get che chance. However, I think I am probably misaligning
frames and missing to set properly NDP indexes. Any suggestion on this is
welcome / appreciated.
==
== - I would need some guidance on flags to use with kzalloc.
==
==Probably GFP_NOIO if you run NFS over it, but that raises
==a question.
==
== thank you for the patience, and the review.
==
== Signed-off-by: Enrico Mioso mrkiko...@gmail.com
== ---
== drivers/net/usb/cdc_ncm.c | 30 --
== include/linux/usb/cdc_ncm.h | 1 +
== 2 files changed, 29 insertions(+), 2 deletions(-)
==
== diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
== index 8067b8f..a6d0666b 100644
== --- a/drivers/net/usb/cdc_ncm.c
== +++ b/drivers/net/usb/cdc_ncm.c
== @@ -60,6 +60,10 @@ static bool prefer_mbim;
== module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
== MODULE_PARM_DESC(prefer_mbim, Prefer MBIM setting on dual NCM/MBIM
functions);
==
== +static bool move_ndp_to_end;
== +module_param(move_ndp_to_end, bool, S_IRUGO | S_IWUSR);
== +MODULE_PARM_DESC(move_ndp_to_end, Move NDP block to the end of the NCM
aggregate);
==
==Not another parameter please. If the alternate frames are processed as
==well as the current frames, all is well and we can generally produces
==such frames. If not, we want a black list.
I would agree on this point: and I was exploring different alternatives also,
such as having this option settable when invoking cdc_ncm_bind_common from
huawei_cdc_ncm for example. Unfortunately, I don't feel confident we could do
that.
First of all: this driver supports more devices than Huawei ones, so I feel we
have chances to break them modifying the frame structure.
Even more complicated is the fact that huawei NCM devices are not easily
distinguishable one another from the driver perspective. Some heuristics may be
put in place probably, but I don't have access to old enough NCM devices to
know best.
The Huawei vendor driver put NDPs at end of frame - and does so for all devices
in my opinion, but I can't be really sure about that.
Not now. Anyway I can change this as desired. Would something like a sysfs knob
be preferable? User-space surely has a good chance to tell us what to do here.
==
== +
== static void cdc_ncm_txpath_bh(unsigned long param);
== static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
== static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
== @@ -684,6 +688,8 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
== ctx-tx_curr_skb = NULL;
== }
==
== + kfree(ctx-delayed_ndp);
== +
== kfree(ctx);
== }
==
== @@ -994,6 +1000,9 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
== ndpoffset = le16_to_cpu(ndp16-wNextNdpIndex);
== }
==
== + if ((move_ndp_to_end) (ctx-delayed_ndp-dwSignature == sign))
== + return ndp16;
== +
== /* align new NDP */
== cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
==
== @@ -1008,7 +1017,13 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
== nth16-wNdpIndex = cpu_to_le16(skb-len);
==
== /* push a new empty NDP */
== - ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
== + if (!move_ndp_to_end) {
== + ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
== + } else {
== + ndp16 = kzalloc(sizeof(*ndp16), GFP_KERNEL);
== + ctx-delayed_ndp = ndp16;
== + }
== +
== ndp16-dwSignature = sign
[RFC PATCH] cdc_ncm: support moving the NDP part of the frame to the end of NCM package
The NCM specification as per
http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip
does not define a fixed position for different frame parts.
The NCM header is effectively the head of a list of NDPs (NCM datagram
pointers), each of them pointing to a set of ethernet frames.
In general, the NDP can be anywhere after the header.
Some devices however are not quite respecting the specs - as they mandate
the NDP pointers to be after the payload, at the end of the NCM package.
This patch aims to support this scenario, introducing a module parameter
enabling this funcitonality.
Is this approach acceptable?
What does work:
- the module compiles, and seems to cause no crash
What doesn't:
- the device for now will ignore our frames
- I would need some guidance on flags to use with kzalloc.
thank you for the patience, and the review.
Signed-off-by: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_ncm.c | 30 --
include/linux/usb/cdc_ncm.h | 1 +
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..a6d0666b 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -60,6 +60,10 @@ static bool prefer_mbim;
module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(prefer_mbim, Prefer MBIM setting on dual NCM/MBIM
functions);
+static bool move_ndp_to_end;
+module_param(move_ndp_to_end, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(move_ndp_to_end, Move NDP block to the end of the NCM
aggregate);
+
static void cdc_ncm_txpath_bh(unsigned long param);
static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
@@ -684,6 +688,8 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
ctx-tx_curr_skb = NULL;
}
+ kfree(ctx-delayed_ndp);
+
kfree(ctx);
}
@@ -994,6 +1000,9 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
ndpoffset = le16_to_cpu(ndp16-wNextNdpIndex);
}
+ if ((move_ndp_to_end) (ctx-delayed_ndp-dwSignature == sign))
+ return ndp16;
+
/* align new NDP */
cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
@@ -1008,7 +1017,13 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
nth16-wNdpIndex = cpu_to_le16(skb-len);
/* push a new empty NDP */
- ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
+ if (!move_ndp_to_end) {
+ ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
+ } else {
+ ndp16 = kzalloc(sizeof(*ndp16), GFP_KERNEL);
+ ctx-delayed_ndp = ndp16;
+ }
+
ndp16-dwSignature = sign;
ndp16-wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) +
sizeof(struct usb_cdc_ncm_dpe16));
return ndp16;
@@ -1023,6 +1038,8 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff
*skb, __le32 sign)
struct sk_buff *skb_out;
u16 n = 0, index, ndplen;
u8 ready2send = 0;
+ unsigned int skb_prev_len;
+ char *delayed_alloc_ptr;
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
@@ -1074,7 +1091,8 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff
*skb, __le32 sign)
ndp16 = cdc_ncm_ndp(ctx, skb_out, sign, skb-len +
ctx-tx_modulus + ctx-tx_remainder);
/* align beginning of next frame */
- cdc_ncm_align_tail(skb_out, ctx-tx_modulus,
ctx-tx_remainder, ctx-tx_max);
+ if (!move_ndp_to_end)
+ cdc_ncm_align_tail(skb_out, ctx-tx_modulus,
ctx-tx_remainder, ctx-tx_max);
/* check if we had enough room left for both NDP and frame */
if (!ndp16 || skb_out-len + skb-len ctx-tx_max) {
@@ -,6 +1129,14 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff
*skb, __le32 sign)
dev_kfree_skb_any(skb);
skb = NULL;
+ if ((move_ndp_to_end) (ctx-delayed_ndp)) {
+ skb_prev_len = skb_out-len;
+ delayed_alloc_ptr = memset(skb_put(skb_out,
ctx-max_ndp_size), 0, ctx-max_ndp_size);
+ memcpy(ctx-delayed_ndp, delayed_alloc_ptr,
sizeof(struct usb_cdc_ncm_ndp16));
+ kfree(ctx-delayed_ndp);
+ cdc_ncm_align_tail(skb_out, ctx-tx_modulus,
ctx-tx_remainder, ctx-tx_max);
+ }
+
/* send now if this NDP is full */
if (index = CDC_NCM_DPT_DATAGRAMS_MAX) {
ready2send = 1;
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 7c9b484..cc02a0d 100644
--- a/include/linux/usb
[PATCH RFC 1/2] cdc_ncm: add the currently processed NDP frame to global driver data
This is useful to split up the cdc_ncm_ndp function later on.
The resulting code will be anyway stateful.
Signed-Off-By: Enrico Mioso mrkiko...@gmail.com
---
include/linux/usb/cdc_ncm.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 7c9b484..9172256 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -100,6 +100,7 @@ struct cdc_ncm_ctx {
struct sk_buff *tx_curr_skb;
struct sk_buff *tx_rem_skb;
__le32 tx_rem_sign;
+ struct usb_cdc_ncm_ndp16 *tx_curr_ndp16;
spinlock_t mtx;
atomic_t stop;
--
2.4.2
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC 0/2] cdc_ncm refactoring
I changed my mind, and decided to try in following this new way. This series splits the cdc_ncm_ndp function in two parts: - one that finds NDP blocks already present in the SKB being sent out - one that pushes new ones, starting from where the _find function left. After this splitting it seems more easy to modify the location where the NDP is disposed. What do you think about this? From now on, I need a little bit of help: I think we might work on the cdc_ncm_ndp16_push function, still I am open to any suggestion. Let me know if you like this. Enrico Enrico Mioso (2): cdc_ncm: add the currently processed NDP frame to global driver data cdc_ncm: split the cdc_ncm_ndp funciton drivers/net/usb/cdc_ncm.c | 30 +- include/linux/usb/cdc_ncm.h | 1 + 2 files changed, 22 insertions(+), 9 deletions(-) -- 2.4.2 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC 2/2] cdc_ncm: split the cdc_ncm_ndp funciton
Split this function in two new ones:
- cdc_ncm_ndp16_find: finds an NDP block in the chain mathcing a supplied
signature; a pointer to it is returned in case of success;
- cdc_ncm_ndp16_push: create and add to skb a new NDP block;
cdc_ncm_ndp16_push refers to the last NDP visited by cdc_ncm_ndp16_find, hence
this code is stateful.
Signed-Off-By: Enrico Mioso mrkiko...@gmail.com
---
drivers/net/usb/cdc_ncm.c | 30 +-
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..3c837d6 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -980,7 +980,7 @@ static void cdc_ncm_align_tail(struct sk_buff *skb, size_t
modulus, size_t remai
/* return a pointer to a valid struct usb_cdc_ncm_ndp16 of type sign, possibly
* allocating a new one within skb
*/
-static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct
sk_buff *skb, __le32 sign, size_t reserve)
+static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16_find(struct cdc_ncm_ctx *ctx,
struct sk_buff *skb, __le32 sign)
{
struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
@@ -988,12 +988,20 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
/* follow the chain of NDPs, looking for a match */
while (ndpoffset) {
- ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data + ndpoffset);
- if (ndp16-dwSignature == sign)
- return ndp16;
+ ctx-tx_curr_ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb-data +
ndpoffset);
+ if (ctx-tx_curr_ndp16-dwSignature == sign)
+ ndp16 = ctx-tx_curr_ndp16;
ndpoffset = le16_to_cpu(ndp16-wNextNdpIndex);
}
+
+ return ndp16;
+}
+static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16_push(struct cdc_ncm_ctx *ctx,
struct sk_buff *skb, __le32 sign, size_t reserve)
+{
+ struct usb_cdc_ncm_ndp16 *ndp16 = ctx-tx_curr_ndp16;
+ struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
+
/* align new NDP */
cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx-tx_max);
@@ -1070,11 +1078,15 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct
sk_buff *skb, __le32 sign)
break;
}
- /* get the appropriate NDP for this skb */
- ndp16 = cdc_ncm_ndp(ctx, skb_out, sign, skb-len +
ctx-tx_modulus + ctx-tx_remainder);
-
- /* align beginning of next frame */
- cdc_ncm_align_tail(skb_out, ctx-tx_modulus,
ctx-tx_remainder, ctx-tx_max);
+ /* search for the appropriate NDP for this skb */
+ ndp16 = cdc_ncm_ndp16_find(ctx, skb_out, sign);
+
+ if (ndp16 == NULL)
+ {
+ ndp16 = cdc_ncm_ndp16_push(ctx, skb_out, sign, skb-len +
ctx-tx_modulus + ctx-tx_remainder);
+ }
+else
+ cdc_ncm_align_tail(skb_out, ctx-tx_modulus,
ctx-tx_remainder, ctx-tx_max);
/* check if we had enough room left for both NDP and frame */
if (!ndp16 || skb_out-len + skb-len ctx-tx_max) {
--
2.4.2
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [cdc_ncm] guidance and help refactoring cdc_ncm
Hello Greg, hello everyone reading. I am sorry If I wasn't specific enough - I'll be this time! :) Or I try at least. On Mon, 1 Jun 2015, Greg KH wrote: ==Date: Mon, 1 Jun 2015 02:59:17 ==From: Greg KH g...@kroah.com ==To: Enrico Mioso mrkiko...@gmail.com ==Cc: linux-...@vger.kernel.org, netdev@vger.kernel.org, ==Oliver Neukum oli...@neukum.org ==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm == ==On Sun, May 31, 2015 at 04:37:11PM +0200, Enrico Mioso wrote: == Hello guys. == I am writing to you all to ask for help and assistance in refactoring the == cdc_ncm driver to support newer devices. == In particular - I would need step-by-step guidance in doing this: or any == other kind of help would be anyway greatly apreciated. == == 1 - What we need: == We would need to refactor the driver to be able to re-order parts of the NCM == package itself. == In particular, being a single NCM frame composed of different parts, we would == need more flexibility in changing their order. == ==Do you have hardware that needs this now? What exactly needs to be done ==here that currently doesn't work? yes - there is hardware that curently doesn't work with the actual code. In particular, I am referring to Huawei 3G / 4G modems: - Huawei E3131: will not work with some firmware versions, works with others / olders ones - Huawei E3372 (LTE modem): will not work. I received various mail messages from people trying to configure different devices that aren't working: and partly the situation is confusing since sometimes devices with very similar product names are pretty different, or derive from different hardware branches. Regarding what needs to be done: it's important to note that those devices follow an USB specification. the network control Model spec as found here: http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip aims to provide more efficient netowkring over USB solutions, batching frames for example. The fundamental packet unit here is the NCM Package: which can hold more ethernet-style frames in it. The device and the host will negotiate once appropriate some frame characteristics. The specification doesn't mandate where some parts of the package should be placed: infact, they can be put in somewhat arbitrary places. This is true for the NDP part: we actually as I understand it, are putting it near the beginning of the frame. Some Huawei devices, started to mandate it to be at the end of it, ignoring the frame otherwise. == == 2 - What might be nice == To do so, it would be nice to have the driver queue up frames, sending them == out as needed. this already happens to a certain extent, but the NCM package == is created in the process and updated in the while as I understood the code. == The best thing would be to have the NCM package created only before sending == it out, to achieve for best performance and code readability. == ==Would this really make things faster? Probably it would, depending on the setup we are considering. Considering a standard setup where those devices are being connected to a laptop or a PC with relatively high resources, this would not make so much difference. But it's not so unusual to see these devices running coupled with small devices: and here this could make the difference in some cases. But this would not be my main goal: getting things working faster is good, but I would like just to see them working now: and so I am trying to gather help / information / guidance / code in general so in case I might try if needed in the future. == == I already contactedprivately some of you to have some more insight on what == needs to be done, and to understand better how to organize the effort. I == unfortunately miss the time to do this right now: and infact I can't even be == sure to be able to do this, due to various problems (my tesis, my life in == general). == But gathering more informations and in general trying to get some help is == the best thing I feel like doing right now. == == The compelling reasons I find for trying to fix the situation are: == 1 - The fact these drivers are used in different products integrating or == interfacing with 3G/4G technologies. == ==Is there hardware that has out-of-tree drivers that implement what you ==are referring to here? Or does someone just want this to make the ==hardware work better? == ==I think we need more specifics before being able to determine exactly ==what needs to be done. == ==thanks, == ==greg k-h == Thank you Greg for your precious help. Once again - some devices will just not work. There is an out-of-tree vendor driver implementing what I am referring to: it contains code to work with many different devices from Huawei, but only the NCM related parts would be of use in this scenario. Other devices are already supported and working in the kernel. the driver can be found here: https://dl.dropboxusercontent.com/u/15043728/ArchLinuxArm
Re: [cdc_ncm] guidance and help refactoring cdc_ncm
Thank you Oliver, thank you all for reading this thread and the attention. For a more detailed discussion and how we got here, you might google for the thread: Is this 32-bit NCM? and Is this 32-bit NCM?y (follow up). Where the y letter comes from a mistake of mine. The specification does only mandate the position of the NTH (header). The rest can be in any order and position in general. This will work with most devices: except, of course, those from Huawei. Our aggregate looks something like this from my perspective (anyone correct me in case): NTH: header NDP: contains indexing informations ethernet packet 1 ethernet packet 2 ... ethernet packet n; While it should look like: NTH: header ethernet packet 1 ethernet packet 2 ... ethernet packet n; NDP: contains indexing informations but, when introducing such a change: you might break other devices now working. Infact, clearly there are multiple vendors producing NCM device, as you might also see by looking at the dirver's comments. So in general, we should be able to dynamically change the way in which the driver order things in the package. and that's why I initially proposed the redesign. thank you guys, for the patience and time. Enrico -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [cdc_ncm] guidance and help refactoring cdc_ncm
On Mon, 1 Jun 2015, Oliver Neukum wrote: ==Date: Mon, 1 Jun 2015 14:00:22 ==From: Oliver Neukum oneu...@suse.com ==To: Enrico Mioso mrkiko...@gmail.com ==Cc: you...@gmail.com, Greg KH g...@kroah.com, linux-...@vger.kernel.org, ==netdev@vger.kernel.org ==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm == ==On Mon, 2015-06-01 at 13:41 +0200, Enrico Mioso wrote: == Thank you Oliver, thank you all for reading this thread and the attention. == For a more detailed discussion and how we got here, you might google for the == thread: == Is this 32-bit NCM? == and == Is this 32-bit NCM?y (follow up). == Where the y letter comes from a mistake of mine. == ==Having read them it looks like the issues of padding and ==sequence numbers are open. == == The specification does only mandate the position of the NTH (header). The rest == can be in any order and position in general. This will work with most devices: == except, of course, those from Huawei. == ==Indeed. And a redesign for crap devices looks like ==a bad idea. == == Our aggregate looks something like this from my perspective (anyone correct me == in case): == NTH: header == NDP: contains indexing informations == ethernet packet 1 == ethernet packet 2 == ... == ethernet packet n; == == While it should look like: == NTH: header == ethernet packet 1 == ethernet packet 2 == ... == ethernet packet n; == NDP: contains indexing informations == == but, when introducing such a change: you might break other devices now working. == Infact, clearly there are multiple vendors producing NCM device, as you might == also see by looking at the dirver's comments. == So in general, we should be able to dynamically change the way in which the == driver order things in the package. == and that's why I initially proposed the redesign. == ==OK, so the NDP needs to be at the end. However in the old thread ==you state that this requires the NDP to be built between the ==final aggregate and physically transmitting. I think this is a false ==choice. You could just as well copy the NDP around provided you reserve ==enough space at the end of the skb. Yes, probably you can do so. I am not against anything at this moment. == == Regards == Oliver == == == == -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [cdc_ncm] guidance and help refactoring cdc_ncm
Thank you Oliver for the reply.
On Mon, 1 Jun 2015, Oliver Neukum wrote:
==Date: Mon, 1 Jun 2015 09:48:26
==From: Oliver Neukum oneu...@suse.com
==To: Enrico Mioso mrkiko...@gmail.com
==Cc: Greg KH g...@kroah.com, linux-...@vger.kernel.org,
==netdev@vger.kernel.org, you...@gmail.com
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Mon, 2015-06-01 at 08:53 +0200, Enrico Mioso wrote:
==
== A 32-bit version of the driver (talking 32-bit NCM) is here:
== http://www.gstorm.eu/cdc_ncm.c
== I modified the original driver with the help of a very talented friend.
== It works: but there seem to be no real reasons to implement this properly.
We
== did this in a consistent effort to understand what was wrong, and here it
is.
==
==Well, you are really talking about two different things here.
==Do we ever fail because we only do 16 bit as opposed to 32 bit?
==Your 32 bit driver looks good, but it raises the question of what to do
==if this test:
==
==if (le16_to_cpu(ctx-ncm_parm.bmNtbFormatsSupported)
==USB_CDC_NCM_NTB32_SUPPORTED) {
==
==fails. Otherwise it looks ready for merging.
==
== Regards
== Oliver
==
==
==
==
Oh - I am sorry. Infact, I am taling about two different things here. No, I've
seen no device failing because of this (16 vs. 32 bit problem).
I was mentioning this only as an additional thing to consider, at least from my
side.
We are failing on some cases only because of the position we put the NDP part
of the NCM frame. Infact, this 32-bit driver will work when the 16 bit one
does, and fail when the 16 bit one does.
Thank you for your review and consideration.
It would be nice to see this code merged - but I think we might need to merge
the two drivers in a way that reduces code de-duplication. But this might be
work for a second take in case.
Thank you again,
Enrico
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC cdc_ncm] introducing allocation mode
First of all - hello everyone, and thank you for the help.
I am here to learn something - so I am happy about having the opportunity of
discussing this with you. I am a newbie in development, sure.
On Mon, 1 Jun 2015, Oliver Neukum wrote:
==Date: Mon, 1 Jun 2015 14:00:22
==From: Oliver Neukum oneu...@suse.com
==To: Enrico Mioso mrkiko...@gmail.com
==Cc: you...@gmail.com, Greg KH g...@kroah.com, linux-...@vger.kernel.org,
==netdev@vger.kernel.org
==Subject: Re: [cdc_ncm] guidance and help refactoring cdc_ncm
==
==On Mon, 2015-06-01 at 13:41 +0200, Enrico Mioso wrote:
== Thank you Oliver, thank you all for reading this thread and the attention.
== For a more detailed discussion and how we got here, you might google for
the
== thread:
== Is this 32-bit NCM?
== and
== Is this 32-bit NCM?y (follow up).
== Where the y letter comes from a mistake of mine.
==
==Having read them it looks like the issues of padding and
==sequence numbers are open.
==
No - we tested the sequence number thing - and it didn't influence the device
behaviour.
We also tested the padding if I am not wrong: with the same results. Bjorn Mork
did a great job in making the cdc_ncm driver more flexible and rendering
debugging so much easier. I thank him.
== The specification does only mandate the position of the NTH (header). The
rest
== can be in any order and position in general. This will work with most
devices:
== except, of course, those from Huawei.
==
==Indeed. And a redesign for crap devices looks like
==a bad idea.
==
== Our aggregate looks something like this from my perspective (anyone correct
me
== in case):
== NTH: header
== NDP: contains indexing informations
== ethernet packet 1
== ethernet packet 2
== ...
== ethernet packet n;
==
== While it should look like:
== NTH: header
== ethernet packet 1
== ethernet packet 2
== ...
== ethernet packet n;
== NDP: contains indexing informations
==
== but, when introducing such a change: you might break other devices now
working.
== Infact, clearly there are multiple vendors producing NCM device, as you
might
== also see by looking at the dirver's comments.
== So in general, we should be able to dynamically change the way in which the
== driver order things in the package.
== and that's why I initially proposed the redesign.
==
==OK, so the NDP needs to be at the end. However in the old thread
==you state that this requires the NDP to be built between the
==final aggregate and physically transmitting. I think this is a false
==choice. You could just as well copy the NDP around provided you reserve
==enough space at the end of the skb.
==
== Regards
== Oliver
==
==
==
==
This might be true - so let's work on it. Reading the code I realized
that the cdc_ncm_ndp() (which btw I would like to have renamed to cdc_ncm_ndp16
as I did in a previous patchset), does something interesting here.
So - the function serches the already prepared frame for an appropriate NDP,
as the comment suggests. It uses the signature to find one. Until now, moving
the NDP at the end would pose no problems, even doing so dynamically (that's
the final goal).
If the appropriate ndp is found, we return it, going ahead if this doesn't
happen, updating our offset accordingly.
We return the ndp16 anyway for the function calling us to use it as reference.
Infact, the real copying of the ndp in the skb asI understand is happening
here:
ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb, ctx-max_ndp_size), 0,
ctx-max_ndp_size);
Infact we call cdc_ncm_align_tail (you can observe we call it here and also in
cdc_ncm_fill_tx_frame()).
So I am trying to change this, having this function work in two mode:
- the direct allocation mode: that's how it works now
- the non direct allocation mode: where the caller should do this work
What do you think about this ?
--
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8067b8f..bcd919d 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -978,9 +978,9 @@ static void cdc_ncm_align_tail(struct sk_buff *skb, size_t
modulus, size_t remai
}
/* return a pointer to a valid struct usb_cdc_ncm_ndp16 of type sign, possibly
- * allocating a new one within skb
+ * allocating a new one within skb if in direct allocation mode (normal mode
of operation).
*/
-static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct
sk_buff *skb, __le32 sign, size_t reserve)
+static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct
sk_buff *skb, __le32 sign, size_t reserve, unsigned int direct_allocation)
{
struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
struct usb_cdc_ncm_nth16 *nth16 = (void *)skb-data;
@@ -994,8 +994,9 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct
cdc_ncm_ctx *ctx, struct sk_
ndpoffset = le16_to_cpu(ndp16-wNextNdpIndex);
}
- /* align new NDP */
- cdc_ncm_align_tail(skb, ctx-tx_ndp_modulus, 0, ctx
[cdc_ncm] guidance and help refactoring cdc_ncm
Hello guys. I am writing to you all to ask for help and assistance in refactoring the cdc_ncm driver to support newer devices. In particular - I would need step-by-step guidance in doing this: or any other kind of help would be anyway greatly apreciated. 1 - What we need: We would need to refactor the driver to be able to re-order parts of the NCM package itself. In particular, being a single NCM frame composed of different parts, we would need more flexibility in changing their order. 2 - What might be nice To do so, it would be nice to have the driver queue up frames, sending them out as needed. this already happens to a certain extent, but the NCM package is created in the process and updated in the while as I understood the code. The best thing would be to have the NCM package created only before sending it out, to achieve for best performance and code readability. I already contactedprivately some of you to have some more insight on what needs to be done, and to understand better how to organize the effort. I unfortunately miss the time to do this right now: and infact I can't even be sure to be able to do this, due to various problems (my tesis, my life in general). But gathering more informations and in general trying to get some help is the best thing I feel like doing right now. The compelling reasons I find for trying to fix the situation are: 1 - The fact these drivers are used in different products integrating or interfacing with 3G/4G technologies. 2 - To gain more flexibility in the long term. Thank you guys for reading this message and everything. Please keep me in CC since I am not subscribed to this list. Enrico Mioso My Tox ID is: 7C593F402A3C8632D87AB4B948D492294C39A6A614464ECF843CA3429FB023284180472C7475 I like / reocmmend usage of open messaging standards: my preferred XMPP ID (JID) is: mrk...@jit.si. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
