CDC_NCM adding support IFF_NOARP for infineon modem platform

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

From: Wei Shuai <[email protected]>
Date: Sat, 12 Jan 2013 19:34:39 +0800

Quoted 4 lines

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

Is it no able to do ARP or, the more likely case, does broadcast not work at all?

If it's the latter, IFF_NOARP is just making over the real problem.

I'm not applying this, no hardware device should set IFF_NOARP. You probably really want IFF_POINTOPOINT or similar.

Quoted 8 lines

From: Wei Shuai <[email protected]>
Date: Sat, 12 Jan 2013 19:34:39 +0800

Quoted 4 lines

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

Quoted 7 lines

Is it no able to do ARP or, the more likely case, does broadcast not work at all?

If it's the latter, IFF_NOARP is just making over the real problem.

I'm not applying this, no hardware device should set IFF_NOARP. You probably really want IFF_POINTOPOINT or similar.

IFF_NOARP is already done for other WWAN devices (sierra_net, hso, cdc-ether, cdc-phonet, lg-vl600, etc) so there is some precedent. Some drivers (phonet, hso) set *both* POINTTOPOINT and NOARP. Is that redundant, and should all WWAN drivers be moved to only POINTTOPOINT?

(aside: usbnet has FLAG_POINTTOPOINT, but that's nothing to do with IFF_POINTTOPOINT, it only controls whether the interface is named usbX or ethX. Confusing.)

Quoted 1 line

should all WWAN drivers be moved to only POINTTOPOINT?

I can't answer any of your questions unless you tell me what the real limitation of these devices is.

From: Dan Williams <[email protected]>

Quoted 8 lines

IFF_NOARP is already done for other WWAN devices (sierra_net, hso, cdc-ether, cdc-phonet, lg-vl600, etc) so there is some precedent. Some drivers (phonet, hso) set *both* POINTTOPOINT and NOARP. Is that redundant, and should all WWAN drivers be moved to only POINTTOPOINT?

(aside: usbnet has FLAG_POINTTOPOINT, but that's nothing to do with IFF_POINTTOPOINT, it only controls whether the interface is named usbX or ethX. Confusing.)

I can't answer any of your questions unless you tell me what the real limitation of these devices is.

For the second time, is the problem that these devices cannot support broadcast packets properly?

The main problem is that these devices don't support ethernet. They support IP (v4 and _maybe_ v6) with an ethernet header. Many of them will do ARP (and IPv6 ND) as well to complete the picture, but some of them don't and that's what these drivers try to deal with.

Quoted 8 lines

From: Wei Shuai <[email protected]>
Date: Sat, 12 Jan 2013 19:34:39 +0800

Quoted 4 lines

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

Quoted 7 lines

Is it no able to do ARP or, the more likely case, does broadcast not work at all?

If it's the latter, IFF_NOARP is just making over the real problem.

I'm not applying this, no hardware device should set IFF_NOARP. You probably really want IFF_POINTOPOINT or similar.

IFF_NOARP is already done for other WWAN devices (sierra_net, hso, cdc-ether, cdc-phonet, lg-vl600, etc) so there is some precedent. Some drivers (phonet, hso) set *both* POINTTOPOINT and NOARP. Is that redundant, and should all WWAN drivers be moved to only POINTTOPOINT?

(aside: usbnet has FLAG_POINTTOPOINT, but that's nothing to do with IFF_POINTTOPOINT, it only controls whether the interface is named usbX or ethX. Confusing.)

Dan

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

Signed-off-by Wei Shuai <[email protected]>

---
 drivers/net/usb/cdc_ncm.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

drivers/net/usb/cdc_ncm.c +22 -0

index 71b6e92..6093e07 100644
@@ -55,6 +55,9 @@
 
 #define	DRIVER_VERSION				"14-Mar-2012"
 
+/* Flags for driver_info::data */
+#define CDC_NCM_DRIVER_DATA_NOARP  1
+
 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);

@@ -573,6 +576,10 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 		return -ENODEV;
 #endif
 
+	if (dev->driver_info->data & CDC_NCM_DRIVER_DATA_NOARP) {
+		/* some buggy device cannot do ARP*/
+		dev->net->flags |= IFF_NOARP;
+	}
 	/* NCM data altsetting is always 1 */
 	ret = cdc_ncm_bind_common(dev, intf, 1);
 

@@ -1155,6 +1162,21 @@ static const struct driver_info wwan_info = {
 	.tx_fixup = cdc_ncm_tx_fixup,
 };
 
+/* Same as wwan_info, but with IFF_NOARP  */
+static const struct driver_info wwan_noarp_info = {
+	.description = "Mobile Broadband Network Device (NO ARP)",
+	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+			| FLAG_WWAN,
+	.data = CDC_NCM_DRIVER_DATA_NOARP,
+	.bind = cdc_ncm_bind,
+	.unbind = cdc_ncm_unbind,
+	.check_connect = cdc_ncm_check_connect,
+	.manage_power = usbnet_manage_power,
+	.status = cdc_ncm_status,
+	.rx_fixup = cdc_ncm_rx_fixup,
+	.tx_fixup = cdc_ncm_tx_fixup,
+};
+
 static const struct usb_device_id cdc_devs[] = {
 	/* Ericsson MBM devices like F5521gw */
 	{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO

@@ -1194,6 +1216,13 @@ static const struct usb_device_id cdc_devs[] = {
 	  .driver_info = (unsigned long)&wwan_info,
 	},
 

Quoted 1 line

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

So given that Dave now approves of IFF_NOARP, let's change your original patch to add a FLAG_NOARP for the .flags structure instead of inventing another mechanism for .data.

Dan

Quoted 66 lines

Signed-off-by Wei Shuai <[email protected]>

---
 drivers/net/usb/cdc_ncm.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

drivers/net/usb/cdc_ncm.c +29 -0

index 71b6e92..6093e07 100644
@@ -55,6 +55,9 @@
 
 #define      DRIVER_VERSION                          "14-Mar-2012"
 
+/* Flags for driver_info::data */
+#define CDC_NCM_DRIVER_DATA_NOARP  1
+
 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);

@@ -573,6 +576,10 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
              return -ENODEV;
 #endif
 
+     if (dev->driver_info->data & CDC_NCM_DRIVER_DATA_NOARP) {
+             /* some buggy device cannot do ARP*/
+             dev->net->flags |= IFF_NOARP;
+     }
      /* NCM data altsetting is always 1 */
      ret = cdc_ncm_bind_common(dev, intf, 1);
 

@@ -1155,6 +1162,21 @@ static const struct driver_info wwan_info = {
      .tx_fixup = cdc_ncm_tx_fixup,
 };
 
+/* Same as wwan_info, but with IFF_NOARP  */
+static const struct driver_info wwan_noarp_info = {
+     .description = "Mobile Broadband Network Device (NO ARP)",
+     .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+                     | FLAG_WWAN,
+     .data = CDC_NCM_DRIVER_DATA_NOARP,
+     .bind = cdc_ncm_bind,
+     .unbind = cdc_ncm_unbind,
+     .check_connect = cdc_ncm_check_connect,
+     .manage_power = usbnet_manage_power,
+     .status = cdc_ncm_status,
+     .rx_fixup = cdc_ncm_rx_fixup,
+     .tx_fixup = cdc_ncm_tx_fixup,
+};
+
 static const struct usb_device_id cdc_devs[] = {
      /* Ericsson MBM devices like F5521gw */
      { .match_flags = USB_DEVICE_ID_MATCH_INT_INFO

@@ -1194,6 +1216,13 @@ static const struct usb_device_id cdc_devs[] = {
        .driver_info = (unsigned long)&wwan_info,
      },
 
+     /* Infineon(now Intel) HSPA Modem platform */
+     { USB_DEVICE_AND_INTERFACE_INFO(0x1519, 0x0443,
+             USB_CLASS_COMM,
+             USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
+       .driver_info = (unsigned long)&wwan_noarp_info,
+     },
+
      /* Generic CDC-NCM devices */
      { USB_INTERFACE_INFO(USB_CLASS_COMM,
              USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),

OK, I will follow up. after add FLAG_NOARP, how should I handle IFF_NOARP? will I do it in cdc_ncm.c or usb_net.c?

2013/1/17 Dan Williams <[email protected]>:

Quoted 65 lines · Wei Shuai

Quoted 1 line

Infineon(now Intel) HSPA Modem platform NCM cannot support ARP. so I introduce a flag CDC_NCM_DRIVER_DATA_NOARP which is defined in driver_info:data. so later on, if more such buggy devices are found, they could use same flag to handle.

So given that Dave now approves of IFF_NOARP, let's change your original patch to add a FLAG_NOARP for the .flags structure instead of inventing another mechanism for .data.

Dan

Quoted 56 lines

Signed-off-by Wei Shuai <[email protected]>

---
 drivers/net/usb/cdc_ncm.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

drivers/net/usb/cdc_ncm.c +22 -0

index 71b6e92..6093e07 100644
@@ -55,6 +55,9 @@
 
 #define      DRIVER_VERSION                          "14-Mar-2012"
 
+/* Flags for driver_info::data */
+#define CDC_NCM_DRIVER_DATA_NOARP  1
+
 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);

@@ -573,6 +576,10 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
              return -ENODEV;
 #endif
 
+     if (dev->driver_info->data & CDC_NCM_DRIVER_DATA_NOARP) {
+             /* some buggy device cannot do ARP*/
+             dev->net->flags |= IFF_NOARP;
+     }
      /* NCM data altsetting is always 1 */
      ret = cdc_ncm_bind_common(dev, intf, 1);
 

@@ -1155,6 +1162,21 @@ static const struct driver_info wwan_info = {
      .tx_fixup = cdc_ncm_tx_fixup,
 };
 
+/* Same as wwan_info, but with IFF_NOARP  */
+static const struct driver_info wwan_noarp_info = {
+     .description = "Mobile Broadband Network Device (NO ARP)",
+     .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+                     | FLAG_WWAN,
+     .data = CDC_NCM_DRIVER_DATA_NOARP,
+     .bind = cdc_ncm_bind,
+     .unbind = cdc_ncm_unbind,
+     .check_connect = cdc_ncm_check_connect,
+     .manage_power = usbnet_manage_power,
+     .status = cdc_ncm_status,
+     .rx_fixup = cdc_ncm_rx_fixup,
+     .tx_fixup = cdc_ncm_tx_fixup,
+};
+
 static const struct usb_device_id cdc_devs[] = {
      /* Ericsson MBM devices like F5521gw */
      { .match_flags = USB_DEVICE_ID_MATCH_INT_INFO

@@ -1194,6 +1216,13 @@ static const struct usb_device_id cdc_devs[] = {
        .driver_info = (unsigned long)&wwan_info,
      },
 

Quoted 12 lines

drivers/net/usb/cdc_ncm.c static const struct usb_device_id cdc_devs[] = { · line 1228

Quoted 10 lines

drivers/net/usb/cdc_ncm.c +7 -0

+     /* Infineon(now Intel) HSPA Modem platform */
+     { USB_DEVICE_AND_INTERFACE_INFO(0x1519, 0x0443,
+             USB_CLASS_COMM,
+             USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
+       .driver_info = (unsigned long)&wwan_noarp_info,
+     },
+
      /* Generic CDC-NCM devices */
      { USB_INTERFACE_INFO(USB_CLASS_COMM,
              USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html