2011-11-15 04:53:17

by Yao, Costa

[permalink] [raw]
Subject: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
2 Delete desc.bInterfaceNumber != 0 check.

Signed-off-by: Costa Yao <[email protected]>
---
drivers/bluetooth/btusb.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2bd87d4..fbac911 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -57,8 +57,8 @@ static struct usb_driver btusb_driver;
#define BTUSB_ATH3012 0x80

static struct usb_device_id btusb_table[] = {
- /* Generic Bluetooth USB device */
- { USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
+ /* Bluetooth USB interface */
+ { USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },

/* Broadcom SoftSailing reporting vendor specific */
{ USB_DEVICE(0x05ac, 0x21e1) },
@@ -918,10 +918,6 @@ static int btusb_probe(struct usb_interface *intf,

BT_DBG("intf %p id %p", intf, id);

- /* interface numbers are hardcoded in the spec */
- if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
- return -ENODEV;
-
if (!id->driver_info) {
const struct usb_device_id *match;
match = usb_match_id(intf, blacklist_table);
@@ -1017,8 +1013,12 @@ static int btusb_probe(struct usb_interface *intf,

hdev->owner = THIS_MODULE;

- /* Interface numbers are hardcoded in the specification */
- data->isoc = usb_ifnum_to_if(data->udev, 1);
+ /* According to HCI-USB specification, the interface for
+ * SCO data endpoint follows the interface for commands,
+ * events and ACL data
+ */
+ data->isoc = usb_ifnum_to_if(data->udev,
+ intf->cur_altsetting->desc.bInterfaceNumber + 1);

if (!reset)
set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
--
1.7.4.1


2011-11-16 20:24:40

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

* Yao, Costa <[email protected]> [2011-11-16 02:14:45 +0000]:

> 1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
> 2 Delete desc.bInterfaceNumber != 0 check.
>
> Signed-off-by: Costa Yao <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 2bd87d4..39dd65c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -57,8 +57,8 @@ static struct usb_driver btusb_driver;
> #define BTUSB_ATH3012 0x80
>
> static struct usb_device_id btusb_table[] = {
> - /* Generic Bluetooth USB device */
> - { USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
> + /* Bluetooth USB interface */
> + { USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },
>
> /* Broadcom SoftSailing reporting vendor specific */
> { USB_DEVICE(0x05ac, 0x21e1) },
> @@ -918,10 +918,6 @@ static int btusb_probe(struct usb_interface *intf,
>
> BT_DBG("intf %p id %p", intf, id);
>
> - /* interface numbers are hardcoded in the spec */
> - if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
> - return -ENODEV;
> -
> if (!id->driver_info) {
> const struct usb_device_id *match;
> match = usb_match_id(intf, blacklist_table);
> @@ -1017,8 +1013,12 @@ static int btusb_probe(struct usb_interface *intf,
>
> hdev->owner = THIS_MODULE;
>
> - /* Interface numbers are hardcoded in the specification */
> - data->isoc = usb_ifnum_to_if(data->udev, 1);
> + /* According to HCI-USB specification, the interface for
> + * SCO data endpoints follows the interface for commands,
> + * events and ACL data
> + */
> + data->isoc = usb_ifnum_to_if(data->udev,
> + intf->cur_altsetting->desc.bInterfaceNumber + 1);
>
> if (!reset)
> set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);

Patch doesn't apply, please rebase and resend, fixing the trailing whitespace
issues. Also add the Acked-by message to your patch.


Applying: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching
/home/padovan/p/linux-trees/bluetooth-next/.git/rebase-apply/patch:15: trailing whitespace.
/* Bluetooth USB interface */
/home/padovan/p/linux-trees/bluetooth-next/.git/rebase-apply/patch:16: trailing whitespace.
{ USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },
/home/padovan/p/linux-trees/bluetooth-next/.git/rebase-apply/patch:37: trailing whitespace.
/* According to HCI-USB specification, the interface for
/home/padovan/p/linux-trees/bluetooth-next/.git/rebase-apply/patch:38: trailing whitespace.
* SCO data endpoints follows the interface for commands,
/home/padovan/p/linux-trees/bluetooth-next/.git/rebase-apply/patch:39: trailing whitespace.
* events and ACL data
error: patch failed: drivers/bluetooth/btusb.c:57
error: drivers/bluetooth/btusb.c: patch does not apply
Patch failed at 0001 RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Gustavo

2011-11-16 09:14:31

by Marcel Holtmann

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

> I have gone through commits about the adding of USB_DEVICE entry, and found as follow:
>
> For Broadcom SoftSailing reporting vendor specific entry, no detailed commit description.

I think that this got posted to the mailing list at least.

> For Apple MacBookPro7,1 entry, its interface class is 0xff, so cannot be removed.
>
> For Apple iMac11,1 entry, its interface class is 0xff, so cannot be removed.
>
> For Apple MacBookPro6,2 entry, its interface class is 0xff, so cannot be removed.
>
> For Apple MacBookAir3,1, MacBookAir3,2 entry, its interface class is 0xff, so cannot be removed.
>
> For Apple MacBookAir4,1 entry, no detailed commit description.
>
> For Apple MacBookPro8,2 entry, no detailed commit description.
>
> For Apple MacMini5,1, no detailed commit description.

for all the MacBook its seems we can use USB_DEVICE_AND_INTERFACE_INFO
with 0xff, 0x01, 0x01. So at least we would just match the first
interface and not all interfaces of these chips.

T: Bus=03 Lev=02 Prnt=03 Port=02 Cnt=01 Dev#= 6 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=05ac ProdID=821b Rev= 0.34
S: Manufacturer=Apple Inc.
S: Product=Bluetooth USB Host Controller
C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr= 0mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
I: If#= 1 Alt= 1 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
I: If#= 1 Alt= 2 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
I: If#= 1 Alt= 3 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 32 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 32 Ivl=1ms
I: If#= 1 Alt= 4 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 64 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 64 Ivl=1ms
I: If#= 1 Alt= 5 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 64 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 64 Ivl=1ms
I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=84(I) Atr=02(Bulk) MxPS= 32 Ivl=0ms
E: Ad=04(O) Atr=02(Bulk) MxPS= 32 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)

Especially since the SCO endpoints have a proper 0xe0, 0x01, 0x01
descriptor our check here becomes simpler and can be made generic.

> For Ericsson entry, no commit description.
>
> For Canyon CN-BTU1 entry, no detailed commit description.

These two are really just old and messed up devices. We have no chance
to do anything here.

> For BCM20702A0 entry, its interface class is 0xff, so it cannot be removed.

Maybe it is similar as the Apple dongles. It uses 0xff, 0x01, 0x01 and
then we just should USB_DEVICE_AND_INTERFACE_INFO as well here.

> For "AVM BlueFRITZ", "Bluetooth Ulttraport Module from IBM", and "ALPS Modules with non-standard id" entries, no related commit description.

I do have the BlueFritz at home and can check up on it, but if we trust
the Internet, then it is like this:

I: If#= 0 Alt= 0 #EPs= 1 Cls=09(hub ) Sub=00 Prot=00 Driver=hub
E: Ad=81(I) Atr=03(Int.) MxPS= 8 Ivl=255ms
T: Bus=03 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 2 Spd=12 MxCh= 0
D: Ver= 1.10 Cls=ff(vend.) Sub=ff Prot=ff MxPS=64 #Cfgs= 1
P: Vendor=057c ProdID=3800 Rev=15.00
S: Manufacturer=Bluetooth Device
S: Product=Bluetooth Device
S: SerialNumber=2C1F880***
C:* #Ifs= 2 Cfg#= 1 Atr=a0 MxPwr=200mA
I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms

So an USB_DEVICE_AND_INTERFACE_INFO with 0xff, 0xff, 0xff would be fine.

And for the old Ultraport and ALPS, we most likely will not easily find
these devices.

Regards

Marcel



2011-11-16 08:29:32

by Yao, Costa

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

SGkgTWFyY2VsLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IE1hcmNl
bCBIb2x0bWFubiBbbWFpbHRvOm1hcmNlbEBob2x0bWFubi5vcmddDQo+IFNlbnQ6IDIwMTHlubQx
MeaciDE05pelIDE3OjMyDQo+IFRvOiBZYW8sIENvc3RhDQo+IENjOiBwYWRvdmFuQHByb2Z1c2lv
bi5tb2JpOyBsaW51eC1ibHVldG9vdGhAdmdlci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBb
UEFUQ0hdIEJsdWV0b290aDogYnR1c2I6IFVzZSBVU0JfSU5URVJGQUNFX0lORk8gdG8gZG8gZGV2
aWNlDQo+IG1hdGNoaW5nDQo+IA0KPiBIaSBDb3N0YSwNCj4gDQo+ID4gMSBVc2UgVVNCX0lOVEVS
RkFDRV9JTkZPIHRvIGRvIGRldmljZSBtYXRjaGluZywgb3RoZXIgdGhhbg0KPiBVU0JfREVWSUNF
X0lORk8uDQo+ID4gMiBBbmQgd2UgdHJ5IHRvIGZpbmQgdGhlIGZpcnN0IGludGVyZmFjZSBjb250
YWluaW5nIHRoZSBpbnRlcnJ1cHQgZW5kcG9pbnQuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBD
b3N0YSBZYW8gPGNxeWFvQHFjYS5xdWFsY29tbS5jb20+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMv
Ymx1ZXRvb3RoL2J0dXNiLmMgfCAgIDE2ICsrKysrKysrKysrLS0tLS0NCj4gPiAgMSBmaWxlcyBj
aGFuZ2VkLCAxMSBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAt
LWdpdCBhL2RyaXZlcnMvYmx1ZXRvb3RoL2J0dXNiLmMgYi9kcml2ZXJzL2JsdWV0b290aC9idHVz
Yi5jDQo+ID4gaW5kZXggOWRiMjQ3Ni4uODBkN2UyZCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJz
L2JsdWV0b290aC9idHVzYi5jDQo+ID4gKysrIGIvZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYw0K
PiA+IEBAIC01OCw3ICs1OCw3IEBAIHN0YXRpYyBzdHJ1Y3QgdXNiX2RyaXZlciBidHVzYl9kcml2
ZXI7DQo+ID4NCj4gPiAgc3RhdGljIHN0cnVjdCB1c2JfZGV2aWNlX2lkIGJ0dXNiX3RhYmxlW10g
PSB7DQo+ID4gIAkvKiBHZW5lcmljIEJsdWV0b290aCBVU0IgZGV2aWNlICovDQo+ID4gLQl7IFVT
Ql9ERVZJQ0VfSU5GTygweGUwLCAweDAxLCAweDAxKSB9LA0KPiA+ICsJeyBVU0JfSU5URVJGQUNF
X0lORk8oMHhlMCwgMHgwMSwgMHgwMSkgfSwNCj4gPg0KPiA+ICAJLyogQnJvYWRjb20gU29mdFNh
aWxpbmcgcmVwb3J0aW5nIHZlbmRvciBzcGVjaWZpYyAqLw0KPiA+ICAJeyBVU0JfREVWSUNFKDB4
MDVhYywgMHgyMWUxKSB9LA0KPiANCj4gdGhpcyBhY3R1YWxseSBtZWFucyB0aGF0IHNvbWUgb2Yg
dGhlIFVTQl9ERVZJQ0UgZW50cmllcyBjYW4gYmUgcmVtb3ZlZC4NCj4gUGxlYXNlIGdvIHRocm91
Z2ggb3VyIGNvbW1pdCBsb2dzIGZvciBidHVzYi5jIGFuZCBmaWd1cmUgb3V0IHdoaWNoIG9uZXMg
Y2FuIGJlDQo+IHJlbW92ZWQgYW5kIHNlbmQgYSBzZXBhcmF0ZSBwYXRjaCBmb3IgdGhhdC4NCkkg
aGF2ZSBnb25lIHRocm91Z2ggY29tbWl0cyBhYm91dCB0aGUgYWRkaW5nIG9mIFVTQl9ERVZJQ0Ug
ZW50cnksIGFuZCBmb3VuZCBhcyBmb2xsb3c6DQoNCkZvciBCcm9hZGNvbSBTb2Z0U2FpbGluZyBy
ZXBvcnRpbmcgdmVuZG9yIHNwZWNpZmljIGVudHJ5LCBubyBkZXRhaWxlZCBjb21taXQgZGVzY3Jp
cHRpb24uDQoNCkZvciBBcHBsZSBNYWNCb29rUHJvNywxIGVudHJ5LCBpdHMgaW50ZXJmYWNlIGNs
YXNzIGlzIDB4ZmYsIHNvIGNhbm5vdCBiZSByZW1vdmVkLg0KDQpGb3IgQXBwbGUgaU1hYzExLDEg
ZW50cnksIGl0cyBpbnRlcmZhY2UgY2xhc3MgaXMgMHhmZiwgc28gY2Fubm90IGJlIHJlbW92ZWQu
DQoNCkZvciBBcHBsZSBNYWNCb29rUHJvNiwyIGVudHJ5LCBpdHMgaW50ZXJmYWNlIGNsYXNzIGlz
IDB4ZmYsIHNvIGNhbm5vdCBiZSByZW1vdmVkLg0KDQpGb3IgQXBwbGUgTWFjQm9va0FpcjMsMSwg
TWFjQm9va0FpcjMsMiBlbnRyeSwgaXRzIGludGVyZmFjZSBjbGFzcyBpcyAweGZmLCBzbyBjYW5u
b3QgYmUgcmVtb3ZlZC4NCg0KRm9yIEFwcGxlIE1hY0Jvb2tBaXI0LDEgZW50cnksIG5vIGRldGFp
bGVkIGNvbW1pdCBkZXNjcmlwdGlvbi4NCg0KRm9yIEFwcGxlIE1hY0Jvb2tQcm84LDIgZW50cnks
IG5vIGRldGFpbGVkIGNvbW1pdCBkZXNjcmlwdGlvbi4NCg0KRm9yIEFwcGxlIE1hY01pbmk1LDEs
IG5vIGRldGFpbGVkIGNvbW1pdCBkZXNjcmlwdGlvbi4NCg0KRm9yIEVyaWNzc29uIGVudHJ5LCBu
byBjb21taXQgZGVzY3JpcHRpb24uDQoNCkZvciBDYW55b24gQ04tQlRVMSBlbnRyeSwgbm8gZGV0
YWlsZWQgY29tbWl0IGRlc2NyaXB0aW9uLg0KDQpGb3IgQkNNMjA3MDJBMCBlbnRyeSwgaXRzIGlu
dGVyZmFjZSBjbGFzcyBpcyAweGZmLCBzbyBpdCBjYW5ub3QgYmUgcmVtb3ZlZC4NCg0KRm9yICJB
Vk0gQmx1ZUZSSVRaIiwgIkJsdWV0b290aCBVbHR0cmFwb3J0IE1vZHVsZSBmcm9tIElCTSIsIGFu
ZCAiQUxQUyBNb2R1bGVzIHdpdGggbm9uLXN0YW5kYXJkIGlkIiBlbnRyaWVzLCBubyByZWxhdGVk
IGNvbW1pdCBkZXNjcmlwdGlvbi4NCg0KPiA+IEBAIC05MTUsOSArOTE1LDE0IEBAIHN0YXRpYyBp
bnQgYnR1c2JfcHJvYmUoc3RydWN0IHVzYl9pbnRlcmZhY2UNCj4gPiAqaW50ZiwNCj4gPg0KPiA+
ICAJQlRfREJHKCJpbnRmICVwIGlkICVwIiwgaW50ZiwgaWQpOw0KPiA+DQo+ID4gLQkvKiBpbnRl
cmZhY2UgbnVtYmVycyBhcmUgaGFyZGNvZGVkIGluIHRoZSBzcGVjICovDQo+ID4gLQlpZiAoaW50
Zi0+Y3VyX2FsdHNldHRpbmctPmRlc2MuYkludGVyZmFjZU51bWJlciAhPSAwKQ0KPiA+ICsJZm9y
IChpID0gMDsgaSA8IGludGYtPmN1cl9hbHRzZXR0aW5nLT5kZXNjLmJOdW1FbmRwb2ludHM7IGkr
Kykgew0KPiA+ICsJCWVwX2Rlc2MgPSAmaW50Zi0+Y3VyX2FsdHNldHRpbmctPmVuZHBvaW50W2ld
LmRlc2M7DQo+ID4gKw0KPiA+ICsJCWlmICh1c2JfZW5kcG9pbnRfaXNfaW50X2luKGVwX2Rlc2Mp
KQ0KPiA+ICsJCQlicmVhazsNCj4gPiArDQo+ID4gIAkJcmV0dXJuIC1FTk9ERVY7DQo+ID4gKwl9
DQo+IA0KPiBUaGVyZSBpcyBubyBwb2ludCBpbiB3YWxraW5nIHRoZSBlbmRwb2ludCBsaXN0IHR3
aWNlIGluIHRoZSBwcm9iZSBmdW5jdGlvbi4gU3RvcmUNCj4gcG9zc2libGUgY2FuZGlkYXRlcyBm
b3IgaW50X2luLCBidWxrX291dCBhbmQgYnVsa19pbiBkZXNjcmlwdG9ycyBzbyB0aGF0IGxhdGVy
IG9uDQo+IHdlIGNhbiBqdXN0IGFzc2lnbiB0aGVtIHRvIG91ciBkYXRhLg0KPiANCj4gVGhlbiB3
ZSBjYW4ganVzdCBjaGVjayBpZiB0aGlzIGludGVyZmFjZSBoYXMgaW50IGFuZCBwcm9wZXIgYnVs
ayBlbmRwb2ludHMNCj4gYXZhaWxhYmxlLiBBbmQgaW4gZXJyb3IgY2FzZSByZXR1cm4gRU5PREVW
Lg0KPiANCj4gQWN0dWFsbHkgdGhpbmtpbmcgYWJvdXQgaXQsIHdlIGFscmVhZHkgZG8gdGhhdCwg
c28ganVzdCByZW1vdmluZyB0aGUgaGFyZGNvZGVkDQo+IGNoZWNrIGZvciBkZXNjLmJJbnRlcmZh
Y2VOdW1iZXIgIT0gMCBzaG91bGQgZG8gdGhlIHRyaWNrLg0KPiANCj4gPiAgCWlmICghaWQtPmRy
aXZlcl9pbmZvKSB7DQo+ID4gIAkJY29uc3Qgc3RydWN0IHVzYl9kZXZpY2VfaWQgKm1hdGNoOw0K
PiA+IEBAIC0xMDE0LDggKzEwMTksOSBAQCBzdGF0aWMgaW50IGJ0dXNiX3Byb2JlKHN0cnVjdCB1
c2JfaW50ZXJmYWNlDQo+ID4gKmludGYsDQo+ID4NCj4gPiAgCWhkZXYtPm93bmVyID0gVEhJU19N
T0RVTEU7DQo+ID4NCj4gPiAtCS8qIEludGVyZmFjZSBudW1iZXJzIGFyZSBoYXJkY29kZWQgaW4g
dGhlIHNwZWNpZmljYXRpb24gKi8NCj4gPiAtCWRhdGEtPmlzb2MgPSB1c2JfaWZudW1fdG9faWYo
ZGF0YS0+dWRldiwgMSk7DQo+ID4gKw0KPiA+ICsJZGF0YS0+aXNvYyA9IHVzYl9pZm51bV90b19p
ZihkYXRhLT51ZGV2LA0KPiA+ICsJCTEraW50Zi0+Y3VyX2FsdHNldHRpbmctPmRlc2MuYkludGVy
ZmFjZU51bWJlcik7DQo+IA0KPiBBbmQgaGVyZSB5b3UgbmVlZCB0byB0byBjaGVjayBhbGwgaW50
ZXJmYWNlcy4gSnVzdCBhc3N1bWluZyBpdCBpcyArMSBtaWdodCBub3QgYmUNCj4gZ29vZCBlbm91
Z2guIE9yIGF0IGxlYXN0IHB1dCBhIHByb3BlciBjb21tZW50IGluIGhlcmUgdGhhdCB3ZSBleHBl
Y3QgdGhlDQo+IElTT0MgaW50ZXJmYWNlIHRvIGJlICsxLiBBbmQgdGhlbiBkb24ndCBkbyAxKywg
SSBwcmVmZXIgdG8gaGF2ZQ0KPiBiSW50ZXJmYWNlTnVtYmVyICsgMS4NCj4gDQo+IFJlZ2FyZHMN
Cj4gDQo+IE1hcmNlbA0KPiANCg0K

2011-11-16 05:45:27

by Marcel Holtmann

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

> 1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
> 2 Delete desc.bInterfaceNumber != 0 check.
>
> Signed-off-by: Costa Yao <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel



2011-11-16 02:14:45

by Yao, Costa

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

MSBVc2UgVVNCX0lOVEVSRkFDRV9JTkZPIHRvIGRvIGRldmljZSBtYXRjaGluZywgb3RoZXIgdGhh
biBVU0JfREVWSUNFX0lORk8uDQoyIERlbGV0ZSBkZXNjLmJJbnRlcmZhY2VOdW1iZXIgIT0gMCBj
aGVjay4NCg0KU2lnbmVkLW9mZi1ieTogQ29zdGEgWWFvIDxjcXlhb0BxY2EucXVhbGNvbW0uY29t
Pg0KLS0tDQogZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYyB8ICAgMTYgKysrKysrKystLS0tLS0t
LQ0KIDEgZmlsZXMgY2hhbmdlZCwgOCBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQ0KDQpk
aWZmIC0tZ2l0IGEvZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYyBiL2RyaXZlcnMvYmx1ZXRvb3Ro
L2J0dXNiLmMNCmluZGV4IDJiZDg3ZDQuLjM5ZGQ2NWMgMTAwNjQ0DQotLS0gYS9kcml2ZXJzL2Js
dWV0b290aC9idHVzYi5jDQorKysgYi9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jDQpAQCAtNTcs
OCArNTcsOCBAQCBzdGF0aWMgc3RydWN0IHVzYl9kcml2ZXIgYnR1c2JfZHJpdmVyOw0KICNkZWZp
bmUgQlRVU0JfQVRIMzAxMgkJMHg4MA0KIA0KIHN0YXRpYyBzdHJ1Y3QgdXNiX2RldmljZV9pZCBi
dHVzYl90YWJsZVtdID0gew0KLQkvKiBHZW5lcmljIEJsdWV0b290aCBVU0IgZGV2aWNlICovDQot
CXsgVVNCX0RFVklDRV9JTkZPKDB4ZTAsIDB4MDEsIDB4MDEpIH0sDQorCS8qIEJsdWV0b290aCBV
U0IgaW50ZXJmYWNlICovDQorCXsgVVNCX0lOVEVSRkFDRV9JTkZPKDB4ZTAsIDB4MDEsIDB4MDEp
IH0sDQogDQogCS8qIEJyb2FkY29tIFNvZnRTYWlsaW5nIHJlcG9ydGluZyB2ZW5kb3Igc3BlY2lm
aWMgKi8NCiAJeyBVU0JfREVWSUNFKDB4MDVhYywgMHgyMWUxKSB9LA0KQEAgLTkxOCwxMCArOTE4
LDYgQEAgc3RhdGljIGludCBidHVzYl9wcm9iZShzdHJ1Y3QgdXNiX2ludGVyZmFjZSAqaW50ZiwN
CiANCiAJQlRfREJHKCJpbnRmICVwIGlkICVwIiwgaW50ZiwgaWQpOw0KIA0KLQkvKiBpbnRlcmZh
Y2UgbnVtYmVycyBhcmUgaGFyZGNvZGVkIGluIHRoZSBzcGVjICovDQotCWlmIChpbnRmLT5jdXJf
YWx0c2V0dGluZy0+ZGVzYy5iSW50ZXJmYWNlTnVtYmVyICE9IDApDQotCQlyZXR1cm4gLUVOT0RF
VjsNCi0NCiAJaWYgKCFpZC0+ZHJpdmVyX2luZm8pIHsNCiAJCWNvbnN0IHN0cnVjdCB1c2JfZGV2
aWNlX2lkICptYXRjaDsNCiAJCW1hdGNoID0gdXNiX21hdGNoX2lkKGludGYsIGJsYWNrbGlzdF90
YWJsZSk7DQpAQCAtMTAxNyw4ICsxMDEzLDEyIEBAIHN0YXRpYyBpbnQgYnR1c2JfcHJvYmUoc3Ry
dWN0IHVzYl9pbnRlcmZhY2UgKmludGYsDQogDQogCWhkZXYtPm93bmVyID0gVEhJU19NT0RVTEU7
DQogDQotCS8qIEludGVyZmFjZSBudW1iZXJzIGFyZSBoYXJkY29kZWQgaW4gdGhlIHNwZWNpZmlj
YXRpb24gKi8NCi0JZGF0YS0+aXNvYyA9IHVzYl9pZm51bV90b19pZihkYXRhLT51ZGV2LCAxKTsN
CisJLyogQWNjb3JkaW5nIHRvIEhDSS1VU0Igc3BlY2lmaWNhdGlvbiwgdGhlIGludGVyZmFjZSBm
b3INCisJICogU0NPIGRhdGEgZW5kcG9pbnRzIGZvbGxvd3MgdGhlIGludGVyZmFjZSBmb3IgY29t
bWFuZHMsDQorCSAqIGV2ZW50cyBhbmQgQUNMIGRhdGENCisJICovDQorCWRhdGEtPmlzb2MgPSB1
c2JfaWZudW1fdG9faWYoZGF0YS0+dWRldiwNCisJCWludGYtPmN1cl9hbHRzZXR0aW5nLT5kZXNj
LmJJbnRlcmZhY2VOdW1iZXIgKyAxKTsNCiANCiAJaWYgKCFyZXNldCkNCiAJCXNldF9iaXQoSENJ
X1FVSVJLX05PX1JFU0VULCAmaGRldi0+cXVpcmtzKTsNCi0tIA0KMS43LjQuMQ0KDQoNCj4gLS0t
LS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEhvbHRtYW5uIFttYWlsdG86
bWFyY2VsQGhvbHRtYW5uLm9yZ10NCj4gU2VudDogMjAxMeW5tDEx5pyIMTbml6UgOToxNg0KPiBU
bzogWWFvLCBDb3N0YQ0KPiBDYzogcGFkb3ZhbkBwcm9mdXNpb24ubW9iaTsgbGludXgtYmx1ZXRv
b3RoQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIXSBCbHVldG9vdGg6IGJ0
dXNiOiBVc2UgVVNCX0lOVEVSRkFDRV9JTkZPIHRvIGRvIGRldmljZQ0KPiBtYXRjaGluZw0KPiAN
Cj4gSGkgQ29zdGEsDQo+IA0KPiA+IDEgVXNlIFVTQl9JTlRFUkZBQ0VfSU5GTyB0byBkbyBkZXZp
Y2UgbWF0Y2hpbmcsIG90aGVyIHRoYW4NCj4gVVNCX0RFVklDRV9JTkZPLg0KPiA+IDIgRGVsZXRl
IGRlc2MuYkludGVyZmFjZU51bWJlciAhPSAwIGNoZWNrLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1i
eTogQ29zdGEgWWFvIDxjcXlhb0BxY2EucXVhbGNvbW0uY29tPg0KPiA+IC0tLQ0KPiA+ICBkcml2
ZXJzL2JsdWV0b290aC9idHVzYi5jIHwgICAxNiArKysrKysrKy0tLS0tLS0tDQo+ID4gIDEgZmls
ZXMgY2hhbmdlZCwgOCBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQ0KPiANCj4gdGhpcyBs
b29rcyBmaW5lIHRvIG1lIG5vdy4gQW5kIGlzbid0IHRoaXMgYSBtdWNoIGNsZWFuZXIgcGF0Y2gg
OykNCj4gDQo+ID4gLQkvKiBJbnRlcmZhY2UgbnVtYmVycyBhcmUgaGFyZGNvZGVkIGluIHRoZSBz
cGVjaWZpY2F0aW9uICovDQo+ID4gLQlkYXRhLT5pc29jID0gdXNiX2lmbnVtX3RvX2lmKGRhdGEt
PnVkZXYsIDEpOw0KPiA+ICsJLyogQWNjb3JkaW5nIHRvIEhDSS1VU0Igc3BlY2lmaWNhdGlvbiwg
dGhlIGludGVyZmFjZSBmb3INCj4gPiArCSAqIFNDTyBkYXRhIGVuZHBvaW50IGZvbGxvd3MgdGhl
IGludGVyZmFjZSBmb3IgY29tbWFuZHMsDQo+ID4gKwkgKiBldmVudHMgYW5kIEFDTCBkYXRhDQo+
ID4gKwkgKi8NCj4gDQo+IE1pbm9yIG5pdHBpY2sgaGVyZS4gSXQgaXMgU0NPIGRhdGEgZW5kcG9p
bnRzIChwbHVyYWwpLiBKdXN0IHJlc2VuZCB0aGUgcGF0Y2ggd2l0aA0KPiB0aGlzIGZpeGVkIGFu
ZCBmZWVsIGZyZWUgdG8gYWRkIG15IEFDSy4NCj4gDQo+ID4gKwlkYXRhLT5pc29jID0gdXNiX2lm
bnVtX3RvX2lmKGRhdGEtPnVkZXYsDQo+ID4gKwkJaW50Zi0+Y3VyX2FsdHNldHRpbmctPmRlc2Mu
YkludGVyZmFjZU51bWJlciArIDEpOw0KPiA+DQo+ID4gIAlpZiAoIXJlc2V0KQ0KPiA+ICAJCXNl
dF9iaXQoSENJX1FVSVJLX05PX1JFU0VULCAmaGRldi0+cXVpcmtzKTsNCj4gDQo+IEFja2VkLWJ5
OiBNYXJjZWwgSG9sdG1hbm4gPG1hcmNlbEBob2x0bWFubi5vcmc+DQo+IA0KPiBSZWdhcmRzDQo+
IA0KPiBNYXJjZWwNCj4gDQoNCg==

2011-11-16 01:16:22

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

> 1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
> 2 Delete desc.bInterfaceNumber != 0 check.
>
> Signed-off-by: Costa Yao <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)

this looks fine to me now. And isn't this a much cleaner patch ;)

> - /* Interface numbers are hardcoded in the specification */
> - data->isoc = usb_ifnum_to_if(data->udev, 1);
> + /* According to HCI-USB specification, the interface for
> + * SCO data endpoint follows the interface for commands,
> + * events and ACL data
> + */

Minor nitpick here. It is SCO data endpoints (plural). Just resend the
patch with this fixed and feel free to add my ACK.

> + data->isoc = usb_ifnum_to_if(data->udev,
> + intf->cur_altsetting->desc.bInterfaceNumber + 1);
>
> if (!reset)
> set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel



2011-11-15 02:23:08

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

> 1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
> 2 And we try to find the first interface containing the interrupt endpoint.
>
> Signed-off-by: Costa Yao <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 63 ++++++++++++++++++++++++--------------------
> 1 files changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 2bd87d4..2575256 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -57,8 +57,8 @@ static struct usb_driver btusb_driver;
> #define BTUSB_ATH3012 0x80
>
> static struct usb_device_id btusb_table[] = {
> - /* Generic Bluetooth USB device */
> - { USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
> + /* Bluetooth USB interface */
> + { USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },
>
> /* Broadcom SoftSailing reporting vendor specific */
> { USB_DEVICE(0x05ac, 0x21e1) },
> @@ -912,14 +912,35 @@ static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> struct usb_endpoint_descriptor *ep_desc;
> + struct usb_endpoint_descriptor *intr_ep = NULL;
> + struct usb_endpoint_descriptor *bulk_tx_ep = NULL;
> + struct usb_endpoint_descriptor *bulk_rx_ep = NULL;
> struct btusb_data *data;
> struct hci_dev *hdev;
> int i, err;
>
> BT_DBG("intf %p id %p", intf, id);
>
> - /* interface numbers are hardcoded in the spec */
> - if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
> + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> + ep_desc = &intf->cur_altsetting->endpoint[i].desc;
> +
> + if (usb_endpoint_is_int_in(ep_desc)) {
> + intr_ep = ep_desc;
> + continue;
> + }
> +
> + if (usb_endpoint_is_bulk_out(ep_desc)) {
> + bulk_tx_ep = ep_desc;
> + continue;
> + }
> +
> + if (usb_endpoint_is_bulk_in(ep_desc)) {
> + bulk_rx_ep = ep_desc;
> + continue;
> + }
> + }
> +
> + if (!intr_ep || !bulk_tx_ep || !bulk_rx_ep)
> return -ENODEV;
>
> if (!id->driver_info) {
> @@ -954,29 +975,9 @@ static int btusb_probe(struct usb_interface *intf,
> if (!data)
> return -ENOMEM;
>
> - for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> - ep_desc = &intf->cur_altsetting->endpoint[i].desc;
> -
> - if (!data->intr_ep && usb_endpoint_is_int_in(ep_desc)) {
> - data->intr_ep = ep_desc;
> - continue;
> - }
> -
> - if (!data->bulk_tx_ep && usb_endpoint_is_bulk_out(ep_desc)) {
> - data->bulk_tx_ep = ep_desc;
> - continue;
> - }
> -
> - if (!data->bulk_rx_ep && usb_endpoint_is_bulk_in(ep_desc)) {
> - data->bulk_rx_ep = ep_desc;
> - continue;
> - }
> - }
> -
> - if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
> - kfree(data);
> - return -ENODEV;
> - }
> + data->intr_ep = intr_ep;
> + data->bulk_tx_ep = bulk_tx_ep;
> + data->bulk_rx_ep = bulk_rx_ep;

so what is wrong with just this approach:

@@ -907,10 +907,6 @@ static int btusb_probe(struct usb_interface *intf,

BT_DBG("intf %p id %p", intf, id);

- /* interface numbers are hardcoded in the spec */
- if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
- return -ENODEV;
-
if (!id->driver_info) {
const struct usb_device_id *match;
match = usb_match_id(intf, blacklist_table);

> data->cmdreq_type = USB_TYPE_CLASS;
>
> @@ -1017,8 +1018,12 @@ static int btusb_probe(struct usb_interface *intf,
>
> hdev->owner = THIS_MODULE;
>
> - /* Interface numbers are hardcoded in the specification */
> - data->isoc = usb_ifnum_to_if(data->udev, 1);
> + /* Note: As HCI-USB description,
> + * We can expect isoc interface to be bInterfaceNumber + 1.
> + * Or it is better to check all interfaces to look for isoc.
> + */

And this comment is misleading.

/* According to HCI-USB specification the interface for
* SCO data endpoints follows the interface for commands,
* events and ACL data */

Use this one instead.

> + data->isoc = usb_ifnum_to_if(data->udev,
> + intf->cur_altsetting->desc.bInterfaceNumber + 1);
>
> if (!reset)
> set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);

if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
return err;
}
}

And please prepare and extra patch on top of this one, that adds a check
for the proper interface descriptor. We only wanna claim the ISOC
interface if they are actually Cls=e0(wlcon) Sub=01 Prot=01. Otherwise
we should not claim that interface.

Regards

Marcel



2011-11-14 10:20:40

by Yao, Costa

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

SGkgTWFyY2VsLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IE1hcmNl
bCBIb2x0bWFubiBbbWFpbHRvOm1hcmNlbEBob2x0bWFubi5vcmddDQo+IFNlbnQ6IDIwMTHlubQx
MeaciDE05pelIDE3OjMyDQo+IFRvOiBZYW8sIENvc3RhDQo+IENjOiBwYWRvdmFuQHByb2Z1c2lv
bi5tb2JpOyBsaW51eC1ibHVldG9vdGhAdmdlci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBb
UEFUQ0hdIEJsdWV0b290aDogYnR1c2I6IFVzZSBVU0JfSU5URVJGQUNFX0lORk8gdG8gZG8gZGV2
aWNlDQo+IG1hdGNoaW5nDQo+IA0KPiBIaSBDb3N0YSwNCj4gDQo+ID4gMSBVc2UgVVNCX0lOVEVS
RkFDRV9JTkZPIHRvIGRvIGRldmljZSBtYXRjaGluZywgb3RoZXIgdGhhbg0KPiBVU0JfREVWSUNF
X0lORk8uDQo+ID4gMiBBbmQgd2UgdHJ5IHRvIGZpbmQgdGhlIGZpcnN0IGludGVyZmFjZSBjb250
YWluaW5nIHRoZSBpbnRlcnJ1cHQgZW5kcG9pbnQuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBD
b3N0YSBZYW8gPGNxeWFvQHFjYS5xdWFsY29tbS5jb20+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMv
Ymx1ZXRvb3RoL2J0dXNiLmMgfCAgIDE2ICsrKysrKysrKysrLS0tLS0NCj4gPiAgMSBmaWxlcyBj
aGFuZ2VkLCAxMSBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAt
LWdpdCBhL2RyaXZlcnMvYmx1ZXRvb3RoL2J0dXNiLmMgYi9kcml2ZXJzL2JsdWV0b290aC9idHVz
Yi5jDQo+ID4gaW5kZXggOWRiMjQ3Ni4uODBkN2UyZCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJz
L2JsdWV0b290aC9idHVzYi5jDQo+ID4gKysrIGIvZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYw0K
PiA+IEBAIC01OCw3ICs1OCw3IEBAIHN0YXRpYyBzdHJ1Y3QgdXNiX2RyaXZlciBidHVzYl9kcml2
ZXI7DQo+ID4NCj4gPiAgc3RhdGljIHN0cnVjdCB1c2JfZGV2aWNlX2lkIGJ0dXNiX3RhYmxlW10g
PSB7DQo+ID4gIAkvKiBHZW5lcmljIEJsdWV0b290aCBVU0IgZGV2aWNlICovDQo+ID4gLQl7IFVT
Ql9ERVZJQ0VfSU5GTygweGUwLCAweDAxLCAweDAxKSB9LA0KPiA+ICsJeyBVU0JfSU5URVJGQUNF
X0lORk8oMHhlMCwgMHgwMSwgMHgwMSkgfSwNCj4gPg0KPiA+ICAJLyogQnJvYWRjb20gU29mdFNh
aWxpbmcgcmVwb3J0aW5nIHZlbmRvciBzcGVjaWZpYyAqLw0KPiA+ICAJeyBVU0JfREVWSUNFKDB4
MDVhYywgMHgyMWUxKSB9LA0KPiANCj4gdGhpcyBhY3R1YWxseSBtZWFucyB0aGF0IHNvbWUgb2Yg
dGhlIFVTQl9ERVZJQ0UgZW50cmllcyBjYW4gYmUgcmVtb3ZlZC4NCj4gUGxlYXNlIGdvIHRocm91
Z2ggb3VyIGNvbW1pdCBsb2dzIGZvciBidHVzYi5jIGFuZCBmaWd1cmUgb3V0IHdoaWNoIG9uZXMg
Y2FuIGJlDQo+IHJlbW92ZWQgYW5kIHNlbmQgYSBzZXBhcmF0ZSBwYXRjaCBmb3IgdGhhdC4NCk9r
LiBJIHdpbGwgc2VuZCBhIHNlcGFyYXRlIHBhdGNoIGZvciByZW1vdmluZyBzb21lIFVTQl9ERVZJ
Q0UgZW50cmllcy4gDQpJdCBtYWtlcyBzZW5zZSB0byBkbyByZW1vdmUgbGF0ZXIuIFNvIEkgd2ls
bCBjb250aW51ZSB0byBtb2RpZnkgdGhpcyBwYXRjaCBmb3IgeW91ciByZXZpZXcgZmlyc3RseS4g
DQoNCj4gDQo+ID4gQEAgLTkxNSw5ICs5MTUsMTQgQEAgc3RhdGljIGludCBidHVzYl9wcm9iZShz
dHJ1Y3QgdXNiX2ludGVyZmFjZQ0KPiA+ICppbnRmLA0KPiA+DQo+ID4gIAlCVF9EQkcoImludGYg
JXAgaWQgJXAiLCBpbnRmLCBpZCk7DQo+ID4NCj4gPiAtCS8qIGludGVyZmFjZSBudW1iZXJzIGFy
ZSBoYXJkY29kZWQgaW4gdGhlIHNwZWMgKi8NCj4gPiAtCWlmIChpbnRmLT5jdXJfYWx0c2V0dGlu
Zy0+ZGVzYy5iSW50ZXJmYWNlTnVtYmVyICE9IDApDQo+ID4gKwlmb3IgKGkgPSAwOyBpIDwgaW50
Zi0+Y3VyX2FsdHNldHRpbmctPmRlc2MuYk51bUVuZHBvaW50czsgaSsrKSB7DQo+ID4gKwkJZXBf
ZGVzYyA9ICZpbnRmLT5jdXJfYWx0c2V0dGluZy0+ZW5kcG9pbnRbaV0uZGVzYzsNCj4gPiArDQo+
ID4gKwkJaWYgKHVzYl9lbmRwb2ludF9pc19pbnRfaW4oZXBfZGVzYykpDQo+ID4gKwkJCWJyZWFr
Ow0KPiA+ICsNCj4gPiAgCQlyZXR1cm4gLUVOT0RFVjsNCj4gPiArCX0NCj4gDQo+IFRoZXJlIGlz
IG5vIHBvaW50IGluIHdhbGtpbmcgdGhlIGVuZHBvaW50IGxpc3QgdHdpY2UgaW4gdGhlIHByb2Jl
IGZ1bmN0aW9uLiBTdG9yZQ0KPiBwb3NzaWJsZSBjYW5kaWRhdGVzIGZvciBpbnRfaW4sIGJ1bGtf
b3V0IGFuZCBidWxrX2luIGRlc2NyaXB0b3JzIHNvIHRoYXQgbGF0ZXIgb24NCj4gd2UgY2FuIGp1
c3QgYXNzaWduIHRoZW0gdG8gb3VyIGRhdGEuDQo+IA0KPiBUaGVuIHdlIGNhbiBqdXN0IGNoZWNr
IGlmIHRoaXMgaW50ZXJmYWNlIGhhcyBpbnQgYW5kIHByb3BlciBidWxrIGVuZHBvaW50cw0KPiBh
dmFpbGFibGUuIEFuZCBpbiBlcnJvciBjYXNlIHJldHVybiBFTk9ERVYuDQo+IA0KPiBBY3R1YWxs
eSB0aGlua2luZyBhYm91dCBpdCwgd2UgYWxyZWFkeSBkbyB0aGF0LCBzbyBqdXN0IHJlbW92aW5n
IHRoZSBoYXJkY29kZWQNCj4gY2hlY2sgZm9yIGRlc2MuYkludGVyZmFjZU51bWJlciAhPSAwIHNo
b3VsZCBkbyB0aGUgdHJpY2suDQo+IA0KPiA+ICAJaWYgKCFpZC0+ZHJpdmVyX2luZm8pIHsNCj4g
PiAgCQljb25zdCBzdHJ1Y3QgdXNiX2RldmljZV9pZCAqbWF0Y2g7DQo+ID4gQEAgLTEwMTQsOCAr
MTAxOSw5IEBAIHN0YXRpYyBpbnQgYnR1c2JfcHJvYmUoc3RydWN0IHVzYl9pbnRlcmZhY2UNCj4g
PiAqaW50ZiwNCj4gPg0KPiA+ICAJaGRldi0+b3duZXIgPSBUSElTX01PRFVMRTsNCj4gPg0KPiA+
IC0JLyogSW50ZXJmYWNlIG51bWJlcnMgYXJlIGhhcmRjb2RlZCBpbiB0aGUgc3BlY2lmaWNhdGlv
biAqLw0KPiA+IC0JZGF0YS0+aXNvYyA9IHVzYl9pZm51bV90b19pZihkYXRhLT51ZGV2LCAxKTsN
Cj4gPiArDQo+ID4gKwlkYXRhLT5pc29jID0gdXNiX2lmbnVtX3RvX2lmKGRhdGEtPnVkZXYsDQo+
ID4gKwkJMStpbnRmLT5jdXJfYWx0c2V0dGluZy0+ZGVzYy5iSW50ZXJmYWNlTnVtYmVyKTsNCj4g
DQo+IEFuZCBoZXJlIHlvdSBuZWVkIHRvIHRvIGNoZWNrIGFsbCBpbnRlcmZhY2VzLiBKdXN0IGFz
c3VtaW5nIGl0IGlzICsxIG1pZ2h0IG5vdCBiZQ0KPiBnb29kIGVub3VnaC4gT3IgYXQgbGVhc3Qg
cHV0IGEgcHJvcGVyIGNvbW1lbnQgaW4gaGVyZSB0aGF0IHdlIGV4cGVjdCB0aGUNCj4gSVNPQyBp
bnRlcmZhY2UgdG8gYmUgKzEuIEFuZCB0aGVuIGRvbid0IGRvIDErLCBJIHByZWZlciB0byBoYXZl
DQo+IGJJbnRlcmZhY2VOdW1iZXIgKyAxLg0KPiANCj4gUmVnYXJkcw0KPiANCj4gTWFyY2VsDQo+
IA0KDQo=

2011-11-14 09:32:20

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

Hi Costa,

> 1 Use USB_INTERFACE_INFO to do device matching, other than USB_DEVICE_INFO.
> 2 And we try to find the first interface containing the interrupt endpoint.
>
> Signed-off-by: Costa Yao <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 +++++++++++-----
> 1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 9db2476..80d7e2d 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -58,7 +58,7 @@ static struct usb_driver btusb_driver;
>
> static struct usb_device_id btusb_table[] = {
> /* Generic Bluetooth USB device */
> - { USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
> + { USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },
>
> /* Broadcom SoftSailing reporting vendor specific */
> { USB_DEVICE(0x05ac, 0x21e1) },

this actually means that some of the USB_DEVICE entries can be removed.
Please go through our commit logs for btusb.c and figure out which ones
can be removed and send a separate patch for that.

> @@ -915,9 +915,14 @@ static int btusb_probe(struct usb_interface *intf,
>
> BT_DBG("intf %p id %p", intf, id);
>
> - /* interface numbers are hardcoded in the spec */
> - if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
> + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> + ep_desc = &intf->cur_altsetting->endpoint[i].desc;
> +
> + if (usb_endpoint_is_int_in(ep_desc))
> + break;
> +
> return -ENODEV;
> + }

There is no point in walking the endpoint list twice in the probe
function. Store possible candidates for int_in, bulk_out and bulk_in
descriptors so that later on we can just assign them to our data.

Then we can just check if this interface has int and proper bulk
endpoints available. And in error case return ENODEV.

Actually thinking about it, we already do that, so just removing the
hardcoded check for desc.bInterfaceNumber != 0 should do the trick.

> if (!id->driver_info) {
> const struct usb_device_id *match;
> @@ -1014,8 +1019,9 @@ static int btusb_probe(struct usb_interface *intf,
>
> hdev->owner = THIS_MODULE;
>
> - /* Interface numbers are hardcoded in the specification */
> - data->isoc = usb_ifnum_to_if(data->udev, 1);
> +
> + data->isoc = usb_ifnum_to_if(data->udev,
> + 1+intf->cur_altsetting->desc.bInterfaceNumber);

And here you need to to check all interfaces. Just assuming it is +1
might not be good enough. Or at least put a proper comment in here that
we expect the ISOC interface to be +1. And then don't do 1+, I prefer to
have bInterfaceNumber + 1.

Regards

Marcel