[PATCH] I have no idea what I'm doing

[PATCH] I have no idea what I'm doing

[Chris Mason]

Hi Marcel,

I upgraded my macbook from 4.0.2 to 4.1.2, and now I get
messages similar to this bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=100651

and this one:

https://bugzilla.kernel.org/show_bug.cgi?id=101171

[21552.711888] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
[21728.019676] Bluetooth: hci0: BCM: Read verbose config info failed (-16)

A little poking around shows you recently added this check, and from
what I can tell with grep and friends the btbcm_read_verbose_config()
function is completely new?  I don't see similar reads being done by
older kernels.

So, I pushed some code around.  It does compile, but printk proves that
only the second hunk is happening on my box, so testing is somewhat
light.

With this applied, I'm able to use bluetooth again.

Signed-off-by: Chris Mason <clm@fb.com>

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 4bba866..90e7099 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -294,11 +294,10 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 
 	/* Read Verbose Config Version Info */
 	skb = btbcm_read_verbose_config(hdev);
-	if (IS_ERR(skb))
-		return PTR_ERR(skb);
-
-	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
-	kfree_skb(skb);
+	if (!IS_ERR(skb)) {
+		BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
+		kfree_skb(skb);
+	}
 
 	switch ((rev & 0xf000) >> 12) {
 	case 0:
@@ -378,12 +377,11 @@ int btbcm_setup_apple(struct hci_dev *hdev)
 
 	/* Read Verbose Config Version Info */
 	skb = btbcm_read_verbose_config(hdev);
-	if (IS_ERR(skb))
-		return PTR_ERR(skb);
-
-	BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
-		get_unaligned_le16(skb->data + 5));
-	kfree_skb(skb);
+	if (!IS_ERR(skb)) {
+		BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
+			get_unaligned_le16(skb->data + 5));
+		kfree_skb(skb);
+	}
 
 	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 

Re: [PATCH] I have no idea what I'm doing

[Marcel Holtmann]

Hi Chris,

> I upgraded my macbook from 4.0.2 to 4.1.2, and now I get
> messages similar to this bugzilla:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=100651
> 
> and this one:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=101171
> 
> [21552.711888] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
> [21728.019676] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
> 
> A little poking around shows you recently added this check, and from
> what I can tell with grep and friends the btbcm_read_verbose_config()
> function is completely new?  I don't see similar reads being done by
> older kernels.
> 
> So, I pushed some code around.  It does compile, but printk proves that
> only the second hunk is happening on my box, so testing is somewhat
> light.
> 
> With this applied, I'm able to use bluetooth again.
> 
> Signed-off-by: Chris Mason <clm@fb.com>
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 4bba866..90e7099 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -294,11 +294,10 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 
> 	/* Read Verbose Config Version Info */
> 	skb = btbcm_read_verbose_config(hdev);
> -	if (IS_ERR(skb))
> -		return PTR_ERR(skb);
> -
> -	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
> -	kfree_skb(skb);
> +	if (!IS_ERR(skb)) {
> +		BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
> +		kfree_skb(skb);
> +	}

leave this one out of it. Since all Broadcom chips do actually support this as far as I can tell. We really want to fail this one since something went wrong in that case.

> 
> 	switch ((rev & 0xf000) >> 12) {
> 	case 0:
> @@ -378,12 +377,11 @@ int btbcm_setup_apple(struct hci_dev *hdev)
> 
> 	/* Read Verbose Config Version Info */
> 	skb = btbcm_read_verbose_config(hdev);
> -	if (IS_ERR(skb))
> -		return PTR_ERR(skb);
> -
> -	BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
> -		get_unaligned_le16(skb->data + 5));
> -	kfree_skb(skb);
> +	if (!IS_ERR(skb)) {
> +		BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
> +			get_unaligned_le16(skb->data + 5));
> +		kfree_skb(skb);
> +	}
> 
> 	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);

This hunk is fine. I would accept a patch for this. I tested 8 Apple Bluetooth modules and they were all fine with this command. It seems there is this one chip in this one MacBook that is not.

Actually can you run something like "hciconfig hci0 version" and see if it really is a Broadcom chip in there or if Apple switched vendors and we are accidentally assuming it is Broadcom while in reality it is not. If it is not then the quirk might actually not apply either anymore.

