[PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Brad Sawatzky]

Fixes a bug/inconsistency revealed by the additional sanity checking in
   commit 063a2da8f01806906f7d7b1a1424b9afddebc443
introduced in the original 2.6.24 branch.

The Handspring Visor / PalmOS 4 device structure defines .num_bulk_out=2
but the usb-serial probe returns num_bulk_out=3, triggering the check in
the above commit and forcing a bail out when the device (a Garmin iQue in
my case) attempts to connect.  The patch bumps the expected number of
endpoints to 3.

I suppose it's possible that the kernel is identifying 3 bulk endpoints
when there should only be 2 and there is some issue with the lower level
endpoint probe?

FWIW, this patch will probably solve the following kernel bug report for
Treo users (identical symptoms, different model PalmOS units):
  <http://bugzilla.kernel.org/show_bug.cgi?id=10118>


Signed-off-by: Brad Sawatzky <brad+kernel@swatter.net>
---

--- drivers/usb/serial/visor.c.orig	2008-03-25 21:32:40.000000000 -0400
+++ drivers/usb/serial/visor.c	2008-03-25 21:29:57.000000000 -0400
@@ -191,7 +191,7 @@ static struct usb_serial_driver handspri
 	.id_table =		id_table,
 	.num_interrupt_in =	NUM_DONT_CARE,
 	.num_bulk_in =		2,
-	.num_bulk_out =		2,
+	.num_bulk_out =		3,
 	.num_ports =		2,
 	.open =			visor_open,
 	.close =		visor_close,

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Oliver Neukum]

Am Mittwoch, 26. März 2008 03:32:43 schrieb Brad Sawatzky:
> Fixes a bug/inconsistency revealed by the additional sanity checking in
>    commit 063a2da8f01806906f7d7b1a1424b9afddebc443
> introduced in the original 2.6.24 branch.
> 
> The Handspring Visor / PalmOS 4 device structure defines .num_bulk_out=2
> but the usb-serial probe returns num_bulk_out=3, triggering the check in
> the above commit and forcing a bail out when the device (a Garmin iQue in
> my case) attempts to connect.  The patch bumps the expected number of
> endpoints to 3.
> 
> I suppose it's possible that the kernel is identifying 3 bulk endpoints
> when there should only be 2 and there is some issue with the lower level
> endpoint probe?

Send in "lsusb -v". Are you sure all devices have 3 endpoints?

	Regards
		Oliver

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Brad Sawatzky]

On Wed, 26 Mar 2008, Oliver Neukum wrote:

> Am Mittwoch, 26. März 2008 03:32:43 schrieb Brad Sawatzky:
[ . . . ]
> > I suppose it's possible that the kernel is identifying 3 bulk endpoints
> > when there should only be 2 and there is some issue with the lower level
> > endpoint probe?
> 
> Send in "lsusb -v". Are you sure all devices have 3 endpoints?

Seems to:

% sudo lsusb -v
Bus 002 Device 016: ID 091e:0004 Garmin International 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x091e Garmin International
  idProduct          0x0004 
  bcdDevice            1.00
  iManufacturer           1 Palm, Inc.
  iProduct                2 Palm Handheld
  iSerial                 5 PalmSN12345678
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           53
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0xc0
      Self Powered
    MaxPower                2mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           5
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol      0 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x03  EP 3 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0020  1x 32 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0020  1x 32 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x05  EP 5 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0020  1x 32 bytes
        bInterval               0
Device Status:     0x0001
  Self Powered

[ Trimmed out other devices ]

-- Brad

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Alan Stern]

On Wed, 26 Mar 2008, Brad Sawatzky wrote:

> On Wed, 26 Mar 2008, Oliver Neukum wrote:
> 
> > Am Mittwoch, 26. März 2008 03:32:43 schrieb Brad Sawatzky:
> [ . . . ]
> > > I suppose it's possible that the kernel is identifying 3 bulk endpoints
> > > when there should only be 2 and there is some issue with the lower level
> > > endpoint probe?
> > 
> > Send in "lsusb -v". Are you sure all devices have 3 endpoints?
> 
> Seems to:

You did not understand Oliver's question.  Yes, your device has 3
bulk-OUT endpoints (and 2 bulk-IN).  But do you know whether _all_
Visor/Palm OS devices do?  If they don't, your patch will cause the
driver to stop working when someone plugs in a device with only 2
endpoints.

