Re: [4.15 stable regression] "Bluetooth: btusb: Fix quirk for Atheros 1525/QCA6174" breaks bluetooth on some devices

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 27-04-18 10:57, Hans de Goede wrote:
> Hi,
> 
> On 26-04-18 20:27, Takashi Iwai wrote:
>> On Thu, 26 Apr 2018 18:33:38 +0200,
>> Takashi Iwai wrote:
>>>
>>> On Thu, 26 Apr 2018 14:18:23 +0200,
>>> Hans de Goede wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 25-04-18 15:10, Takashi Iwai wrote:
>>>>> On Wed, 25 Apr 2018 14:49:59 +0200,
>>>>> Hans de Goede wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 25-04-18 14:47, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Apr 25, 2018 at 02:40:37PM +0200, Hans de Goede wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 18-04-18 15:18, Hans de Goede wrote:
>>>>>>>>> Hi Takashi, Marcel,
>>>>>>>>>
>>>>>>>>> It seems that this commit:
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.15.y&id=7ec32f585fefd7c154453aa29ccf8fa2a11cc865
>>>>>>>>>
>>>>>>>>> Is breaking bluetooth on some devices, see:
>>>>>>>>>
>>>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1568911
>>>>>>>>>
>>>>>>>>> The problem is the following error now being thrown:
>>>>>>>>>
>>>>>>>>> [   28.466248] Bluetooth: hci0: don't support firmware rome 0x1020200
>>>>>>>>>
>>>>>>>>> Looking at the code I wonder if maybe we need to mask the ver_rom
>>>>>>>>> with & 0xfff when comparing it to the qca_devices_table[i].rom_version
>>>>>>>>> filed ?
>>>>>>>>>
>>>>>>>>> Or maybe the commit is actually wrong, or maybe devices with the
>>>>>>>>> 0cf3:3004 USB id need either the BTUSB_QCA_ROM or BTUSB_ATH3012
>>>>>>>>> quirk depending on the device and we need to probe this somehow?
>>>>>>>>
>>>>>>>> I've been receiving more complaints from users about this on
>>>>>>>> various devices, so I think that the 7ec32f585fefd7c154453aa29ccf8fa2a11cc865
>>>>>>>> commit should be reverted from 4.15.x while we figure this out.
>>>>>>>>
>>>>>>>> Does anyone have any idea how we cam distinguish between the 2
>>>>>>>> different versions which seem to be hiding between the same USB-id ?
>>>>>>>
>>>>>>> 4.15.y is end-of-life, so there is no more releases being made for it,
>>>>>>> sorry.
>>>>>>
>>>>>> Ah, right, no problem, Fedora should be moving to 4.16.x soon then
>>>>>> anyways.
>>>>>>
>>>>>>> But, this commit is in 4.4.y, 4.9.y, 4.14.y, and 4.16.  Can you revert
>>>>>>> it in Linus's tree and I can revert it everywhere else as well?
>>>>>>
>>>>>> Takashi, do you agree that reverting this for now is best? And if so
>>>>>> can you please send a revert?
>>>>>
>>>>> The best would be to fix it properly :)
>>>>>
>>>>> But I agree that it needs a quick resolution, and the revert is
>>>>> appropriate in this case.  Since you've confirmed that the revert
>>>>> worked, feel free to submit the revert patch from your side.
>>>>
>>>> Done.
>>>>
>>>>> Back to the original issue: now I'm wondering what made such
>>>>> inconsistent behavior.  My current suspect is the racy driver loading
>>>>> between btusb and ath3k.  Both have the same USB ID, and the driver
>>>>> loading order may interfere the behavior with each other?
>>>>>
>>>>> Or might it be a WiFi/BT combo chip that may have a racy firmware
>>>>> initialization?
>>>>
>>>> I've no idea I'm afraid.
>>>
>>> I checked with the original reporter about this possibility, and all
>>> failed.  So, it's not about the interaction between them.
>>>
>>> And, now after looking at the log in your bugzilla, I guess the fix
>>> would be something like below.
>>>
>>> Basically the only significant difference between these two BT quirks
>>> is the RAM-patching.  Without RAM patching, the wrong ROM versions are
>>> read by ath3k as is on the device of our reporter.
>>>
>>> OTOH, there are devices with the correct ROM versions, and at
>>> btusb_setup_qca(), we give -ENODEV at open just because the function
>>> expects only the buggy version numbers.  Instead of -ENODEV, it should
>>> just return 0.
>>>
>>> Hans, could you check whether it works for them?
>>
>> ... or better with the one below.
> 
> Yes that one indeed looks better.
> 
> I've started a Fedora kernel (test) build with the revert + the patch
> from you below and asked the reporter_s_ of this problem to test, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=1568911
> (you may want to put yourself in the Cc there).
> 
> In the mean time I've also stumbled over this bug, which is another
> bug tracking the same issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=199271

So the reporter of this bug has tested Takashi's patch (below) on top
of the revert and his bluetooth is still working, so I think we can
move ahead and get the revert + Takashi's patch merged.

Takashi, can you do a formal submission of your patch please?

Thanks,

Hans


>> -- 8< --
>> From: Takashi Iwai <tiwai@suse.de>
>> Subject: [PATCH] Bluetooth: btusb: Apply QCQ_ROME setup for BTUSB_ATH3012
>>   quirk, too
>>
>> In commit f44cb4b19ed4 ("Bluetooth: btusb: Fix quirk for Atheros
>> 1525/QCA6174") we tried to address the non-working Atheros BT devices
>> by changing the quirk from BTUSB_ATH3012 to BTUSB_QCQ_ROME.  This made
>> such devices working while it turned out to break other existing chips
>> with the very same USB ID.
>>
>> This is another attempt to tackle the issue.  The essential point to
>> use BTUSB_QCA_ROME is to apply the btusb_setup_qca() and do RAM-
>> patching.  And the previous attempt failed because btusb_setup_qcq()
>> returns -ENODEV if the ROM version doesn't match with the expected
>> ones.  For some devices that have already the "correct" ROM versions,
>> we may just skip the setup procedure and continue the rest.
>>
>> So, this patch applies btusb_setup_qca() also in BTUSB_ATH3012 quirk,
>> and adds a check of the ROM version in the function to skip the setup
>> if the ROM version looks already sane.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> ---
>>   drivers/bluetooth/btusb.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index c8c8b0b8d333..720356320ace 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -2673,6 +2673,10 @@ static int btusb_setup_qca(struct hci_dev *hdev)
>>           return err;
>>       ver_rom = le32_to_cpu(ver.rom_version);
>> +    /* Don't care about high ROM versions */
>> +    if (ver_rom & ~0xffffU)
>> +        return 0;
>> +
>>       for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
>>           if (ver_rom == qca_devices_table[i].rom_version)
>>               info = &qca_devices_table[i];
>> @@ -3055,6 +3059,7 @@ static int btusb_probe(struct usb_interface *intf,
>>       }
>>       if (id->driver_info & BTUSB_ATH3012) {
>> +        data->setup_on_usb = btusb_setup_qca;
>>           hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
>>           set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>           set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>>