Regards

Marcel

Re: [PATCH] I have no idea what I'm doing

[Chris Mason]

On Tue, Jul 14, 2015 at 09:49:28PM +0200, Marcel Holtmann wrote:
> Hi Chris,
> 
> > I upgraded my macbook from 4.0.2 to 4.1.2, and now I get
> > messages similar to this bugzilla:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=100651
> > 
> > and this one:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=101171
> > 
> > [21552.711888] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
> > [21728.019676] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
> > 
> > A little poking around shows you recently added this check, and from
> > what I can tell with grep and friends the btbcm_read_verbose_config()
> > function is completely new?  I don't see similar reads being done by
> > older kernels.
> > 
> > So, I pushed some code around.  It does compile, but printk proves that
> > only the second hunk is happening on my box, so testing is somewhat
> > light.
> > 
> > With this applied, I'm able to use bluetooth again.
> > 
> > Signed-off-by: Chris Mason <clm@fb.com>
> > 
> > diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> > index 4bba866..90e7099 100644
> > --- a/drivers/bluetooth/btbcm.c
> > +++ b/drivers/bluetooth/btbcm.c
> > @@ -294,11 +294,10 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> > 
> > 	/* Read Verbose Config Version Info */
> > 	skb = btbcm_read_verbose_config(hdev);
> > -	if (IS_ERR(skb))
> > -		return PTR_ERR(skb);
> > -
> > -	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
> > -	kfree_skb(skb);
> > +	if (!IS_ERR(skb)) {
> > +		BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
> > +		kfree_skb(skb);
> > +	}
> 
> leave this one out of it. Since all Broadcom chips do actually support
> this as far as I can tell. We really want to fail this one since
> something went wrong in that case.

ack.  Both of those bugzillas look like macs, so they shouldn't need
this hunk either.

> 
> > 
> > 	switch ((rev & 0xf000) >> 12) {
> > 	case 0:
> > @@ -378,12 +377,11 @@ int btbcm_setup_apple(struct hci_dev *hdev)
> > 
> > 	/* Read Verbose Config Version Info */
> > 	skb = btbcm_read_verbose_config(hdev);
> > -	if (IS_ERR(skb))
> > -		return PTR_ERR(skb);
> > -
> > -	BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
> > -		get_unaligned_le16(skb->data + 5));
> > -	kfree_skb(skb);
> > +	if (!IS_ERR(skb)) {
> > +		BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
> > +			get_unaligned_le16(skb->data + 5));
> > +		kfree_skb(skb);
> > +	}
> > 
> > 	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> 
> This hunk is fine. I would accept a patch for this. I tested 8 Apple
> Bluetooth modules and they were all fine with this command. It seems
> there is this one chip in this one MacBook that is not.
> 
>
> Actually can you run something like "hciconfig hci0 version" and see
> if it really is a Broadcom chip in there or if Apple switched vendors
> and we are accidentally assuming it is Broadcom while in reality it is
> not. If it is not then the quirk might actually not apply either
> anymore.

mason@ret ~> hciconfig hci0 version
hci0:	Type: BR/EDR  Bus: USB
	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
	HCI Version: 4.0 (0x6)  Revision: 0x21ae
	LMP Version: 4.0 (0x6)  Subversion: 0x414e
	Manufacturer: Broadcom Corporation (15)

One of the threads I saw when I was googling claimed it worked if you
rebooted from 4.0 into 4.1 but not 4.1 into 4.1.

-chris

Re: [PATCH] I have no idea what I'm doing

[Marcel Holtmann]

Hi Chris,

