[PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

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" <hns@goldelico.com>
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 <neilb@suse.de>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
---
 drivers/net/usb/hso.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index cba1d46..d146e26 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -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");
-- 
1.7.7.4

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[David Miller]

From: "Dr. H. Nikolaus Schaller" <hns@goldelico.com>
Date: Wed, 2 Oct 2013 09:00:18 +0200

> From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 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 <neilb@suse.de>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>

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.

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Hi,

Am 03.10.2013 um 21:29 schrieb David Miller:

> From: "Dr. H. Nikolaus Schaller" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Date: Wed, 2 Oct 2013 09:00:18 +0200
> 
>> From f5c7e15b61f2ce4fe3105ff914f6bfaf5d74af0d Mon Sep 17 00:00:00 2001
>> From: "H. Nikolaus Schaller" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>> 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 <neilb-l3A5Bk7waGM@public.gmane.org>
>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVIAdwXzzRB9H2Q@public.gmane.org>
> 
> 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.

Yes, that would be the right way.

The problem is that the firmware is a black box and the packet format has no
(public) documentation (unless someone inside OPTION has access to it).

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.

BR,
Nikolaus Schaller

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[David Miller]

From: "Dr. H. Nikolaus Schaller" <hns@goldelico.com>
Date: Thu, 3 Oct 2013 21:40:34 +0200

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

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.

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Am 03.10.2013 um 22:00 schrieb David Miller:

> From: "Dr. H. Nikolaus Schaller" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Date: Thu, 3 Oct 2013 21:40:34 +0200
> 
>> 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.
> 
> 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.

Good idea! I will resend the patch.

--- hns--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Hi,

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

> 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" <hns@goldelico.com>
> 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 <neilb@suse.de>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
> ---
> drivers/net/usb/hso.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index cba1d46..d146e26 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -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");
> -- 
> 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

BR,
Nikolaus

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dan Williams]

On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
> Hi,
> 
> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
> 
> > 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" <hns@goldelico.com>
> > 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 <neilb@suse.de>
> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
> > ---
> > drivers/net/usb/hso.c |    3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index cba1d46..d146e26 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -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");
> > -- 
> > 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

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

