From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [4.15 stable regression] "Bluetooth: btusb: Fix quirk for Atheros 1525/QCA6174" breaks bluetooth on some devices From: Hans de Goede To: Takashi Iwai Cc: Greg Kroah-Hartman , Marcel Holtmann , "M. Kristall" , linux-bluetooth@vger.kernel.org References: <6c97cffb-751f-466d-cd7b-42624fdb18c7@redhat.com> <9a752bfc-74bf-25e7-8820-91d1e3163b75@redhat.com> <20180425124757.GA29829@kroah.com> <9b2d31fc-be06-cc05-1ec1-3d0f6f1e21bf@redhat.com> Message-ID: <0983abe5-6e1a-c210-c377-d9fa929549f3@redhat.com> Date: Fri, 27 Apr 2018 14:11:12 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed List-ID: 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 >> 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 >> --- >>   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); >>