2008-03-26 03:10:40

by Brad Sawatzky

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

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 <[email protected]>
---

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


2008-03-26 07:17:29

by Oliver Neukum

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

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

2008-03-27 00:16:27

by Brad Sawatzky

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

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

2008-03-27 02:55:27

by Alan Stern

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

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

2008-03-27 05:07:39

by Brad Sawatzky

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

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

2008-03-27 06:43:36

by Charles Banas

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


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


Attachments:
treo90 (2.63 kB)
treo90.sig (65.00 B)
Download all attachments

2008-03-27 08:56:44

by Oliver Neukum

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

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