>>> I upgraded my macbook from 4.0.2 to 4.1.2, and now I get
>>> messages similar to this bugzilla:
>>> 
>>> https://bugzilla.kernel.org/show_bug.cgi?id=100651
>>> 
>>> and this one:
>>> 
>>> https://bugzilla.kernel.org/show_bug.cgi?id=101171
>>> 
>>> [21552.711888] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
>>> [21728.019676] Bluetooth: hci0: BCM: Read verbose config info failed (-16)
>>> 
>>> A little poking around shows you recently added this check, and from
>>> what I can tell with grep and friends the btbcm_read_verbose_config()
>>> function is completely new?  I don't see similar reads being done by
>>> older kernels.
>>> 
>>> So, I pushed some code around.  It does compile, but printk proves that
>>> only the second hunk is happening on my box, so testing is somewhat
>>> light.
>>> 
>>> With this applied, I'm able to use bluetooth again.
>>> 
>>> Signed-off-by: Chris Mason <clm@fb.com>
>>> 
>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>> index 4bba866..90e7099 100644
>>> --- a/drivers/bluetooth/btbcm.c
>>> +++ b/drivers/bluetooth/btbcm.c
>>> @@ -294,11 +294,10 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
>>> 
>>> 	/* Read Verbose Config Version Info */
>>> 	skb = btbcm_read_verbose_config(hdev);
>>> -	if (IS_ERR(skb))
>>> -		return PTR_ERR(skb);
>>> -
>>> -	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
>>> -	kfree_skb(skb);
>>> +	if (!IS_ERR(skb)) {
>>> +		BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
>>> +		kfree_skb(skb);
>>> +	}
>> 
>> leave this one out of it. Since all Broadcom chips do actually support
>> this as far as I can tell. We really want to fail this one since
>> something went wrong in that case.
> 
> ack.  Both of those bugzillas look like macs, so they shouldn't need
> this hunk either.
> 
>> 
>>> 
>>> 	switch ((rev & 0xf000) >> 12) {
>>> 	case 0:
>>> @@ -378,12 +377,11 @@ int btbcm_setup_apple(struct hci_dev *hdev)
>>> 
>>> 	/* Read Verbose Config Version Info */
>>> 	skb = btbcm_read_verbose_config(hdev);
>>> -	if (IS_ERR(skb))
>>> -		return PTR_ERR(skb);
>>> -
>>> -	BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
>>> -		get_unaligned_le16(skb->data + 5));
>>> -	kfree_skb(skb);
>>> +	if (!IS_ERR(skb)) {
>>> +		BT_INFO("%s: BCM: chip id %u build %4.4u", hdev->name, skb->data[1],
>>> +			get_unaligned_le16(skb->data + 5));
>>> +		kfree_skb(skb);
>>> +	}
>>> 
>>> 	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>> 
>> This hunk is fine. I would accept a patch for this. I tested 8 Apple
>> Bluetooth modules and they were all fine with this command. It seems
>> there is this one chip in this one MacBook that is not.
>> 
>> 
>> Actually can you run something like "hciconfig hci0 version" and see
>> if it really is a Broadcom chip in there or if Apple switched vendors
>> and we are accidentally assuming it is Broadcom while in reality it is
>> not. If it is not then the quirk might actually not apply either
>> anymore.
> 
> mason@ret ~> hciconfig hci0 version
> hci0:	Type: BR/EDR  Bus: USB
> 	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
> 	HCI Version: 4.0 (0x6)  Revision: 0x21ae
> 	LMP Version: 4.0 (0x6)  Subversion: 0x414e
> 	Manufacturer: Broadcom Corporation (15)

so it is a Broadcom. What MacBook is this anyway?

> One of the threads I saw when I was googling claimed it worked if you
> rebooted from 4.0 into 4.1 but not 4.1 into 4.1.

I think that is just someone getting confused. It should always fail if you boot 4.1. No matter what you had running before.

Regards

Marcel

Re: [PATCH] I have no idea what I'm doing

[Chris Mason]

On Tue, Jul 14, 2015 at 10:49:00PM +0200, Marcel Holtmann wrote:
> Hi Chris,
> >> 
> >> Actually can you run something like "hciconfig hci0 version" and see
> >> if it really is a Broadcom chip in there or if Apple switched vendors
> >> and we are accidentally assuming it is Broadcom while in reality it is
> >> not. If it is not then the quirk might actually not apply either
> >> anymore.
> > 
> > mason@ret ~> hciconfig hci0 version
> > hci0:	Type: BR/EDR  Bus: USB
> > 	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
> > 	HCI Version: 4.0 (0x6)  Revision: 0x21ae
> > 	LMP Version: 4.0 (0x6)  Subversion: 0x414e
> > 	Manufacturer: Broadcom Corporation (15)
> 
> so it is a Broadcom. What MacBook is this anyway?

