Package: linux-2.6
Version: 2.6.32-15
Severity: wishlist
Tags: patch

Hi,

Would it be possible to have the patches for Qualcomm Gobi 2000 USB WWAN
support merged in both 2.6.32 and 2.6.34? The patch breaks out code from the
option driver into a generic wwan set of code and then makes both the option
and qcserial drivers use it. I've built and tested both versions on my Thinkpad
X201 (based on 2.6.32-15 and 2.6.34-1~experimental.2) and the necessary code
for loading the firmware (gobi-loader) is in NEW.

The patches originally came from:

http://www.codon.org.uk/~mjg59/gobi_loader/kernel_patches/

and I've rebased them against the current trees to remove any offsets when
stuck at the end of the relevant series files. Also, in the 2.6.32 kernel,
some of the USB IDs added in the patch seem to have been merged in a stable
bugfix update, but the code changes and other IDs haven't, so I had to clean
that up.

The code has been merged upstream in the following commits, so will not be a
long term maintanence headache:

commit e07896e62abbf7a741a5cd5b25ba7637bdf91ad0
Author: Anssi Hannula <anssi.hannula@gmail.com>
Date: Thu Apr 1 12:31:10 2010 -0400

USB: qcserial: Add support for Qualcomm Gobi 2000 devices

Add ids for Qualcomm Gobi 2000 QDL and Modem modes. Gobi 2000 has a
single altsetting in QDL mode, so adapt code to handle that.

Firmware upload protocol is also slightly different, with an
additional firmware file. However, qcserial doesn't handle firmware
uploading.

Tested on Lenovo Thinkpad T510.

Signed-off-by: Anssi Hannula <anssi.hannula@gmail.com>
Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

commit 3d7e59ad88fdb6bc50ae9b7e822d4bb5f68b68f9
Author: Matthew Garrett <mjg@redhat.com>
Date: Thu Apr 1 12:31:09 2010 -0400

USB: qcserial: Use generic USB wwan code

Make qcserial use the generic USB wwan code. This should result in a
performance improvement.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

commit 8b4c6a3ab596961b784659c71dc24b341f938a1a
Author: Matthew Garrett <mjg@redhat.com>
Date: Thu Apr 1 12:31:08 2010 -0400

USB: option: Use generic USB wwan code

As this code was simply factored out of option, this is a simple
conversion.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

commit 0d4561947b8ddd5d944bdbbdc1ea1d6fd9a06041
Author: Matthew Garrett <mjg@redhat.com>
Date: Thu Apr 1 12:31:07 2010 -0400

usb serial: Add generic USB wwan support

The generic USB serial code is ill-suited for high-speed USB wwan devices,
resulting in the option driver. However, other non-option devices may also
gain similar benefits from not using the generic code. Factorise out the
non-option specific code from the option driver and make it available to
other users.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


Thanks,

Mark

-- System Information:
Debian Release: 5.0.4
APT prefers stable
APT policy: (500, 'stable')
Architecture: i386 (i686)

Kernel: Linux 2.6.26-2-686 (SMP w/1 CPU core)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash

On Sat, 2010-06-12 at 22:31 +0100, Mark Hymers wrote:
> Package: linux-2.6
> Version: 2.6.32-15
> Severity: wishlist
> Tags: patch
>
> Hi,
>
> Would it be possible to have the patches for Qualcomm Gobi 2000 USB WWAN
> support merged in both 2.6.32 and 2.6.34? The patch breaks out code from the
> option driver into a generic wwan set of code and then makes both the option
> and qcserial drivers use it. I've built and tested both versions on my Thinkpad
> X201 (based on 2.6.32-15 and 2.6.34-1~experimental.2) and the necessary code
> for loading the firmware (gobi-loader) is in NEW.
>
> The patches originally came from:
>
> http://www.codon.org.uk/~mjg59/gobi_loader/kernel_patches/
>
> and I've rebased them against the current trees to remove any offsets when
> stuck at the end of the relevant series files. Also, in the 2.6.32 kernel,
> some of the USB IDs added in the patch seem to have been merged in a stable
> bugfix update, but the code changes and other IDs haven't, so I had to clean
> that up.
[...]

