Hi,
cdc-acm was broken since after 2.6.4, due to the alt_cursetting changes. I sent a patch, which has been integrated (well, the same one has ;-)) not long ago.
I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a new way:
when plugging the phone, acm_probe() fails on interface #0; I traced the problem to this: usb_interface_claimed() returns true - and in fact intf->dev.driver is already cdc-acm (despite the fact that this is the first call to acm_probe() !), for reasons beyond my comprehension.
But, even if the interface is claimed, the intfdata hasn't been set, which allows to do another check: the attached patch fixes this bug.
HTH,
--
Colin
Am Donnerstag, 15. April 2004 20:11 schrieb Colin Leroy:
> Hi,
>
> cdc-acm was broken since after 2.6.4, due to the alt_cursetting changes. I
> sent a patch, which has been integrated (well, the same one has ;-)) not
> long ago. I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a
> new way: when plugging the phone, acm_probe() fails on interface #0; I
> traced the problem to this: usb_interface_claimed() returns true - and in
> fact intf->dev.driver is already cdc-acm (despite the fact that this is the
> first call to acm_probe() !), for reasons beyond my comprehension.
>
> But, even if the interface is claimed, the intfdata hasn't been set, which
> allows to do another check: the attached patch fixes this bug.
But somebody else may have claimed the interface. You can't simply
assume that only cdc-acm will take the device.
Regards
Oliver
Colin Leroy wrote:
> Hi,
>
> I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a new way:
> ... usb_interface_claimed() returns true ... intf->dev.driver is already cdc-acm
The interface being probed is by definition not going to be claimed
by any other driver ... it shouldn't check or claim that interface.
That test has always been buggy -- better to just remove it. For
that matter, usb_interface_claimed() calls should all vanish ... it's
better to fail if claiming the interface fails (one step, not two).
Care to try an updated patch?
This started to matter because as of RC1, usbcore got rid of the last of
some pre-driver-model code for driver binding. There might be a similar
bug in the ALSA usb audio driver, according to 'grep'.
- Dave
>
> HTH,
On 15 Apr 2004 at 11h04, David Brownell wrote:
Hi David,
> That test has always been buggy -- better to just remove it. For
> that matter, usb_interface_claimed() calls should all vanish ... it's
> better to fail if claiming the interface fails (one step, not two).
> Care to try an updated patch?
Like this one? It works. I'm a bit wondering, however, how comes
usb_interface_claimed() returns true, and the check in
usb_driver_claim_interface() passes?
--
Colin
Hi Colin,
>>That test has always been buggy -- better to just remove it. For
>>that matter, usb_interface_claimed() calls should all vanish ... it's
>>better to fail if claiming the interface fails (one step, not two).
>>Care to try an updated patch?
>
>
> Like this one? It works. I'm a bit wondering, however, how comes
> usb_interface_claimed() returns true, and the check in
> usb_driver_claim_interface() passes?
Pretty much like that one, but not leaking the other urbs ... :)
There are two interfaces involved, for "control" and "data".
"Control" is being probed; and "data" is what gets claimed.
For more info, you could see how "usbnet" handles CDC Ethernet;
see how it parses the CDC Union descriptor (which is what the
FIXME refers to). Or read the CDC spec, from http://www.usb.org as PDF.
- Dave
On Thu, 15 Apr 2004, Colin Leroy wrote:
> Like this one? It works. I'm a bit wondering, however, how comes
> usb_interface_claimed() returns true, and the check in
> usb_driver_claim_interface() passes?
While an interface is being probed, usb_interface_claimed() will always
return true for it. That's because the probing code sets
interface->dev.driver (which is what usb_interface_claimed() tests) before
calling the probe function. If the probe fails then interface->dev.driver
will be reset. This all happens inside bus_match() in drivers/base/bus.c.
For the same reason, the test of (dev->driver) in
usb_driver_claim_interface() -- that is the test you meant, isn't it? --
passes (i.e., yields a true value).
Alan Stern
On 15 Apr 2004 at 12h04, David Brownell wrote:
Hi,
> Pretty much like that one, but not leaking the other urbs ... :)
The joys of copy/pasting :)
> There are two interfaces involved, for "control" and "data".
> "Control" is being probed; and "data" is what gets claimed.
Yup, I realized that after sending the mail.
Here's another patch, which fixes the leak. It also fixes the FIXME, by looking at all interfaces to find the data one. Is it correct ?
(indentation is ugly in this part, I'll send a cosmetic patch if this one is accepted)...
--
Colin
Hi Colin,
> Here's another patch, which fixes the leak. It also fixes the
> FIXME, by looking at all interfaces to find the data one. Is it correct ?
It's looking better, but what I'd rather see is code scanning the
CDC descriptors (see "usbnet"). The deal is that there's actually
no guarantee that there's only one data interface, although that
seems to hold true for many current products.
But unless you're interested in finishing a much-needed rewrite
of that cdc-acm probe() code, this might be a good place to stop.
(Or at least pause!)
- Dave
On 16 Apr 2004 at 11h04, David Brownell wrote:
Hi,
> It's looking better, but what I'd rather see is code scanning the
> CDC descriptors (see "usbnet"). The deal is that there's actually
> no guarantee that there's only one data interface, although that
> seems to hold true for many current products.
Ok... However I don't think i'll find the time to do that right now.
So I think I'll take your advice that it's a good place to pause :)
> But unless you're interested in finishing a much-needed rewrite
> of that cdc-acm probe() code, this might be a good place to stop.
> (Or at least pause!)
Ok, here is the cosmetic patch following the previous one, then :)
HTH,
--
Colin
On 16 Apr 2004 at 23h04, Colin Leroy wrote:
Hi,
> Ok, here is the cosmetic patch following the previous one, then :)
Uhm, previous patch is broken.
Here are the two again.
--
Colin