---
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 86292e6..1c7bdeb 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -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 {
 

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Hi Dan,

Am 16.12.2013 um 20:40 schrieb Dan Williams:

> On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
>> 
>>> 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" <hns@goldelico.com>
>>> 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 <neilb@suse.de>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
>>> ---
>>> drivers/net/usb/hso.c |    3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
>>> index cba1d46..d146e26 100644
>>> --- a/drivers/net/usb/hso.c
>>> +++ b/drivers/net/usb/hso.c
>>> @@ -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");
>>> -- 
>>> 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
> 
> 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.

> 
> 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().

Ok, that looks plausible.

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

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

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.

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

> 
> Dan
> 
> ---
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 86292e6..1c7bdeb 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -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 {
> 
> 

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dan Williams]

On Tue, 2013-12-17 at 20:56 +0100, Dr. H. Nikolaus Schaller wrote:
> Hi Dan,
> 
> Am 16.12.2013 um 20:40 schrieb Dan Williams:
> 
> > On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
> >> Hi,
> >> 
> >> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
> >> 
> >>> 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" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> >>> 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 <neilb-l3A5Bk7waGM@public.gmane.org>
> >>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVIAdwXzzRB9H2Q@public.gmane.org>
> >>> ---
> >>> drivers/net/usb/hso.c |    3 ++-
> >>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>> 
> >>> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> >>> index cba1d46..d146e26 100644
> >>> --- a/drivers/net/usb/hso.c
> >>> +++ b/drivers/net/usb/hso.c
> >>> @@ -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");
> >>> -- 
> >>> 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
> > 
> > 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.
> 
> > 
> > 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().
> 
> Ok, that looks plausible.
> 
> > 
> > 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.
> 
> > 
> > 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.
> 
> 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

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?

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

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

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

Dan

> BR,
> Nikolaus
> 
> > 
> > Dan
> > 
> > ---
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 86292e6..1c7bdeb 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -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 {
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Hi Dan,

Am 17.12.2013 um 23:27 schrieb Dan Williams:

> On Tue, 2013-12-17 at 20:56 +0100, Dr. H. Nikolaus Schaller wrote:
>> Hi Dan,
>> 
>> Am 16.12.2013 um 20:40 schrieb Dan Williams:
>> 
>>> On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
>>>> Hi,
>>>> 
>>>> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
>>>> 
>>>>> 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" <hns@goldelico.com>
>>>>> 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 <neilb@suse.de>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
>>>>> ---
>>>>> drivers/net/usb/hso.c |    3 ++-
>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
>>>>> index cba1d46..d146e26 100644
>>>>> --- a/drivers/net/usb/hso.c
>>>>> +++ b/drivers/net/usb/hso.c
>>>>> @@ -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");
>>>>> -- 
>>>>> 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
>>> 
>>> 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.
>> 
>>> 
>>> 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().
>> 
>> Ok, that looks plausible.
>> 
>>> 
>>> 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.
>> 
>>> 
>>> 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.
>> 
>> 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
> 
> 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.

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

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

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

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.

BR,
Nikolaus

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dan Williams]

On Wed, 2013-12-18 at 14:16 +0100, Dr. H. Nikolaus Schaller wrote:
> Hi Dan,
> 
> Am 17.12.2013 um 23:27 schrieb Dan Williams:
> 
> > On Tue, 2013-12-17 at 20:56 +0100, Dr. H. Nikolaus Schaller wrote:
> >> Hi Dan,
> >> 
> >> Am 16.12.2013 um 20:40 schrieb Dan Williams:
> >> 
> >>> On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
> >>>> Hi,
> >>>> 
> >>>> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
> >>>> 
> >>>>> 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" <hns@goldelico.com>
> >>>>> 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 <neilb@suse.de>
> >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
> >>>>> ---
> >>>>> drivers/net/usb/hso.c |    3 ++-
> >>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> >>>>> index cba1d46..d146e26 100644
> >>>>> --- a/drivers/net/usb/hso.c
> >>>>> +++ b/drivers/net/usb/hso.c
> >>>>> @@ -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");
> >>>>> -- 
> >>>>> 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
> >>> 
> >>> 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.
> >> 
> >>> 
> >>> 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().
> >> 
> >> Ok, that looks plausible.
> >> 
> >>> 
> >>> 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.
> >> 
> >>> 
> >>> 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.
> >> 
> >> 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
> > 
> > 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.

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

> > 
> >> 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.
> > 
> >> 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.
> 
> Yes, I did the mistake as you describe...
> 
> > 
> >>> 
> >>> 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
> 
> 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.

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

Dan

Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

[Dr. H. Nikolaus Schaller]

Hi Dan,

Am 18.12.2013 um 18:49 schrieb Dan Williams:

> On Wed, 2013-12-18 at 14:16 +0100, Dr. H. Nikolaus Schaller wrote:
>> Hi Dan,
>> 
>> Am 17.12.2013 um 23:27 schrieb Dan Williams:
>> 
>>> On Tue, 2013-12-17 at 20:56 +0100, Dr. H. Nikolaus Schaller wrote:
>>>> Hi Dan,
>>>> 
>>>> Am 16.12.2013 um 20:40 schrieb Dan Williams:
>>>> 
>>>>> On Fri, 2013-12-13 at 15:43 +0100, Dr. H. Nikolaus Schaller wrote:
>>>>>> Hi,
>>>>>> 
>>>>>> Am 02.10.2013 um 09:00 schrieb Dr. H. Nikolaus Schaller:
>>>>>> 
>>>>>>> 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" <hns@goldelico.com>
>>>>>>> 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 <neilb@suse.de>
>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.de>
>>>>>>> ---
>>>>>>> drivers/net/usb/hso.c |    3 ++-
>>>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
>>>>>>> index cba1d46..d146e26 100644
>>>>>>> --- a/drivers/net/usb/hso.c
>>>>>>> +++ b/drivers/net/usb/hso.c
>>>>>>> @@ -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");
>>>>>>> -- 
>>>>>>> 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
>>>>> 
>>>>> 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.
>>>> 
>>>>> 
>>>>> 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().
>>>> 
>>>> Ok, that looks plausible.
>>>> 
>>>>> 
>>>>> 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.
>>>> 
>>>>> 
>>>>> 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.
>>>> 
>>>> 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
>>> 
>>> 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.
> 
> 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 :)