I would prefer if you could provide one patch corresponding to each of
the upstream commits, for 2.6.32. This makes it easier to compare
versions and to resolve any future conflicts with stable updates.

There is no point in doing this for 2.6.34 as we will shortly be moving
to 2.6.35 release candidates in experimental.

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

On Sun, 13, Jun, 2010 at 03:33:43AM +0100, Ben Hutchings spoke thus..
> I would prefer if you could provide one patch corresponding to each of
> the upstream commits, for 2.6.32. This makes it easier to compare
> versions and to resolve any future conflicts with stable updates.

Ok. I've attached four patches which seem to be the relevant ones from
git. Note that I had to modify the second patch in order to apply on
top of the current tree. I've compiled and tested this and the device
works fine. Please note that I'm not a kernel hacker by any stretch of
the imagination, so let me know if I need to improve anything.

> There is no point in doing this for 2.6.34 as we will shortly be moving
> to 2.6.35 release candidates in experimental.

Ok, thanks.

Mark

--
Mark Hymers <mhy at debian dot org>

"'I regret nothing?' That's not a song, that's an idiots charter."
Andy Hamilton, Old Harry's Game

On Mon, 2010-06-14 at 14:15 +0100, Mark Hymers wrote:
> On Sun, 13, Jun, 2010 at 03:33:43AM +0100, Ben Hutchings spoke thus..
> > I would prefer if you could provide one patch corresponding to each of
> > the upstream commits, for 2.6.32. This makes it easier to compare
> > versions and to resolve any future conflicts with stable updates.
>
> Ok. I've attached four patches which seem to be the relevant ones from
> git. Note that I had to modify the second patch in order to apply on
> top of the current tree. I've compiled and tested this and the device
> works fine. Please note that I'm not a kernel hacker by any stretch of
> the imagination, so let me know if I need to improve anything.
>
> > There is no point in doing this for 2.6.34 as we will shortly be moving
> > to 2.6.35 release candidates in experimental.
>
> Ok, thanks.

These don't quite sum up to gobi_2.6.32.patch; the interdiff from
gobi_2.6.32.patch to these is:

> diff -u source/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> --- source/drivers/usb/serial/Kconfig 2010-06-11 21:50:52.087338327 +0100
> +++ b/drivers/usb/serial/Kconfig
> @@ -474,6 +474,7 @@
>
> config USB_SERIAL_QUALCOMM
> tristate "USB Qualcomm Serial modem"
> + select USB_SERIAL_WWAN

This presumably corrects a bug in gobi_2.6.32.patch.

