hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION
GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

From: "Dr. H. Nikolaus Schaller" <[email protected]>
Date: Wed, 2 Oct 2013 09:00:18 +0200

Quoted 10 lines

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

Quoted 4 lines

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

I think we should look more deeply into what this bit might mean and why the firmware might be setting it before we even consider applying a patch like this one.

I have made the bug observation from debug log that this bit is set in a response each time the modem has a RING message. It might be specific to this modem and firmware version, i.e. a firmware bug. But we have no means to verify or even change it in the firmware.

I.e. the driver must handle it in a better way.

Because the notification is rejected by the driver, the driver will hang up and the whole modem connection breaks down.

With this patch, the problem was never observed again in ~2 years.

I'd hope the maintainer of this driver can shed some light on it.

From: "Dr. H. Nikolaus Schaller" <[email protected]>
Date: Thu, 3 Oct 2013 21:40:34 +0200

Quoted 10 lines

I have made the bug observation from debug log that this bit is set in a response each time the modem has a RING message. It might be specific to this modem and firmware version, i.e. a firmware bug. But we have no means to verify or even change it in the firmware.

I.e. the driver must handle it in a better way.

Because the notification is rejected by the driver, the driver will hang up and the whole modem connection breaks down.

Quoted 3 lines

With this patch, the problem was never observed again in ~2 years.

I'd hope the maintainer of this driver can shed some light on it.

I think if you suspect the bit is set in response to RING messages then you should define a macro so that the number is not just magic, and put a descriptrive comment above the macro definition for that bit.

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

https://lkml.org/lkml/2013/10/9/263

Quoted 56 lines

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

Quoted 1 line

https://lkml.org/lkml/2013/10/9/263

Likely because nobody formally submitted the patch with a signed-off-by, which indicates their intent that the patch is tested, correct, and can be merged to the kernel.

I looked at this today, and I'm left wondering how any port other than HSO_PORT_MODEM can get the notification via tiocmget_intr_callback(). "serial->tiocmget" is only created for HSO_PORT_MODEM serial ports (see hso_create_bulk_serial_device()):

	if ((port & HSO_PORT_MASK) == HSO_PORT_MODEM) {
		num_urbs = 2;
		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
					   GFP_KERNEL);

and the code in tiocmget_intr_callback() does this:

	tiocmget = serial->tiocmget;
	if (!tiocmget)
		return;

which should mean that only a HSO_PORT_MODEM will ever process the notification. Further, the tiocmget interrupt URB is only created and submitted if serial->tiocmget != NULL, so only HSO_PORT_MODEM should ever be calling into tiocmget_intr_callback().

So I think Eric's patch is actually wrong because it will *always* pass the new check.

The original code had the correct intention, but the original code was obviously wrong for newer devices where the port layout is read from firmware and not from static tables, and thus for recent devices where the modem interface is not USB interface #2.

Can you confirm/deny that the 'modem' interface for your GTM601 is USB interface #6? For example, my Icon 452 has the following USB interface layout:

0: Diag
1: GPS
2: GPS control
3: Application
4: Control
5: Network
6: Modem

So like the GTM601, I would expect any RING notifications to appear for wIndex=0x06.

Does the following patch fix the problem for you?

Dan

---

drivers/net/usb/hso.c +8 -2

index 86292e6..1c7bdeb 100644
@@ -181,15 +181,14 @@ enum rx_ctrl_state{
 	RX_SENT,
 	RX_PENDING
 };
 
 #define BM_REQUEST_TYPE (0xa1)
 #define B_NOTIFICATION  (0x20)
 #define W_VALUE         (0x0)
-#define W_INDEX         (0x2)
 #define W_LENGTH        (0x2)
 
 #define B_OVERRUN       (0x1<<6)
 #define B_PARITY        (0x1<<5)
 #define B_FRAMING       (0x1<<4)
 #define B_RING_SIGNAL   (0x1<<3)
 #define B_BREAK         (0x1<<2)

@@ -1483,31 +1482,38 @@ static void tiocmget_intr_callback(struct urb *urb)
 	struct hso_serial *serial = urb->context;
 	struct hso_tiocmget *tiocmget;
 	int status = urb->status;
 	u16 UART_state_bitmap, prev_UART_state_bitmap;
 	struct uart_icount *icount;
 	struct hso_serial_state_notification *serial_state_notification;
 	struct usb_device *usb;
