On Tue, Dec 30, 2008 at 8:09 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 30 Dec 2008, Erik Ekman wrote:
>
>> > Okay, this is starting to narrow things down. Here's a kernel patch to
>> > add some extra debugging information to the log. Run the same test as
>> > before (you don't need to set usbfs_snoop this time) and post the
>> > resulting dmesg log.
>> >
>> > Alan Stern
>> >
>>
>> Here is the resulting dmesg of tf-tool --acquire:
>
> The initial stuff is all normal.
>
> ...
>> findintfep: 0
>> checkintf: 0
>> ep addr: 129: ptr: f6526f40
>
> Through here.
>
>> findintfep: 0
>> checkintf: 0
>> ep addr: 129: ptr: 00000000
>> findintfep: 0
>> checkintf: 0
>> ep addr: 129: ptr: 00000000
>> findintfep: 0
>> checkintf: 0
>> ep addr: 129: ptr: 00000000
>> findintfep: 0
>> checkintf: 0
>> ep addr: 2: ptr: 00000000
>
> This part indicates where the trouble starts. And I believe I see the
> cause of the problem; the interface gets disabled but never re-enabled.
> You can remove the earlier patch and try the patch below; it should fix
> this problem.
>
> Timo, pay attention to this: There may still be another problem lurking
> in the library source code. _libthinkfinger_usb_hello() behaves very
> badly: It sends a direct Set-Config request without using
> usb_set_configuration(). You should remove the entire first call to
> usb_control_msg() from that function. I know the comment says "should
> not be relevant", but the comment is wrong.
>
> If it turns out that the Set-Config really is needed (it might indeed
> be necessary for Windows systems), it shouldn't be done this way.
> Instead, usb_set_configuration() should be called from within
> _libthinkfinger_usb_init(), just before usb_claim_interface().
>
> Alan Stern
>
>
>
> Index: 2.6.28/drivers/usb/core/usb.h
> ===================================================================
> --- 2.6.28.orig/drivers/usb/core/usb.h
> +++ 2.6.28/drivers/usb/core/usb.h
> @@ -10,7 +10,9 @@ extern int usb_create_ep_files(struct de
> extern void usb_remove_ep_files(struct usb_host_endpoint *endpoint);
>
> extern void usb_enable_endpoint(struct usb_device *dev,
> - struct usb_host_endpoint *ep);
> + struct usb_host_endpoint *ep, bool reset_toggle);
> +extern void usb_enable_interface(struct usb_device *dev,
> + struct usb_interface *intf, bool reset_toggles);
> extern void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr);
> extern void usb_disable_interface(struct usb_device *dev,
> struct usb_interface *intf);
> Index: 2.6.28/drivers/usb/core/driver.c
> ===================================================================
> --- 2.6.28.orig/drivers/usb/core/driver.c
> +++ 2.6.28/drivers/usb/core/driver.c
> @@ -279,9 +279,12 @@ static int usb_unbind_interface(struct d
> * altsetting means creating new endpoint device entries).
> * When either of these happens, defer the Set-Interface.
> */
> - if (intf->cur_altsetting->desc.bAlternateSetting == 0)
> - ; /* Already in altsetting 0 so skip Set-Interface */
> - else if (!error && intf->dev.power.status == DPM_ON)
> + if (intf->cur_altsetting->desc.bAlternateSetting == 0) {
> + /* Already in altsetting 0 so skip Set-Interface.
> + * Just re-enable it without affecting the endpoint toggles.
> + */
> + usb_enable_interface(udev, intf, false);
> + } else if (!error && intf->dev.power.status == DPM_ON)
> usb_set_interface(udev, intf->altsetting[0].
> desc.bInterfaceNumber, 0);
> else
> Index: 2.6.28/drivers/usb/core/message.c
> ===================================================================
> --- 2.6.28.orig/drivers/usb/core/message.c
> +++ 2.6.28/drivers/usb/core/message.c
> @@ -1113,22 +1113,26 @@ void usb_disable_device(struct usb_devic
> * usb_enable_endpoint - Enable an endpoint for USB communications
> * @dev: the device whose interface is being enabled
> * @ep: the endpoint
> + * @reset_toggle: flag to set the endpoint's toggle back to 0
> *
> - * Resets the endpoint toggle, and sets dev->ep_{in,out} pointers.
> + * Resets the endpoint toggle if asked, and sets dev->ep_{in,out} pointers.
> * For control endpoints, both the input and output sides are handled.
> */
> -void usb_enable_endpoint(struct usb_device *dev, struct usb_host_endpoint *ep)
> +void usb_enable_endpoint(struct usb_device *dev, struct usb_host_endpoint *ep,
> + bool reset_toggle)
> {
> int epnum = usb_endpoint_num(&ep->desc);
> int is_out = usb_endpoint_dir_out(&ep->desc);
> int is_control = usb_endpoint_xfer_control(&ep->desc);
>
> if (is_out || is_control) {
> - usb_settoggle(dev, epnum, 1, 0);
> + if (reset_toggle)
> + usb_settoggle(dev, epnum, 1, 0);
> dev->ep_out[epnum] = ep;
> }
> if (!is_out || is_control) {
> - usb_settoggle(dev, epnum, 0, 0);
> + if (reset_toggle)
> + usb_settoggle(dev, epnum, 0, 0);
> dev->ep_in[epnum] = ep;
> }
> ep->enabled = 1;
> @@ -1138,17 +1142,18 @@ void usb_enable_endpoint(struct usb_devi
> * usb_enable_interface - Enable all the endpoints for an interface
> * @dev: the device whose interface is being enabled
> * @intf: pointer to the interface descriptor
> + * @reset_toggles: flag to set the endpoints' toggles back to 0
> *
> * Enables all the endpoints for the interface's current altsetting.
> */
> -static void usb_enable_interface(struct usb_device *dev,
> - struct usb_interface *intf)
> +void usb_enable_interface(struct usb_device *dev,
> + struct usb_interface *intf, bool reset_toggles)
> {
> struct usb_host_interface *alt = intf->cur_altsetting;
> int i;
>
> for (i = 0; i < alt->desc.bNumEndpoints; ++i)
> - usb_enable_endpoint(dev, &alt->endpoint[i]);
> + usb_enable_endpoint(dev, &alt->endpoint[i], reset_toggles);
> }
>
> /**
> @@ -1271,7 +1276,7 @@ int usb_set_interface(struct usb_device
> * during the SETUP stage - hence EP0 toggles are "don't care" here.
> * (Likewise, EP0 never "halts" on well designed devices.)
> */
> - usb_enable_interface(dev, iface);
> + usb_enable_interface(dev, iface, true);
> if (device_is_registered(&iface->dev))
> usb_create_sysfs_intf_files(iface);
>
> @@ -1346,7 +1351,7 @@ int usb_reset_configuration(struct usb_d
> alt = &intf->altsetting[0];
>
> intf->cur_altsetting = alt;
> - usb_enable_interface(dev, intf);
> + usb_enable_interface(dev, intf, true);
> if (device_is_registered(&intf->dev))
> usb_create_sysfs_intf_files(intf);
> }
> @@ -1604,7 +1609,7 @@ free_interfaces:
> alt = &intf->altsetting[0];
>
> intf->cur_altsetting = alt;
> - usb_enable_interface(dev, intf);
> + usb_enable_interface(dev, intf, true);
> intf->dev.parent = &dev->dev;
> intf->dev.driver = NULL;
> intf->dev.bus = &usb_bus_type;
> Index: 2.6.28/drivers/usb/core/usb.c
> ===================================================================
> --- 2.6.28.orig/drivers/usb/core/usb.c
> +++ 2.6.28/drivers/usb/core/usb.c
> @@ -362,7 +362,7 @@ struct usb_device *usb_alloc_dev(struct
> dev->ep0.desc.bLength = USB_DT_ENDPOINT_SIZE;
> dev->ep0.desc.bDescriptorType = USB_DT_ENDPOINT;
> /* ep0 maxpacket comes later, from device descriptor */
> - usb_enable_endpoint(dev, &dev->ep0);
> + usb_enable_endpoint(dev, &dev->ep0, true);
> dev->can_submit = 1;
>
> /* Save readable and stable topology id, distinguishing devices
> Index: 2.6.28/drivers/usb/core/hub.c
> ===================================================================
> --- 2.6.28.orig/drivers/usb/core/hub.c
> +++ 2.6.28/drivers/usb/core/hub.c
> @@ -2385,7 +2385,7 @@ void usb_ep0_reinit(struct usb_device *u
> {
> usb_disable_endpoint(udev, 0 + USB_DIR_IN);
> usb_disable_endpoint(udev, 0 + USB_DIR_OUT);
> - usb_enable_endpoint(udev, &udev->ep0);
> + usb_enable_endpoint(udev, &udev->ep0, true);
> }
> EXPORT_SYMBOL_GPL(usb_ep0_reinit);
>
>
>
Works for me, both with tf-tool --acquire and pam_fprint_enroll, both
which scan 3 swipes.
/Erik
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