> help
> Say Y here if you have a Qualcomm USB modem device. These are
> usually wireless cellular modems.
> @@ -566,7 +567,7 @@
> module will be called keyspan_pda.
>
> config USB_SERIAL_WWAN
> - tristate
> + tristate
>
> config USB_SERIAL_OPTION
> tristate "USB driver for GSM and CDMA modems"
> diff -u source/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
> --- source/drivers/usb/serial/qcserial.c 2010-06-11 21:52:03.599336246 +0100
> +++ b/drivers/usb/serial/qcserial.c
> @@ -15,6 +15,7 @@
> #include <linux/tty_flip.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/slab.h>
> #include "usb-wwan.h"
>
> #define DRIVER_AUTHOR "Qualcomm Inc"
> @@ -22,7 +23,7 @@
>
> static int debug;
>
> -static const struct usb_device_id id_table[] = {
> +static struct usb_device_id id_table[] = {
> {USB_DEVICE(0x05c6, 0x9211)}, /* Acer Gobi QDL device */
> {USB_DEVICE(0x05c6, 0x9212)}, /* Acer Gobi Modem Device */
> {USB_DEVICE(0x03f0, 0x1f1d)}, /* HP un2400 Gobi Modem Device */
> diff -u source/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> --- source/drivers/usb/serial/usb_wwan.c 2010-06-11 21:50:52.110835306 +0100
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -26,6 +26,7 @@
> #include <linux/kernel.h>
> #include <linux/jiffies.h>
> #include <linux/errno.h>
> +#include <linux/slab.h>
> #include <linux/tty.h>
> #include <linux/tty_flip.h>
> #include <linux/module.h>
> @@ -216,7 +217,6 @@
> } else {
> tty = tty_port_tty_get(&port->port);
> if (urb->actual_length) {
> - tty_buffer_request_room(tty, urb->actual_length);

Should this really have been removed?

> tty_insert_flip_string(tty, data, urb->actual_length);
> tty_flip_buffer_push(tty);
> } else
> @@ -224,9 +224,9 @@
> tty_kref_put(tty);
>
> /* Resubmit urb so we continue receiving */
> - if (port->port.count && status != -ESHUTDOWN) {
> + if (status != -ESHUTDOWN) {
> err = usb_submit_urb(urb, GFP_ATOMIC);
> - if (err)
> + if (err && err != -EPERM)

What's correct here?

> printk(KERN_ERR "%s: resubmit read urb failed. "
> "(%d)", __func__, err);
> else
> @@ -610,7 +610,7 @@
> for (i = 0; i < serial->num_ports; i++) {
> port = serial->port[i];
> if (!port->interrupt_in_urb) {
> - dbg("%s: No interrupt URB for port %d
", __func__, i);
> + dbg("%s: No interrupt URB for port %d", __func__, i);

The newline should definitely be included, though this is not really
important.

> continue;
> }
> err = usb_submit_urb(port->interrupt_in_urb, GFP_NOIO);
> diff -u source/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
> --- source/drivers/usb/serial/usb-wwan.h 2010-06-11 21:50:52.103759088 +0100
> +++ b/drivers/usb/serial/usb-wwan.h
> @@ -39,6 +39,7 @@
> unsigned int suspended:1;
> int in_flight;
> int (*send_setup) (struct usb_serial_port *port);
> + void *private;
> };
>
> struct usb_wwan_port_private {

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

On Tue, 15, Jun, 2010 at 01:44:45AM +0100, Ben Hutchings spoke thus..
> > diff -u source/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> > --- source/drivers/usb/serial/Kconfig 2010-06-11 21:50:52.087338327 +0100
> > +++ b/drivers/usb/serial/Kconfig
> > @@ -474,6 +474,7 @@
> >
> > config USB_SERIAL_QUALCOMM
> > tristate "USB Qualcomm Serial modem"
> > + select USB_SERIAL_WWAN
>
> This presumably corrects a bug in gobi_2.6.32.patch.

Yes and I suspect I didn't notice before as USB_SERIAL_WWAN would have
been selected anyways.

> > -static const struct usb_device_id id_table[] = {
> > +static struct usb_device_id id_table[] = {
> > {USB_DEVICE(0x05c6, 0x9211)}, /* Acer Gobi QDL device */
> > {USB_DEVICE(0x05c6, 0x9212)}, /* Acer Gobi Modem Device */
> > {USB_DEVICE(0x03f0, 0x1f1d)}, /* HP un2400 Gobi Modem Device */

Upstream linux-2.6 has the version we now have with the split out code.

> > tty = tty_port_tty_get(&port->port);
> > if (urb->actual_length) {
> > - tty_buffer_request_room(tty, urb->actual_length);
>
> Should this really have been removed?
>
> > @@ -224,9 +224,9 @@
> > tty_kref_put(tty);
> >
> > /* Resubmit urb so we continue receiving */
> > - if (port->port.count && status != -ESHUTDOWN) {
> > + if (status != -ESHUTDOWN) {
> > err = usb_submit_urb(urb, GFP_ATOMIC);
> > - if (err)
> > + if (err && err != -EPERM)
>
> What's correct here?

For both of these, comparing what was merged upstream and what was in
the 2.6.32 original patch from Matthew's website, the split out version
from git is what was merged upstream. I'd like to ask Matthew for
further details if that's ok (again, I'm not a kernel hacker, just
trying to get this device working in Squeeze). I'm assuming it would
be safer and easier to merge what went into upstream git.

> > printk(KERN_ERR "%s: resubmit read urb failed. "
> > "(%d)", __func__, err);
> > else
> > @@ -610,7 +610,7 @@
> > for (i = 0; i < serial->num_ports; i++) {
> > port = serial->port[i];
> > if (!port->interrupt_in_urb) {
> > - dbg("%s: No interrupt URB for port %d
", __func__, i);
> > + dbg("%s: No interrupt URB for port %d", __func__, i);
>
> The newline should definitely be included, though this is not really
> important.

It was merged upstream without the newline.

> > diff -u source/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
> > --- source/drivers/usb/serial/usb-wwan.h 2010-06-11 21:50:52.103759088 +0100
> > +++ b/drivers/usb/serial/usb-wwan.h
> > @@ -39,6 +39,7 @@
> > unsigned int suspended:1;
> > int in_flight;
> > int (*send_setup) (struct usb_serial_port *port);
> > + void *private;
> > };
> >
> > struct usb_wwan_port_private {

The private pointer is in the upstream merged patch, but not the
original patch against 2.6.32.


--
Mark Hymers <mhy at debian dot org>

"++?????++ Out of Cheese Error. Redo From Start."
Interesting Times, Terry Pratchett



--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100615075436.GA24376@hymers.org.uk">http://lists.debian.org/20100615075436.GA24376@hymers.org.uk

Mark Hymers <mhy@debian.org> writes:
> On Tue, 15, Jun, 2010 at 01:44:45AM +0100, Ben Hutchings spoke thus..
>
>> > @@ -610,7 +610,7 @@
>> > for (i = 0; i < serial->num_ports; i++) {
>> > port = serial->port[i];
>> > if (!port->interrupt_in_urb) {
>> > - dbg("%s: No interrupt URB for port %d
", __func__, i);
>> > + dbg("%s: No interrupt URB for port %d", __func__, i);
>>
>> The newline should definitely be included, though this is not really
>> important.
>
> It was merged upstream without the newline.

Probably because include/linux/usb/serial.h has this:

/* Use our own dbg macro */
#undef dbg
#define dbg(format, arg...)
do {
if (debug)
printk(KERN_DEBUG "%s: " format "
", __FILE__, ##arg);
} while (0)



Bjørn


--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 87typ464ue.fsf@nemi.mork.no">http://lists.debian.org/87typ464ue.fsf@nemi.mork.no

On Tue, 2010-06-15 at 08:54 +0100, Mark Hymers wrote:
> On Tue, 15, Jun, 2010 at 01:44:45AM +0100, Ben Hutchings spoke thus..
[...]
> > > tty = tty_port_tty_get(&port->port);
> > > if (urb->actual_length) {
> > > - tty_buffer_request_room(tty, urb->actual_length);
> >
> > Should this really have been removed?

Answer: yes, it is pointless to call this function without checking the
result, and tty_insert_flip_string() calls it properly.

> > > @@ -224,9 +224,9 @@
> > > tty_kref_put(tty);
> > >
> > > /* Resubmit urb so we continue receiving */
> > > - if (port->port.count && status != -ESHUTDOWN) {
> > > + if (status != -ESHUTDOWN) {
> > > err = usb_submit_urb(urb, GFP_ATOMIC);
> > > - if (err)
> > > + if (err && err != -EPERM)
> >
> > What's correct here?
>
> For both of these, comparing what was merged upstream and what was in
> the 2.6.32 original patch from Matthew's website, the split out version
> from git is what was merged upstream.
[...]

I understood that, but wanted to check that there was not a semantic
difference between 2.6.32 and .35 that made the former correct.
However, now that I've looked more closely I think this change is valid
for both versions.

Therefore, I'm applying the changes you sent.

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

On Tue, 15, Jun, 2010 at 01:27:19PM +0100, Ben Hutchings spoke thus..
> I understood that, but wanted to check that there was not a semantic
> difference between 2.6.32 and .35 that made the former correct.
> However, now that I've looked more closely I think this change is valid
> for both versions.
>
> Therefore, I'm applying the changes you sent.

I spoke to Matthew and he confirmed that those changes should be fine to
merge too. Thanks for your work on this and sorry for the confusion.

Mark


--
Mark Hymers <mhy at debian dot org>

"The first thing we do, let's kill all the Lawyers"
Henry VI Part II, Shakespeare



--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100615150212.GA13469@hymers.org.uk">http://lists.debian.org/20100615150212.GA13469@hymers.org.uk