Received: by 10.223.185.116 with SMTP id b49csp1016759wrg; Fri, 16 Feb 2018 10:53:06 -0800 (PST) X-Google-Smtp-Source: AH8x224mjG2HR5cbr+zoxZJl9TPwfUSk5vtp8hqSrZ+zSdIjvwhcGvAGHqhRtuaLhuSXDhBF2QEV X-Received: by 10.99.132.74 with SMTP id k71mr5795134pgd.4.1518807186670; Fri, 16 Feb 2018 10:53:06 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518807186; cv=none; d=google.com; s=arc-20160816; b=cuqPzw/QifHaIrv8/zVqbLBKZzN244KzVnKWYaW7uLbQlhwjoiuEsJbh4XwRcIqFAN oP3/w9x1AXzLl9cEsvB0FzNbLIRWQHFEsZPgnSFjdxXJKSrdNFyQvpbVNkxRZoZga2h+ gU8F4phJWPxBz7JMR1qNSbsDyoXmXaNmYXMHQET4U/S+ABPBjuT/VetWFj/HP3OqqFDG q0uOYhxCQuAvn2e2/W5iRqE7PsKFDPSWEllsRsW/1S3QfTHEXBfLqJ0UTfmW2Lvzythb xYwcYeriiKuNDQDR3FoVwBPJdh44nttOvUZpcabwfAeqOLYAV92qI9Ae1M4IRhZGb10s LY4A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=XZ/NcolO1x+fjhvMlQO1IVgCklyfQDJgDDRKZDaVX8Q=; b=kFEpy/Ri3GZiCE9h8X9H5YDBJf2NBYowVFw0pdkQdz49T3XzLYi3pngkdcoVhorStn 5cuEpTL/98rdLyoKvMcHTUSLDWkwQ1ClEpo9H0Yqn7Xx6WIXoDEQhWMdBU8HS1XfJVOh SKAixyRehzNAsk0LYQO2aNzSTPTOGKcUsVtCGRwy8eIloz2xxdEeuWUjPA0a97LhKmTj jkanNTwuRN+PfhZ1bh6zaUFHjdOmMYRFHyF2m1xS1yfgWBEa0ltB5jyJl3lg3Xrgn0Ko pTxtHGZ/wC4DI2XJW7fowgLkA7wDOWHMbhCQYtQmoILMXXZxeUAb0W2yZhzDiKkLKJOf NkqQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d92-v6si137253pld.383.2018.02.16.10.52.52; Fri, 16 Feb 2018 10:53:06 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755390AbeBPI4d (ORCPT + 99 others); Fri, 16 Feb 2018 03:56:33 -0500 Received: from mail-wm0-f51.google.com ([74.125.82.51]:53906 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755351AbeBPI4a (ORCPT ); Fri, 16 Feb 2018 03:56:30 -0500 Received: by mail-wm0-f51.google.com with SMTP id t74so1806544wme.3 for ; Fri, 16 Feb 2018 00:56:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=XZ/NcolO1x+fjhvMlQO1IVgCklyfQDJgDDRKZDaVX8Q=; b=L4/YKVe5r0Q79duL2dJ3oG4A5JKj6PMvuOxMM5U6oDK0UIiU1J38fobJJ3LBSNJLsp DvQnfZ58ponfGDv2x4iozGMnOWBadlG9rwbFATT9m6J8PmPtq0IPMgxg2MuZl2aMEfdt Mr59j0QoAmdzUbTt1soDY7fqw4cC5uUcD/z2hM8L7sK6U1Wc5gtAGmPhH6KZQA7Ybc3N j5d7jH8dkYAE2VqGzuqgh4f1PddtM7aYcCFCoc+k+0vAtawxkJao7Q+1ecNf3n7P+vZ0 6xKfHZenmsl22iRWPO0dKPw55EqWISnFrdsiHg0aHK+qd/kGosRvijoVOgdgpALMgyW9 4HZA== X-Gm-Message-State: APf1xPBinXvqobySJvmiVGr1OcF5/GI8Rliw1pDZ3pwFT2HcQi6tKUfl 99qyhzfmGVG+/+NeFoG70V+puQ== X-Received: by 10.80.207.10 with SMTP id c10mr6956773edk.133.1518771389402; Fri, 16 Feb 2018 00:56:29 -0800 (PST) Received: from shalem.localdomain (546A5441.cm-12-3b.dynamic.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id r47sm11733027edd.24.2018.02.16.00.56.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Feb 2018 00:56:28 -0800 (PST) Subject: Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version To: Brian Norris Cc: Marcel Holtmann , Gustavo Padovan , Johan Hedberg , linux-bluetooth@vger.kernel.org, linux-serial@vger.kernel.org, linux-acpi@vger.kernel.org, stable@vger.kernel.org, Leif Liddy , Matthias Kaehlcke , Daniel Drake , Kai-Heng Feng , matadeen@qti.qualcomm.com, linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Guenter Roeck References: <20180108094416.4789-1-hdegoede@redhat.com> <20180213022455.GA151190@rodete-desktop-imager.corp.google.com> <8cd918fd-bf6f-70ac-e561-e7deffa695f0@redhat.com> <20180216022721.GA69988@rodete-desktop-imager.corp.google.com> From: Hans de Goede Message-ID: <345b0de8-1a23-d2f8-bc56-507eadf7faa7@redhat.com> Date: Fri, 16 Feb 2018 09:56:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180216022721.GA69988@rodete-desktop-imager.corp.google.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 16-02-18 03:27, Brian Norris wrote: > Hi Hans, > > On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote: >> On 13-02-18 03:24, Brian Norris wrote: >>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>> >>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>> has several issues (see the original commit message). An added advantage >>>> of moving over to the USB-core reset-resume handling is that it also >>>> disables autosuspend for these devices, which is similarly broken on these. >>> >>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>> think so -- I'm using one now. >> >> And have you manually enabled USB autosuspend for it, or are you >> running something which might have done so, e.g. powertop --auto-tune ? >> >> Because if you did not do that then you're already not using autosuspend >> for your QCA devices and this patch will change nothing. > > I use a set of udev rules that manually whitelist devices for > autosuspend. You can see it here: > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > You'll find at least one Rome chip in there. > >>> Thus, this is a poor solution, which >>> negatively affects my systems. However, I see that this patch was >>> applied regardless... >> >> Note that there already is a quirk to handle broken suspend/resume >> behavior on ALL QCA devices in older kernels. Also note that the > > Yes, and the quirk was broken, and I made sure it got reverted when it > broke my devices ;) Note that the revert for this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15 Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens in btusb_open(), hence firmware loading gets interrupted." And says: "If we really want to reset the USB device, we need to do it before btusb_open(). Let's handle it in drivers/usb/core/quirks.c." It does not mention that the quirk is not necessary on some devices only that the implementation of it we had before had issues. And the original commit of the quirk: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 Says: "There's been numerous reported instances where BTUSB_QCA_ROME bluetooth controllers stop functioning upon resume from suspend." So it may be platform specific but it is not just limited to 1 or 2 platforms I think. Note I'm not saying that I don't believe you that the quirk is not necessary on your devices. >> patches series which this commit builds on top of was already >> setting USB_QUIRK_RESET_RESUME for some devices in >> usb/core/quirks.c. >> >> All my commit does is instead of duplicating all the QCA USB-ids in >> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >> to btusb.c so that we don't need to duplicate the USB-id tables. > > I was slightly more OK with marking specific IDs as broken, because > those IDs didn't happen to be ones that I knew were currently working. > Now you're breaking my systems again. But this time, it's more subtle > because bluetooth will still work, but we just suck more power leaving > our USB port active all the time. I see, sorry about that. Ok, so we are going to need to make the reset-on-resume quirking more fine grained. I can see 3 ways to do this: 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the usb_device_id table inside btusb.c (I still believe duplicating most ids to usb/core/quirks.c is a bad idea). 2) Use dmi based whitelisting to opt out of reset-resume behavior on QCA btusb devices. 3) Use dmi based blacklisting which enables reset-resume behavior. In retrospect I guess 3 would have been best, but if we do that now it will cause regressions. I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the QCA usb-ids except for the ones where you know things work and which only seem to be used in working devices (based on you not having objections against the additions of the quirk for some ids to drivers/usb/core/quirks.c). And if then those usb-ids do turn out to have broken suspend on on some devices too I guess we need to move to 2. >> The result of the combination of these patches is that the custom >> DIY reset on resume handling btusb.c was doing is now replaced >> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. >> >> As a (desirable) side effect this also disables USB autosuspend >> for QCA devices since the USB-core does not allow USB autosuspend >> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown >> this to be necessary on at least some QCA devices and given that >> these devices tend to loose there firmware on a suspend, it seems >> sensible to not allow autosuspend on them. > > But you're not accurately targeting the "some". AFAICT, you're wasting > power on my system. OK, so it turns out that this is a platform problem, which was not known to those involved until now. As mentioned before solution 3. from my above list of solutions would then be ideal, but I don't see how we can get there now without causing regressions where bluetooth stops working completely for people. Regards, Hans