Hm. Not that I am aware of. The driver loads automatically at boot time
and we use the AT_O commands to set up networking. Maybe it is simply
hidden because I see it in the ifconfig as hso0.

>  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.)
> 
>>> 
>>>> 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.
>>> 
>>>> 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.
>> 
>> Yes, I did the mistake as you describe...
>> 
>>> 
>>>>> 
>>>>> 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
>> 
>> 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.

Ah, I see. I did have it but not in the most current code. So I have added
a little printk().

Now I have firstly removed my patch and then added yours and I can confirm that it works fine:

w/o patch:

[  143.964111] tiocmget_intr_callback: serial de61b800
[  143.969207] tiocmget_intr_callback: status 00000000
[  143.974334] tiocmget_intr_callback: tiocmget dddd5d40
[  143.979614] tiocmget_intr_callback: serial_state_notification dddd5d60
[  143.986419] tiocmget_intr_callback: wValue 0000
[  143.991149] tiocmget_intr_callback: wValue 06
[  143.995727] tiocmget_intr_callback: wValue 0002
[  144.000457] usb 1-2: hso received invalid serial state notification
[  144.007019] hso[1522:tiocmget_intr_callback]a1 20 00 00 06 00 02 00 0b 00

with patch:

[  192.589172] tiocmget_intr_callback: serial de69da00
[  192.594299] tiocmget_intr_callback: status 00000000
[  192.599395] tiocmget_intr_callback: tiocmget de5b4540
[  192.604675] tiocmget_intr_callback: serial_state_notification de5b4560
[  192.611511] tiocmget_intr_callback: wValue 0000
[  192.616241] tiocmget_intr_callback: wValue 06
[  192.620788] tiocmget_intr_callback: wValue 0002

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

And I have found that the tiocmget_intr_callback is only active if I open the Modem port.
If I use the Application port, I don't see any tiocmget_intr_callback messages
(but the RING text message ).

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

So I can confirm for the GTM601:

Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

BR,
Nikolaus

[PATCH] hso: fix handling of modem port SERIAL_STATE notifications

[Dan Williams]

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 <dcbw@redhat.com>
Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/net/usb/hso.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 86292e6..1a48234 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -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,41 @@ 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;
+
+	/* wIndex should be the USB interface number of the port to which the
+	 * notification applies, which should always be the Modem port.
+	 */
 	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 {
 
-- 
1.8.3.1

Re: [PATCH] hso: fix handling of modem port SERIAL_STATE notifications

[David Miller]

From: Dan Williams <dcbw@redhat.com>
Date: Mon, 06 Jan 2014 10:07:29 -0600

> 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 <dcbw@redhat.com>
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

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

Re: [PATCH] hso: fix handling of modem port SERIAL_STATE notifications

[Dan Williams]

On Mon, 2014-01-06 at 16:30 -0500, David Miller wrote:
> From: Dan Williams <dcbw@redhat.com>
> Date: Mon, 06 Jan 2014 10:07:29 -0600
> 
> > 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 <dcbw@redhat.com>
> > Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> 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?

Dan

Re: [PATCH] hso: fix handling of modem port SERIAL_STATE notifications

[David Miller]

From: Dan Williams <dcbw@redhat.com>
Date: Mon, 06 Jan 2014 15:39:40 -0600

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

You don't have to if you think the BUG_ON() is warranted.