macbookpro 13" from 2013 (newer model in '13)

> 
> > One of the threads I saw when I was googling claimed it worked if
> > you rebooted from 4.0 into 4.1 but not 4.1 into 4.1.
> 
> I think that is just someone getting confused. It should always fail
> if you boot 4.1. No matter what you had running before.
> 

That's my experience.

-chris

Re: [PATCH] I have no idea what I'm doing

[Marcel Holtmann]

Hi Chris,

>>>> Actually can you run something like "hciconfig hci0 version" and see
>>>> if it really is a Broadcom chip in there or if Apple switched vendors
>>>> and we are accidentally assuming it is Broadcom while in reality it is
>>>> not. If it is not then the quirk might actually not apply either
>>>> anymore.
>>> 
>>> mason@ret ~> hciconfig hci0 version
>>> hci0:	Type: BR/EDR  Bus: USB
>>> 	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
>>> 	HCI Version: 4.0 (0x6)  Revision: 0x21ae
>>> 	LMP Version: 4.0 (0x6)  Subversion: 0x414e
>>> 	Manufacturer: Broadcom Corporation (15)
>> 
>> so it is a Broadcom. What MacBook is this anyway?
> 
> macbookpro 13" from 2013 (newer model in '13)

besides having that Bluetooth module for testing, I also have that actual Macbook. And I did test this. It worked fine for me. Maybe some OS X update messed with the firmware. Worth while checking.

Regards

Marcel

Re: [PATCH] I have no idea what I'm doing

[Chris Mason]

On Tue, Jul 14, 2015 at 10:58:50PM +0200, Marcel Holtmann wrote:
> Hi Chris,
> 
> >>>> Actually can you run something like "hciconfig hci0 version" and see
> >>>> if it really is a Broadcom chip in there or if Apple switched vendors
> >>>> and we are accidentally assuming it is Broadcom while in reality it is
> >>>> not. If it is not then the quirk might actually not apply either
> >>>> anymore.
> >>> 
> >>> mason@ret ~> hciconfig hci0 version
> >>> hci0:	Type: BR/EDR  Bus: USB
> >>> 	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
> >>> 	HCI Version: 4.0 (0x6)  Revision: 0x21ae
> >>> 	LMP Version: 4.0 (0x6)  Subversion: 0x414e
> >>> 	Manufacturer: Broadcom Corporation (15)
> >> 
> >> so it is a Broadcom. What MacBook is this anyway?
> > 
> > macbookpro 13" from 2013 (newer model in '13)
> 
> besides having that Bluetooth module for testing, I also have that actual Macbook. And I did test this. It worked fine for me. Maybe some OS X update messed with the firmware. Worth while checking.

This only runs linux, I don't dual boot.

-chris

Re: [PATCH] I have no idea what I'm doing

[Marcel Holtmann]

Hi Chris,

>>>>>> Actually can you run something like "hciconfig hci0 version" and see
>>>>>> if it really is a Broadcom chip in there or if Apple switched vendors
>>>>>> and we are accidentally assuming it is Broadcom while in reality it is
>>>>>> not. If it is not then the quirk might actually not apply either
>>>>>> anymore.
>>>>> 
>>>>> mason@ret ~> hciconfig hci0 version
>>>>> hci0:	Type: BR/EDR  Bus: USB
>>>>> 	BD Address: 60:03:08:8D:0D:A9  ACL MTU: 1021:8  SCO MTU: 64:1
>>>>> 	HCI Version: 4.0 (0x6)  Revision: 0x21ae
>>>>> 	LMP Version: 4.0 (0x6)  Subversion: 0x414e
>>>>> 	Manufacturer: Broadcom Corporation (15)
>>>> 
>>>> so it is a Broadcom. What MacBook is this anyway?
>>> 
>>> macbookpro 13" from 2013 (newer model in '13)
>> 
>> besides having that Bluetooth module for testing, I also have that actual Macbook. And I did test this. It worked fine for me. Maybe some OS X update messed with the firmware. Worth while checking.
> 
> This only runs linux, I don't dual boot.

if you never ran any OS X or OS X update, then this might have been a bogus firmware when the hardware shipped and by now they have all been updated. That would make some sense at least.

Regards

Marcel