+	int if_num;
 
 	/* Sanity checks */
 	if (!serial)
 		return;
 	if (status) {
 		handle_usb_error(status, __func__, serial->parent);
 		return;
 	}
+
+	/* tiocmget is only supported on HSO_PORT_MODEM */
 	tiocmget = serial->tiocmget;
 	if (!tiocmget)
 		return;
+	BUG_ON((serial->parent->port_spec & HSO_PORT_MASK) != HSO_PORT_MODEM);
+
 	usb = serial->parent->usb;
+	if_num = serial->parent->interface->altsetting->desc.bInterfaceNumber;
+
 	serial_state_notification = &tiocmget->serial_state_notification;
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    le16_to_cpu(serial_state_notification->wIndex) != if_num ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");
 		DUMP(serial_state_notification,
 		     sizeof(struct hso_serial_state_notification));
 	} else {
 

Hi Dan,

Am 16.12.2013 um 20:40 schrieb Dan Williams:

Quoted 57 lines · Dr. H. Nikolaus Schaller

Quoted 56 lines

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

Quoted 5 lines

Quoted 1 line

https://lkml.org/lkml/2013/10/9/263

Likely because nobody formally submitted the patch with a signed-off-by, which indicates their intent that the patch is tested, correct, and can be merged to the kernel.

Ok, I see. I just wasn't aware of the proposal at all since I missed the discussion and wasn't on CC.

Therefore I have added Eric to the CC.

Quoted 13 lines

I looked at this today, and I'm left wondering how any port other than HSO_PORT_MODEM can get the notification via tiocmget_intr_callback(). "serial->tiocmget" is only created for HSO_PORT_MODEM serial ports (see hso_create_bulk_serial_device()):

	if ((port & HSO_PORT_MASK) == HSO_PORT_MODEM) {
		num_urbs = 2;
		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
					   GFP_KERNEL);

and the code in tiocmget_intr_callback() does this:

Quoted 8 lines

	tiocmget = serial->tiocmget;
	if (!tiocmget)
		return;

which should mean that only a HSO_PORT_MODEM will ever process the notification. Further, the tiocmget interrupt URB is only created and submitted if serial->tiocmget != NULL, so only HSO_PORT_MODEM should ever be calling into tiocmget_intr_callback().

Ok, that looks plausible.

Quoted 8 lines

So I think Eric's patch is actually wrong because it will *always* pass the new check.

The original code had the correct intention, but the original code was obviously wrong for newer devices where the port layout is read from firmware and not from static tables, and thus for recent devices where the modem interface is not USB interface #2.

This explains why we did run into the problem with the GTM601.

Quoted 13 lines

Can you confirm/deny that the 'modem' interface for your GTM601 is USB interface #6? For example, my Icon 452 has the following USB interface layout:

0: Diag
1: GPS
2: GPS control
3: Application
4: Control
5: Network
6: Modem

Quoted 2 lines

So like the GTM601, I would expect any RING notifications to appear for wIndex=0x06.

Interestingly I see:

0: Diagnostic
1: GPS
2: GPS Control
3: Application
4: Control
5: Modem

I.e. there is no Network interface. Originally we did focus on /dev/ttyHS3

to access the Application interface, but people reported that it is not stable or always so. Therefore we have some udev rule to add symbolic names (e.g. /dev/ttyHS_Application):

http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=GTA04/udev-rules/hso.rules;hb=HEAD

So I think the assignment is not even always the same on the same device.

And I am a little puzzled why we do see the wIndex == 6 if it is the interface number. Maybe it happens only in some scenario where the Modem becomes interface #6.

Quoted 2 lines

Does the following patch fix the problem for you?

I will try but need a little time to setup a test scenario (because I need to trigger RING indications) and will then report.

BR,
Nikolaus

Quoted 55 lines

Dan

---

drivers/net/usb/hso.c +7 -1

index 86292e6..1c7bdeb 100644
@@ -181,15 +181,14 @@ enum rx_ctrl_state{
 	RX_SENT,
 	RX_PENDING
 };
 
 #define BM_REQUEST_TYPE (0xa1)
 #define B_NOTIFICATION  (0x20)
 #define W_VALUE         (0x0)
-#define W_INDEX         (0x2)
 #define W_LENGTH        (0x2)
 
 #define B_OVERRUN       (0x1<<6)
 #define B_PARITY        (0x1<<5)
 #define B_FRAMING       (0x1<<4)
 #define B_RING_SIGNAL   (0x1<<3)
 #define B_BREAK         (0x1<<2)

@@ -1483,31 +1482,38 @@ static void tiocmget_intr_callback(struct urb *urb)
 	struct hso_serial *serial = urb->context;
 	struct hso_tiocmget *tiocmget;
 	int status = urb->status;
 	u16 UART_state_bitmap, prev_UART_state_bitmap;
 	struct uart_icount *icount;
 	struct hso_serial_state_notification *serial_state_notification;
 	struct usb_device *usb;
+	int if_num;
 
 	/* Sanity checks */
 	if (!serial)
 		return;
 	if (status) {
 		handle_usb_error(status, __func__, serial->parent);
 		return;
 	}
+
+	/* tiocmget is only supported on HSO_PORT_MODEM */
 	tiocmget = serial->tiocmget;
 	if (!tiocmget)
 		return;
+	BUG_ON((serial->parent->port_spec & HSO_PORT_MASK) != HSO_PORT_MODEM);
+
 	usb = serial->parent->usb;
+	if_num = serial->parent->interface->altsetting->desc.bInterfaceNumber;
+
 	serial_state_notification = &tiocmget->serial_state_notification;
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||

drivers/net/usb/hso.c static void tiocmget_intr_callback(struct urb *urb) · line 1519

Quoted 10 lines

drivers/net/usb/hso.c +1 -1

 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    le16_to_cpu(serial_state_notification->wIndex) != if_num ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");
 		DUMP(serial_state_notification,
 		     sizeof(struct hso_serial_state_notification));
 	} else {
 

--
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

Quoted 132 lines

Hi Dan,

Am 16.12.2013 um 20:40 schrieb Dan Williams:

Quoted 57 lines · Dr. H. Nikolaus Schaller

Quoted 56 lines

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

Quoted 5 lines

Quoted 1 line

https://lkml.org/lkml/2013/10/9/263

Likely because nobody formally submitted the patch with a signed-off-by, which indicates their intent that the patch is tested, correct, and can be merged to the kernel.

Ok, I see. I just wasn't aware of the proposal at all since I missed the discussion and wasn't on CC.

Therefore I have added Eric to the CC.

Quoted 13 lines

I looked at this today, and I'm left wondering how any port other than HSO_PORT_MODEM can get the notification via tiocmget_intr_callback(). "serial->tiocmget" is only created for HSO_PORT_MODEM serial ports (see hso_create_bulk_serial_device()):

	if ((port & HSO_PORT_MASK) == HSO_PORT_MODEM) {
		num_urbs = 2;
		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
					   GFP_KERNEL);

and the code in tiocmget_intr_callback() does this:

Quoted 8 lines

	tiocmget = serial->tiocmget;
	if (!tiocmget)
		return;

which should mean that only a HSO_PORT_MODEM will ever process the notification. Further, the tiocmget interrupt URB is only created and submitted if serial->tiocmget != NULL, so only HSO_PORT_MODEM should ever be calling into tiocmget_intr_callback().

Ok, that looks plausible.

Quoted 8 lines

So I think Eric's patch is actually wrong because it will *always* pass the new check.

The original code had the correct intention, but the original code was obviously wrong for newer devices where the port layout is read from firmware and not from static tables, and thus for recent devices where the modem interface is not USB interface #2.

This explains why we did run into the problem with the GTM601.

Quoted 13 lines

Can you confirm/deny that the 'modem' interface for your GTM601 is USB interface #6? For example, my Icon 452 has the following USB interface layout:

0: Diag
1: GPS
2: GPS control
3: Application
4: Control
5: Network
6: Modem

Quoted 2 lines

So like the GTM601, I would expect any RING notifications to appear for wIndex=0x06.

Interestingly I see:

0: Diagnostic
1: GPS
2: GPS Control
3: Application
4: Control
5: Modem

Quoted 1 line

I.e. there is no Network interface. Originally we did focus on /dev/ttyHS3

How are you determining the number here? Are you using:

cat /sys/class/tty/ttyHS_Modem/device/bInterfaceNumber

to determine the actual USB interface number associated with the Modem port? Or are you going off the pre-udev-rename ttyHSx numbers?

Quoted 8 lines

to access the Application interface, but people reported that it is not stable or always so. Therefore we have some udev rule to add symbolic names (e.g. /dev/ttyHS_Application):

http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=GTA04/udev-rules/hso.rules;hb=HEAD

So I think the assignment is not even always the same on the same device.

The assignment will always be the same on the same device *and the same firmware*. That is because the hso driver asks the firmware for the port layout, and that is what is put into the 'hsotype' file in sysfs that the udev rules use. So if your telephony/WWAN stack is using the udev symbolic links or reading 'hsotype' directly, you can be assured that ttyHS_Modem is always the port you should be using for PPP.

Quoted 3 lines

And I am a little puzzled why we do see the wIndex == 6 if it is the interface number. Maybe it happens only in some scenario where the Modem becomes interface #6.

I'm still not convinced that that it isn't #6 until we agree on how we're figuring that out :) I may well assume to little of you, so forgive me if you are actually reporting the real USB interface # of the ports. But I've seen too many people confuse the tty numbers with USB interface numbers, so I must ask.

Quoted 6 lines

Quoted 3 lines

Does the following patch fix the problem for you?

I will try but need a little time to setup a test scenario (because I need to trigger RING indications) and will then report.

Here's a RING on the 452 with Modem @ usbif #6:

[65808.711323] tiocmget_intr_callback: W_INDEX 0006
[65808.711330] tiocmget_intr_callback: UART bits 000a
[65808.711333] tiocmget_intr_callback: Modem ifnum 06

and B_TX_CARRIER notification with GIO322 with Modem @ usbif #2:

[66029.365441] tiocmget_intr_callback: W_INDEX 0002
[66029.365444] tiocmget_intr_callback: UART bits 0002
[66029.365445] tiocmget_intr_callback: Modem ifnum 02

Hi Dan,

Am 17.12.2013 um 23:27 schrieb Dan Williams:

Quoted 133 lines · Dr. H. Nikolaus Schaller

Quoted 132 lines

Hi Dan,

Am 16.12.2013 um 20:40 schrieb Dan Williams:

Quoted 57 lines · Dr. H. Nikolaus Schaller

Quoted 56 lines

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

Quoted 5 lines

Quoted 1 line

https://lkml.org/lkml/2013/10/9/263

Likely because nobody formally submitted the patch with a signed-off-by, which indicates their intent that the patch is tested, correct, and can be merged to the kernel.

Ok, I see. I just wasn't aware of the proposal at all since I missed the discussion and wasn't on CC.

Therefore I have added Eric to the CC.

Quoted 13 lines

I looked at this today, and I'm left wondering how any port other than HSO_PORT_MODEM can get the notification via tiocmget_intr_callback(). "serial->tiocmget" is only created for HSO_PORT_MODEM serial ports (see hso_create_bulk_serial_device()):

	if ((port & HSO_PORT_MASK) == HSO_PORT_MODEM) {
		num_urbs = 2;
		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
					   GFP_KERNEL);

and the code in tiocmget_intr_callback() does this:

Quoted 8 lines

	tiocmget = serial->tiocmget;
	if (!tiocmget)
		return;

which should mean that only a HSO_PORT_MODEM will ever process the notification. Further, the tiocmget interrupt URB is only created and submitted if serial->tiocmget != NULL, so only HSO_PORT_MODEM should ever be calling into tiocmget_intr_callback().

Ok, that looks plausible.

Quoted 8 lines

So I think Eric's patch is actually wrong because it will *always* pass the new check.

The original code had the correct intention, but the original code was obviously wrong for newer devices where the port layout is read from firmware and not from static tables, and thus for recent devices where the modem interface is not USB interface #2.

This explains why we did run into the problem with the GTM601.

Quoted 13 lines

Can you confirm/deny that the 'modem' interface for your GTM601 is USB interface #6? For example, my Icon 452 has the following USB interface layout:

0: Diag
1: GPS
2: GPS control
3: Application
4: Control
5: Network
6: Modem

Quoted 2 lines

So like the GTM601, I would expect any RING notifications to appear for wIndex=0x06.

Interestingly I see:

0: Diagnostic
1: GPS
2: GPS Control
3: Application
4: Control
5: Modem

Quoted 8 lines

Quoted 1 line

I.e. there is no Network interface. Originally we did focus on /dev/ttyHS3

How are you determining the number here? Are you using:

cat /sys/class/tty/ttyHS_Modem/device/bInterfaceNumber

to determine the actual USB interface number associated with the Modem port? Or are you going off the pre-udev-rename ttyHSx numbers?

Ah, I did use

root@gta04:~#  ls -d /sys/class/tty/ttyHS*; cat /sys/class/tty/ttyHS*/hsotype
/sys/class/tty/ttyHS0  /sys/class/tty/ttyHS1  /sys/class/tty/ttyHS2  /sys/class/tty/ttyHS3  /sys/class/tty/ttyHS4  /sys/class/tty/ttyHS5
Diagnostic
GPS
GPS Control
Application
Control
Modem

With bInterfaceNumber I get

root@gta04:~# cat /sys/class/tty/ttyHS*/device/bInterfaceNumber
00
01
02
03
04
06

So the Modem is indeed interface 06 and there is just no ttyHS_Network.

Quoted 20 lines

Quoted 8 lines

to access the Application interface, but people reported that it is not stable or always so. Therefore we have some udev rule to add symbolic names (e.g. /dev/ttyHS_Application):

http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=GTA04/udev-rules/hso.rules;hb=HEAD

So I think the assignment is not even always the same on the same device.

The assignment will always be the same on the same device *and the same firmware*. That is because the hso driver asks the firmware for the port layout, and that is what is put into the 'hsotype' file in sysfs that the udev rules use. So if your telephony/WWAN stack is using the udev symbolic links or reading 'hsotype' directly, you can be assured that ttyHS_Modem is always the port you should be using for PPP.

Quoted 3 lines

And I am a little puzzled why we do see the wIndex == 6 if it is the interface number. Maybe it happens only in some scenario where the Modem becomes interface #6.

Quoted 5 lines

I'm still not convinced that that it isn't #6 until we agree on how we're figuring that out :) I may well assume to little of you, so forgive me if you are actually reporting the real USB interface # of the ports. But I've seen too many people confuse the tty numbers with USB interface numbers, so I must ask.

Yes, I did the mistake as you describe...

Quoted 13 lines

Quoted 6 lines

Quoted 3 lines

Does the following patch fix the problem for you?

I will try but need a little time to setup a test scenario (because I need to trigger RING indications) and will then report.

Here's a RING on the 452 with Modem @ usbif #6:

[65808.711323] tiocmget_intr_callback: W_INDEX 0006
[65808.711330] tiocmget_intr_callback: UART bits 000a
[65808.711333] tiocmget_intr_callback: Modem ifnum 06

Quoted 5 lines

and B_TX_CARRIER notification with GIO322 with Modem @ usbif #2:

[66029.365441] tiocmget_intr_callback: W_INDEX 0002
[66029.365444] tiocmget_intr_callback: UART bits 0002
[66029.365445] tiocmget_intr_callback: Modem ifnum 02

Hm. I didn't see these messages (on 3.12). How do I enable them?

BTW: the RING (or +CRING:) text messages do appear on the Application port, while the NO CARRIER after ATA and ending the call appears on the Modem port.

Quoted 167 lines

Hi Dan,

Am 17.12.2013 um 23:27 schrieb Dan Williams:

Quoted 133 lines · Dr. H. Nikolaus Schaller

Quoted 132 lines

Hi Dan,

Am 16.12.2013 um 20:40 schrieb Dan Williams:

Quoted 57 lines · Dr. H. Nikolaus Schaller

Quoted 56 lines

Hi,

Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:

Quoted 48 lines

Hi Jan,

we are using a GTM601 modem (Firmware 1.7) for a while and have spotted an issue that under some conditions the modem sends a packed wIndex over USB that is rejected by the hso driver making troubles afterwards. Not rejecting makes it work fine.

BR,
Nikolaus Schaller

---

From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <[email protected]>
Date: Thu, 15 Nov 2012 14:40:57 +0100
Subject: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

It has been observed that the GTM601 with 1.7 firmware sometimes sends a value wIndex that has bit 0x04 set instead of being reset as the code expects. So we mask it for the error check.

See http://lists.goldelico.com/pipermail/gta04-owner/2012-February/001643.html

Signed-off-by NeilBrown <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/net/usb/hso.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/usb/hso.c +2 -1

index cba1d46..d146e26 100644
@@ -1503,7 +1503,8 @@ static void tiocmget_intr_callback(struct urb *urb)
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
-	    le16_to_cpu(serial_state_notification->wIndex) != W_INDEX ||
+	    (le16_to_cpu(serial_state_notification->wIndex) & ~0x4) !=
+		W_INDEX ||
 	    le16_to_cpu(serial_state_notification->wLength) != W_LENGTH) {
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");

Signature · 1 line

1.7.7.4

I have found this (better) proposal by OPTION, but wonder what did happen to it. It neither shows in mainline 3.13-rc nor linux-next:

Quoted 5 lines

Quoted 1 line

https://lkml.org/lkml/2013/10/9/263

Likely because nobody formally submitted the patch with a signed-off-by, which indicates their intent that the patch is tested, correct, and can be merged to the kernel.

Ok, I see. I just wasn't aware of the proposal at all since I missed the discussion and wasn't on CC.

Therefore I have added Eric to the CC.

Quoted 13 lines

I looked at this today, and I'm left wondering how any port other than HSO_PORT_MODEM can get the notification via tiocmget_intr_callback(). "serial->tiocmget" is only created for HSO_PORT_MODEM serial ports (see hso_create_bulk_serial_device()):

	if ((port & HSO_PORT_MASK) == HSO_PORT_MODEM) {
		num_urbs = 2;
		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
					   GFP_KERNEL);

and the code in tiocmget_intr_callback() does this:

Quoted 8 lines

	tiocmget = serial->tiocmget;
	if (!tiocmget)
		return;

which should mean that only a HSO_PORT_MODEM will ever process the notification. Further, the tiocmget interrupt URB is only created and submitted if serial->tiocmget != NULL, so only HSO_PORT_MODEM should ever be calling into tiocmget_intr_callback().

Ok, that looks plausible.

Quoted 8 lines

So I think Eric's patch is actually wrong because it will *always* pass the new check.

The original code had the correct intention, but the original code was obviously wrong for newer devices where the port layout is read from firmware and not from static tables, and thus for recent devices where the modem interface is not USB interface #2.

This explains why we did run into the problem with the GTM601.

Quoted 13 lines

Can you confirm/deny that the 'modem' interface for your GTM601 is USB interface #6? For example, my Icon 452 has the following USB interface layout:

0: Diag
1: GPS
2: GPS control
3: Application
4: Control
5: Network
6: Modem

Quoted 2 lines

So like the GTM601, I would expect any RING notifications to appear for wIndex=0x06.

Interestingly I see:

0: Diagnostic
1: GPS
2: GPS Control
3: Application
4: Control
5: Modem

Quoted 8 lines

Quoted 1 line

I.e. there is no Network interface. Originally we did focus on /dev/ttyHS3

How are you determining the number here? Are you using:

cat /sys/class/tty/ttyHS_Modem/device/bInterfaceNumber

to determine the actual USB interface number associated with the Modem port? Or are you going off the pre-udev-rename ttyHSx numbers?

Ah, I did use

root@gta04:~#  ls -d /sys/class/tty/ttyHS*; cat /sys/class/tty/ttyHS*/hsotype
/sys/class/tty/ttyHS0  /sys/class/tty/ttyHS1  /sys/class/tty/ttyHS2  /sys/class/tty/ttyHS3  /sys/class/tty/ttyHS4  /sys/class/tty/ttyHS5
Diagnostic
GPS
GPS Control
Application
Control
Modem

With bInterfaceNumber I get

root@gta04:~# cat /sys/class/tty/ttyHS*/device/bInterfaceNumber
00
01
02
03
04
06

Quoted 1 line

So the Modem is indeed interface 06 and there is just no ttyHS_Network.

Ah, excellent.

I assume you use the "disable_net" module parameter for hso.ko to disable the net device? You likely don't want to do that :)

If you
disable the net device and rely on PPP, you're severely limiting the throughput as PPP overhead does not allow reaching HSPA data rates. The GTM601 can only do HSPA 14.4 though, so it's not a huge problem, but if you ever want to use a faster module (HSPA+ 21 or 42 or LTE) you definitely want to ditch PPP.

(PPP is only used between the modem and the host; it's not used over the air... so it's just overhead and another point of failure. The netdevice and the custom Option AT commands for IP details are simpler and faster.)

Quoted 47 lines

Quoted 20 lines

Quoted 8 lines

to access the Application interface, but people reported that it is not stable or always so. Therefore we have some udev rule to add symbolic names (e.g. /dev/ttyHS_Application):

http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=GTA04/udev-rules/hso.rules;hb=HEAD

So I think the assignment is not even always the same on the same device.

The assignment will always be the same on the same device *and the same firmware*. That is because the hso driver asks the firmware for the port layout, and that is what is put into the 'hsotype' file in sysfs that the udev rules use. So if your telephony/WWAN stack is using the udev symbolic links or reading 'hsotype' directly, you can be assured that ttyHS_Modem is always the port you should be using for PPP.

Quoted 3 lines

And I am a little puzzled why we do see the wIndex == 6 if it is the interface number. Maybe it happens only in some scenario where the Modem becomes interface #6.

Quoted 5 lines

I'm still not convinced that that it isn't #6 until we agree on how we're figuring that out :) I may well assume to little of you, so forgive me if you are actually reporting the real USB interface # of the ports. But I've seen too many people confuse the tty numbers with USB interface numbers, so I must ask.

Yes, I did the mistake as you describe...

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

Does the following patch fix the problem for you?

I will try but need a little time to setup a test scenario (because I need to trigger RING indications) and will then report.

Here's a RING on the 452 with Modem @ usbif #6:

[65808.711323] tiocmget_intr_callback: W_INDEX 0006
[65808.711330] tiocmget_intr_callback: UART bits 000a
[65808.711333] tiocmget_intr_callback: Modem ifnum 06

and B_TX_CARRIER notification with GIO322 with Modem @ usbif #2:

[66029.365441] tiocmget_intr_callback: W_INDEX 0002
[66029.365444] tiocmget_intr_callback: UART bits 0002
[66029.365445] tiocmget_intr_callback: Modem ifnum 02

Quoted 1 line

Hm. I didn't see these messages (on 3.12). How do I enable them?

I added them to the driver manually so I could ensure that the fix I posted was working. I noticed similar messages in the logs you originally posted debugging the problem, so I assumed you had some custom logging of your own.

Quoted 4 lines

BTW: the RING (or +CRING:) text messages do appear on the Application port, while the NO CARRIER after ATA and ending the call appears on the Modem port.

Ah fun. Which would explain why I never saw the RING message myself, since I was using the Modem port for AT communication and calling the SIM from another phone.

If you can can test this patch out and confirm/deny that it corrects the problem for you, then I'll post an official version of it.

The existing serial state notification handling expected older Option devices, having a hardcoded assumption that the Modem port was always USB interface #2. That isn't true for devices from the past few years.

hso_serial_state_notification is a local cache of a USB Communications Interface Class SERIAL_STATE notification from the device, and the USB CDC specification (section 6.3, table 67 "Class-Specific Notifications") defines wIndex as the USB interface the event applies to. For hso devices this will always be the Modem port, as the Modem port is the only port which is set up to receive them by the driver.

So instead of always expecting USB interface #2, instead validate the notification with the actual USB interface number of the Modem port.

Signed-off-by Dan Williams <[email protected]>

Tested-by H. Nikolaus Schaller <[email protected]>

From: Dan Williams <[email protected]>
Date: Mon, 06 Jan 2014 10:07:29 -0600

Quoted 11 lines

The existing serial state notification handling expected older Option devices, having a hardcoded assumption that the Modem port was always USB interface #2. That isn't true for devices from the past few years.

hso_serial_state_notification is a local cache of a USB Communications Interface Class SERIAL_STATE notification from the device, and the USB CDC specification (section 6.3, table 67 "Class-Specific Notifications") defines wIndex as the USB interface the event applies to. For hso devices this will always be the Modem port, as the Modem port is the only port which is set up to receive them by the driver.

Quoted 5 lines

So instead of always expecting USB interface #2, instead validate the notification with the actual USB interface number of the Modem port.

Signed-off-by Dan Williams <[email protected]>

Tested-by H. Nikolaus Schaller <[email protected]>

Applied, although that BUG_ON() is a bit harsh. It might have been cleaner to just dev_warn() ratelimited or similar and return.

If the driver ever gets to that callback for any !Modem port, something is horribly wrong, which the BUG_ON() was attempting to make explicit. The driver only enables that callback for the Modem port, so yeah, it's somewhat defensive. I'm happy to send a fixup if you like?