Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210128120007.5676-1-hdegoede@redhat.com>
In-Reply-To: <20210128120007.5676-1-hdegoede@redhat.com>
From:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date:   Fri, 29 Jan 2021 09:49:25 -0800
Message-ID: <CANFp7mX15xLVLnuTCOijNL6uTwiud-q+2Y2o9=GnasD=Y5LMdg@mail.gmail.com>
Subject: Re: Recent(ish) bluetooth hci_suspend_notifier() changes
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Bluez mailing list <linux-bluetooth@vger.kernel.org>,
        Alain Michaud <alainm@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Hans,

Thanks for the pointers. I'll try to spin up a patch to fix these
issues (or ask for some help on my team).

Thanks,
Abhishek

On Thu, Jan 28, 2021 at 4:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> While debugging an rtl8723bs bluetooth issue I noticed that last year
> the bluetooth core has grown a new hci_suspend_notifier() mechanism and
> I noticed a number of possible issues / improvements with that mechanism
> which I noticed:
>
> 1. HCI_OP_WRITE_SCAN_ENABLE gets send to the HCI in some places without
>    a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
>
> 2. HCI_OP_SET_EVENT_FLT gets end to the HCI in some places without
>    a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
>
> 3. hci_req_set_event_filter sends the following requests:
>    1 HCI_OP_SET_EVENT_FLT
>    2 HCI_OP_WRITE_SCAN_ENABLE (if the scan parameters have changed only)
>    3 HCI_OP_SET_EVENT_FLT (if their are relevant whitelist entries_
>    4 HCI_OP_WRITE_SCAN_ENABLE unconditionally
>
>    It seems to me that 2. is unnecessary since it will immediately be
>    followed by 4. and 4. misses the check to see if the scan parameters
>    need updating which 2 does (this check is done in __hci_req_update_scan()
>
>
> 4. There is another unconditional, possibly unnecessary  HCI_OP_WRITE_SCAN_ENABLE
>    in the if (next == BT_SUSPEND_DISCONNECT) {} block of hci_req_prepare_suspend()
>    Note if this is made conditional then the:
>     set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
>    should also be made conditional since then req might not contain any requests
>    in which case suspend_req_complete() will not run.
>
> Regards,
>
> Hans
>