Re: [PATCH 1/1] hso: fix problem with wrong status code sent byOPTION 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@xxxxxxxxxxxxx>
> >>>>> 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@xxxxxxx>
> >>>>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/