Alan Stern

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Brad Sawatzky]

On Wed, 26 Mar 2008, Alan Stern wrote:

> On Wed, 26 Mar 2008, Brad Sawatzky wrote:
> 
> > On Wed, 26 Mar 2008, Oliver Neukum wrote:
> > 
> > > Am Mittwoch, 26. März 2008 03:32:43 schrieb Brad Sawatzky:
> > [ . . . ]
> > > > I suppose it's possible that the kernel is identifying 3 bulk endpoints
> > > > when there should only be 2 and there is some issue with the lower level
> > > > endpoint probe?
> > > 
> > > Send in "lsusb -v". Are you sure all devices have 3 endpoints?
[ . . . ]
> You did not understand Oliver's question.  Yes, your device has 3
> bulk-OUT endpoints (and 2 bulk-IN).  But do you know whether _all_
> Visor/Palm OS devices do?  If they don't, your patch will cause the
> driver to stop working when someone plugs in a device with only 2
> endpoints.

You're absolutely right -- a very legitimate point.

I only have one PalmOS USB device and can only provide data for that unit.
As mentioned in the initial post I think it is very suggestive that there is
a kernel bug report saying that both a Treo90 and a Treo750p that exhibited
identical symptoms.  I'd bet a beer that that the kernel also reports 3
bulk out endpoints for those devices, but I can't prove it.

I'm more concerned that the kernel is creating a bogus 3rd endpoint (for
all devices assigned to the so-called "handspring_device") when it should
not.  Any suggestions on how to check that with my particular unit?

I was hoping that whomever authored the original code (Greg?) might know
where the original num_bulk_out=2 value came from: reverse engineering,
best guess, or honest-to-god documentation from Palm?

FWIW, another option I considered was patching the test added in commit
063a2da8f01806906f7d7b1a1424b9afddebc443 to use '<=' instead of '!=' in the
obvious places.  That is, test if the usb-serial subsystem reports at least
as many endpoints as the device definition "requires" rather than checking
for an exact number.  That behaviour is closer to the pre-2.6.24 code (ie.
no check at all) and might be a better fix (provided this 3rd bulk-out
endpoint I'm seeing is 'real' and not due to a lower-level bug).

-- Brad

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Charles Banas]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Brad Sawatzky wrote:
| I only have one PalmOS USB device and can only provide data for that unit.
| As mentioned in the initial post I think it is very suggestive that
there is
| a kernel bug report saying that both a Treo90 and a Treo750p that
exhibited
| identical symptoms.  I'd bet a beer that that the kernel also reports 3
| bulk out endpoints for those devices, but I can't prove it.
|

Attached is the lspci -v output for my Treo 90. I see two bulk out
endpoints.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH60Gxi1yS1BuzIvgRAuh7AKCR/JXrulXICE33R8pXKvie8VZIbwCeOZa1
zjFCIEC3DaOTHPplI0iWAfg=
=c6GN
-----END PGP SIGNATURE-----

[-- Attachment #2: treo90 --]
[-- Type: text/plain, Size: 2691 bytes --]


Bus 002 Device 003: ID 082d:0200 Handspring 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x082d Handspring
  idProduct          0x0200 
  bcdDevice            1.00
  iManufacturer           0 
  iProduct                0 
  iSerial                 0 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           46
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower                2mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol      0 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x03  EP 3 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
Device Status:     0x0001
  Self Powered

[-- Attachment #3: treo90.sig --]
[-- Type: application/octet-stream, Size: 65 bytes --]

Re: [PATCH] usb-serial: fix regression in Visor/Palm OS module for kernels >= 2.6.24

[Oliver Neukum]

Am Donnerstag, 27. März 2008 06:07:26 schrieb Brad Sawatzky:
> FWIW, another option I considered was patching the test added in commit
> 063a2da8f01806906f7d7b1a1424b9afddebc443 to use '<=' instead of '!=' in the
> obvious places.  That is, test if the usb-serial subsystem reports at least
> as many endpoints as the device definition "requires" rather than checking

The problem with that is that if a device adds an endpoint we may use
the wrong endpoints in drivers, depending on how the endpoints in devices
are numbered. As we have no idea what the consequences would be, we rather
fail cleanly.

	Regards
		Oliver