Date:	Tue, 17 Dec 2013 20:56:33 +0100
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Dan Williams <dcbw@...hat.com>
Cc:	Jan Dumon <j.dumon@...ion.com>, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	Belisko Marek <marek.belisko@...il.com>, e.verdonck@...ion.com
Subject: Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication

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@...delico.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@...e.de>
>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.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 {
> 
> 

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

Powered by blists - more mailing lists