From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: Marcel Holtmann <marcel@holtmann.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Fabien Parent <fparent@baylibre.com>,
Sean Wang <sean.wang@mediatek.com>,
"open list:BLUETOOTH SUBSYSTEM" <linux-bluetooth@vger.kernel.org>,
"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] Bluetooth: Shutdown controller after workqueues are flushed or cancelled
Date: Thu, 05 Aug 2021 08:55:01 +0200 [thread overview]
Message-ID: <87bl6cnzy2.fsf@baylibre.com> (raw)
In-Reply-To: <CAAd53p6T_K67CPthLPObF=OWWCEChW4pMFMwuq87qWmTmzP2VA@mail.gmail.com>
Hi Kai-Heng,
Thanks for your patch,
Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
> On Tue, Aug 3, 2021 at 4:21 PM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Kai-Heng,
>>
>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
>>
>> > Hi Mattijs,
>> >
>> > On Fri, Jul 30, 2021 at 7:40 PM Mattijs Korpershoek
>> > <mkorpershoek@baylibre.com> wrote:
>> >>
>> >> Hi Kai-Heng,
>> >
>> > [snipped]
>> >
>> >> Thank you for your help. Sorry I did not post the logs previously.
>> >>
>> >> dmesg: https://pastebin.com/tpWDNyQr
>> >> ftrace on btmtksdio: https://pastebin.com/jmhvmwUw
>> >
>> > Seems like btmtksdio needs shudown() to be called before flush().
>> > Since the order was there for a very long time, changing the calling
>> > order indeed can break what driver expects.
>> > Can you please test the following patch:
>> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> > index 2560ed2f144d..a61e610a400c 100644
>> > --- a/net/bluetooth/hci_core.c
>> > +++ b/net/bluetooth/hci_core.c
>> > @@ -1785,6 +1785,14 @@ int hci_dev_do_close(struct hci_dev *hdev)
>> > aosp_do_close(hdev);
>> > msft_do_close(hdev);
>> >
>> > + if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
>> > + !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
>> > + test_bit(HCI_UP, &hdev->flags)) {
>> > + /* Execute vendor specific shutdown routine */
>> > + if (hdev->shutdown)
>> > + hdev->shutdown(hdev);
>> > + }
>> > +
>> > if (hdev->flush)
>> > hdev->flush(hdev);
>> >
>> > @@ -1798,14 +1806,6 @@ int hci_dev_do_close(struct hci_dev *hdev)
>> > clear_bit(HCI_INIT, &hdev->flags);
>> > }
>> >
>> > - if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
>> > - !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
>> > - test_bit(HCI_UP, &hdev->flags)) {
>> > - /* Execute vendor specific shutdown routine */
>> > - if (hdev->shutdown)
>> > - hdev->shutdown(hdev);
>> > - }
>> > -
>> > /* flush cmd work */
>> > flush_work(&hdev->cmd_work);
>>
>> Thanks for the patch and your help.
>> I've tried it, but it seems that it does not improve for me.
>> I'm still observing:
>>
>> i500-pumpkin login: root
>> root@i500-pumpkin:~# hciconfig hci0 up
>> Can't init device hci0: Connection timed out (110)
>>
>> Logs for this session:
>> dmesg: https://pastebin.com/iAFk5Tzi
>> ftrace: https://pastebin.com/kEMWSYrE
>
> Thanks for the testing!
> What about moving the shutdown() part right after hci_req_sync_lock()
> so tx/rx can still work:
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 2560ed2f144d4..be3113fb7d4b0 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1727,6 +1727,14 @@ int hci_dev_do_close(struct hci_dev *hdev)
> hci_request_cancel_all(hdev);
> hci_req_sync_lock(hdev);
>
> + if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> + !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> + test_bit(HCI_UP, &hdev->flags)) {
> + /* Execute vendor specific shutdown routine */
> + if (hdev->shutdown)
> + hdev->shutdown(hdev);
> + }
> +
> if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> cancel_delayed_work_sync(&hdev->cmd_timer);
> hci_req_sync_unlock(hdev);
> @@ -1798,14 +1806,6 @@ int hci_dev_do_close(struct hci_dev *hdev)
> clear_bit(HCI_INIT, &hdev->flags);
> }
>
> - if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> - !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> - test_bit(HCI_UP, &hdev->flags)) {
> - /* Execute vendor specific shutdown routine */
> - if (hdev->shutdown)
> - hdev->shutdown(hdev);
> - }
> -
> /* flush cmd work */
> flush_work(&hdev->cmd_work);
I confirm this diff works for me:
root@i500-pumpkin:~# hciconfig hci0 up
root@i500-pumpkin:~# hciconfig hci0 down
root@i500-pumpkin:~# hciconfig hci0 up
root@i500-pumpkin:~# hciconfig hci0
hci0: Type: Primary Bus: SDIO
BD Address: 00:0C:E7:55:FF:12 ACL MTU: 1021:8 SCO MTU: 244:4
UP RUNNING
RX bytes:11268 acl:0 sco:0 events:829 errors:0
TX bytes:182569 acl:0 sco:0 commands:829 errors:0
root@i500-pumpkin:~# hcitool scan
Scanning ...
<redacted> Pixel 3 XL
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
>
>
>
>
>>
>>
>> >
>> > Kai-Heng
>> >
>> >>
>> >> Mattijs
>> >> >
>> >> > Kai-Heng
>> >> >
>> >> >>
>> >> >> Thanks,
>> >> >> Mattijs Korpershoek
>> >> >>
>> >> >>
>> >> >> >
>> >> >> > Regards
>> >> >> >
>> >> >> > Marcel
next prev parent reply other threads:[~2021-08-05 6:55 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-14 7:14 [PATCH v2] Bluetooth: Shutdown controller after workqueues are flushed or cancelled Kai-Heng Feng
2021-05-14 8:12 ` [v2] " bluez.test.bot
2021-05-14 18:15 ` [PATCH v2] " Marcel Holtmann
2021-07-28 13:50 ` Mattijs Korpershoek
2021-07-28 15:25 ` Kai-Heng Feng
2021-07-30 11:40 ` Mattijs Korpershoek
2021-08-03 6:42 ` Kai-Heng Feng
2021-08-03 8:21 ` Mattijs Korpershoek
2021-08-04 14:42 ` Kai-Heng Feng
2021-08-05 6:55 ` Mattijs Korpershoek [this message]
2021-08-05 15:50 ` Kai-Heng Feng
2021-08-06 8:51 ` Mattijs Korpershoek
2021-08-06 15:36 ` Kai-Heng Feng
2021-08-09 9:19 ` Mattijs Korpershoek
[not found] ` <20210802030538.2023-1-hdanton@sina.com>
2021-08-03 6:45 ` Kai-Heng Feng
[not found] ` <20210803074722.2383-1-hdanton@sina.com>
2021-08-04 14:35 ` Kai-Heng Feng
[not found] ` <20210805030024.2603-1-hdanton@sina.com>
2021-08-05 3:44 ` Kai-Heng Feng
[not found] ` <20210805063536.2698-1-hdanton@sina.com>
2021-08-05 7:19 ` Kai-Heng Feng
2021-08-05 6:12 ` Hsin-Yi Wang
[not found] ` <20210805070114.2803-1-hdanton@sina.com>
2021-08-05 7:04 ` Hsin-Yi Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bl6cnzy2.fsf@baylibre.com \
--to=mkorpershoek@baylibre.com \
--cc=davem@davemloft.net \
--cc=fparent@baylibre.com \
--cc=johan.hedberg@gmail.com \
--cc=kai.heng.feng@canonical.com \
--cc=kuba@kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=sean.wang@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.