diff mbox series

[v2] Bluetooth: Call shutdown for HCI_USER_CHANNEL

Message ID 20220926164358.v2.1.Ic8eabc8ed89a07c3d52726dd017539069faac6c4@changeid
State New
Headers show
Series [v2] Bluetooth: Call shutdown for HCI_USER_CHANNEL | expand

Commit Message

Abhishek Pandit-Subedi Sept. 26, 2022, 11:44 p.m. UTC
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Some drivers depend on shutdown being called for proper operation.
Unset HCI_USER_CHANNEL and call the full close routine since shutdown is
complementary to setup.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Using hci_qca, we can get the controller into a bad state simply by
trying to bind to userchannel twice (open+bind+close, then open+bind).
Without running the shutdown routine, the device seems to get into a bad
state. A similar bug also occurs with btmtksdio (using MT7921).

This change properly runs the shutdown routine, which should be
complementary to setup. The reason it unsets the HCI_USER_CHANNEL flag
is that some drivers have complex operations in their shutdown routine
(including sending hci packets) and we need to support the normal data
path for them (including cmd_timeout + recovery mechanisms).

Note for v2: I've gotten a chance to test this on more devices
and figure out why it wasn't working before in v1. I found two problems:
I had a signal pending (SIGTERM) that was messing things up in the
socket release function and the HCI_USER_CHANNEL flag was preventing
hci_sync from operating properly during shutdown on Intel chipsets
(which use the sync functions to send a reset command + other commands
sometimes).

This was tested with hci_qca (QCA6174-A-3), btmtksdio (MT7921-SDIO)
and btusb (with AX200).


Changes in v2:
- Clear HCI_USER_CHANNEL flag at start of close and restore at end.
- Add comment explaning why we need to clear flag and run shutdown.

 net/bluetooth/hci_sync.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Luiz Augusto von Dentz Sept. 27, 2022, 12:10 a.m. UTC | #1
Hi Abhishek,

On Mon, Sep 26, 2022 at 4:44 PM Abhishek Pandit-Subedi
<abhishekpandit@google.com> wrote:
>
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> Some drivers depend on shutdown being called for proper operation.
> Unset HCI_USER_CHANNEL and call the full close routine since shutdown is
> complementary to setup.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Using hci_qca, we can get the controller into a bad state simply by
> trying to bind to userchannel twice (open+bind+close, then open+bind).
> Without running the shutdown routine, the device seems to get into a bad
> state. A similar bug also occurs with btmtksdio (using MT7921).
>
> This change properly runs the shutdown routine, which should be
> complementary to setup. The reason it unsets the HCI_USER_CHANNEL flag
> is that some drivers have complex operations in their shutdown routine
> (including sending hci packets) and we need to support the normal data
> path for them (including cmd_timeout + recovery mechanisms).
>
> Note for v2: I've gotten a chance to test this on more devices
> and figure out why it wasn't working before in v1. I found two problems:
> I had a signal pending (SIGTERM) that was messing things up in the
> socket release function and the HCI_USER_CHANNEL flag was preventing
> hci_sync from operating properly during shutdown on Intel chipsets
> (which use the sync functions to send a reset command + other commands
> sometimes).
>
> This was tested with hci_qca (QCA6174-A-3), btmtksdio (MT7921-SDIO)
> and btusb (with AX200).
>
>
> Changes in v2:
> - Clear HCI_USER_CHANNEL flag at start of close and restore at end.
> - Add comment explaning why we need to clear flag and run shutdown.
>
>  net/bluetooth/hci_sync.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 422f7c6911d9..f9591fcefb8d 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -4731,9 +4731,18 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>  {
>         bool auto_off;
>         int err = 0;
> +       bool was_userchannel;
>
>         bt_dev_dbg(hdev, "");
>
> +       /* Similar to how we first do setup and then set the exclusive access
> +        * bit for userspace, we must first unset userchannel and then clean up.
> +        * Otherwise, the kernel can't properly use the hci channel to clean up
> +        * the controller (some shutdown routines require sending additional
> +        * commands to the controller for example).
> +        */
> +       was_userchannel = hci_dev_test_and_clear_flag(hdev, HCI_USER_CHANNEL);
> +
>         cancel_delayed_work(&hdev->power_off);
>         cancel_delayed_work(&hdev->ncmd_timer);
>         cancel_delayed_work(&hdev->le_scan_disable);
> @@ -4747,7 +4756,6 @@ int hci_dev_close_sync(struct hci_dev *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)

I guess the idea here is that shutdown can be run without the
HCI_USER_CHANNEL flag since the hdev is closing we don't expect any
traffic from socket/user channel? In that case I'd probably suggest
having this on its own function e.g. hci_dev_shutdown which can have
the logic of resetting the flag and restoring at the end. Also it is
probably a good idea to have some test mimicking this behavior on
userchan-tester so we do not accidentally break it.

> @@ -4756,6 +4764,8 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>
>         if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
>                 cancel_delayed_work_sync(&hdev->cmd_timer);
> +               if (was_userchannel)
> +                       hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
>                 return err;
>         }
>
> @@ -4795,7 +4805,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>         auto_off = hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF);
>
>         if (!auto_off && hdev->dev_type == HCI_PRIMARY &&
> -           !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> +           !was_userchannel &&
>             hci_dev_test_flag(hdev, HCI_MGMT))
>                 __mgmt_power_off(hdev);
>
> @@ -4808,7 +4818,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>
>         hci_sock_dev_event(hdev, HCI_DEV_DOWN);
>
> -       if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> +       if (!was_userchannel)
>                 aosp_do_close(hdev);
>                 msft_do_close(hdev);
>         }
> @@ -4858,6 +4868,9 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>         memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
>         bacpy(&hdev->random_addr, BDADDR_ANY);
>
> +       if (was_userchannel)
> +               hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
> +
>         hci_dev_put(hdev);
>         return err;
>  }
> --
> 2.37.3.998.g577e59143f-goog
>
Abhishek Pandit-Subedi Sept. 27, 2022, 12:20 a.m. UTC | #2
On Mon, Sep 26, 2022 at 5:10 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Mon, Sep 26, 2022 at 4:44 PM Abhishek Pandit-Subedi
> <abhishekpandit@google.com> wrote:
> >
> > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >
> > Some drivers depend on shutdown being called for proper operation.
> > Unset HCI_USER_CHANNEL and call the full close routine since shutdown is
> > complementary to setup.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Using hci_qca, we can get the controller into a bad state simply by
> > trying to bind to userchannel twice (open+bind+close, then open+bind).
> > Without running the shutdown routine, the device seems to get into a bad
> > state. A similar bug also occurs with btmtksdio (using MT7921).
> >
> > This change properly runs the shutdown routine, which should be
> > complementary to setup. The reason it unsets the HCI_USER_CHANNEL flag
> > is that some drivers have complex operations in their shutdown routine
> > (including sending hci packets) and we need to support the normal data
> > path for them (including cmd_timeout + recovery mechanisms).
> >
> > Note for v2: I've gotten a chance to test this on more devices
> > and figure out why it wasn't working before in v1. I found two problems:
> > I had a signal pending (SIGTERM) that was messing things up in the
> > socket release function and the HCI_USER_CHANNEL flag was preventing
> > hci_sync from operating properly during shutdown on Intel chipsets
> > (which use the sync functions to send a reset command + other commands
> > sometimes).
> >
> > This was tested with hci_qca (QCA6174-A-3), btmtksdio (MT7921-SDIO)
> > and btusb (with AX200).
> >
> >
> > Changes in v2:
> > - Clear HCI_USER_CHANNEL flag at start of close and restore at end.
> > - Add comment explaning why we need to clear flag and run shutdown.
> >
> >  net/bluetooth/hci_sync.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 422f7c6911d9..f9591fcefb8d 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -4731,9 +4731,18 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >  {
> >         bool auto_off;
> >         int err = 0;
> > +       bool was_userchannel;
> >
> >         bt_dev_dbg(hdev, "");
> >
> > +       /* Similar to how we first do setup and then set the exclusive access
> > +        * bit for userspace, we must first unset userchannel and then clean up.
> > +        * Otherwise, the kernel can't properly use the hci channel to clean up
> > +        * the controller (some shutdown routines require sending additional
> > +        * commands to the controller for example).
> > +        */
> > +       was_userchannel = hci_dev_test_and_clear_flag(hdev, HCI_USER_CHANNEL);
> > +
> >         cancel_delayed_work(&hdev->power_off);
> >         cancel_delayed_work(&hdev->ncmd_timer);
> >         cancel_delayed_work(&hdev->le_scan_disable);
> > @@ -4747,7 +4756,6 @@ int hci_dev_close_sync(struct hci_dev *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)
>
> I guess the idea here is that shutdown can be run without the
> HCI_USER_CHANNEL flag since the hdev is closing we don't expect any
> traffic from socket/user channel? In that case I'd probably suggest
> having this on its own function e.g. hci_dev_shutdown which can have
> the logic of resetting the flag and restoring at the end. Also it is
> probably a good idea to have some test mimicking this behavior on
> userchan-tester so we do not accidentally break it.

Yup, that sounds reasonable. I'll look into userchan-tester before
sending up a v3.

>
> > @@ -4756,6 +4764,8 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >
> >         if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> >                 cancel_delayed_work_sync(&hdev->cmd_timer);
> > +               if (was_userchannel)
> > +                       hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
> >                 return err;
> >         }
> >
> > @@ -4795,7 +4805,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >         auto_off = hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF);
> >
> >         if (!auto_off && hdev->dev_type == HCI_PRIMARY &&
> > -           !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> > +           !was_userchannel &&
> >             hci_dev_test_flag(hdev, HCI_MGMT))
> >                 __mgmt_power_off(hdev);
> >
> > @@ -4808,7 +4818,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >
> >         hci_sock_dev_event(hdev, HCI_DEV_DOWN);
> >
> > -       if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> > +       if (!was_userchannel)
> >                 aosp_do_close(hdev);
> >                 msft_do_close(hdev);
> >         }
> > @@ -4858,6 +4868,9 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >         memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
> >         bacpy(&hdev->random_addr, BDADDR_ANY);
> >
> > +       if (was_userchannel)
> > +               hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
> > +
> >         hci_dev_put(hdev);
> >         return err;
> >  }
> > --
> > 2.37.3.998.g577e59143f-goog
> >
>
>
> --
> Luiz Augusto von Dentz
kernel test robot Sept. 27, 2022, 8:04 a.m. UTC | #3
Hi Abhishek,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master net-next/master net/master linus/master v6.0-rc7 next-20220923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/Bluetooth-Call-shutdown-for-HCI_USER_CHANNEL/20220927-120257
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: parisc-allyesconfig
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0c60eab323f044b8bc6ab401d691e1d47c117bbd
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Abhishek-Pandit-Subedi/Bluetooth-Call-shutdown-for-HCI_USER_CHANNEL/20220927-120257
        git checkout 0c60eab323f044b8bc6ab401d691e1d47c117bbd
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/bluetooth/hci_sync.c: In function 'hci_dev_close_sync':
>> net/bluetooth/hci_sync.c:4821:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    4821 |         if (!was_userchannel)
         |         ^~
   net/bluetooth/hci_sync.c:4823:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    4823 |                 msft_do_close(hdev);
         |                 ^~~~~~~~~~~~~
   net/bluetooth/hci_sync.c: At top level:
   net/bluetooth/hci_sync.c:4826:9: error: expected identifier or '(' before 'if'
    4826 |         if (hdev->flush)
         |         ^~
   net/bluetooth/hci_sync.c:4830:25: error: expected declaration specifiers or '...' before '&' token
    4830 |         skb_queue_purge(&hdev->cmd_q);
         |                         ^
   net/bluetooth/hci_sync.c:4831:20: error: expected declaration specifiers or '...' before '&' token
    4831 |         atomic_set(&hdev->cmd_cnt, 1);
         |                    ^
   net/bluetooth/hci_sync.c:4831:36: error: expected declaration specifiers or '...' before numeric constant
    4831 |         atomic_set(&hdev->cmd_cnt, 1);
         |                                    ^
   net/bluetooth/hci_sync.c:4832:9: error: expected identifier or '(' before 'if'
    4832 |         if (test_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks) &&
         |         ^~
   net/bluetooth/hci_sync.c:4840:20: error: expected declaration specifiers or '...' before '&' token
    4840 |         flush_work(&hdev->cmd_work);
         |                    ^
   net/bluetooth/hci_sync.c:4843:25: error: expected declaration specifiers or '...' before '&' token
    4843 |         skb_queue_purge(&hdev->rx_q);
         |                         ^
   net/bluetooth/hci_sync.c:4844:25: error: expected declaration specifiers or '...' before '&' token
    4844 |         skb_queue_purge(&hdev->cmd_q);
         |                         ^
   net/bluetooth/hci_sync.c:4845:25: error: expected declaration specifiers or '...' before '&' token
    4845 |         skb_queue_purge(&hdev->raw_q);
         |                         ^
   net/bluetooth/hci_sync.c:4848:9: error: expected identifier or '(' before 'if'
    4848 |         if (hdev->sent_cmd) {
         |         ^~
   net/bluetooth/hci_sync.c:4854:31: error: expected ')' before '&' token
    4854 |         clear_bit(HCI_RUNNING, &hdev->flags);
         |                               ^~
         |                               )
   net/bluetooth/hci_sync.c:4855:33: error: expected ')' before numeric constant
    4855 |         hci_sock_dev_event(hdev, HCI_DEV_CLOSE);
         |                                 ^
         |                                 )
   net/bluetooth/hci_sync.c:4858:13: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
    4858 |         hdev->close(hdev);
         |             ^~
   net/bluetooth/hci_sync.c:4861:13: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
    4861 |         hdev->flags &= BIT(HCI_RAW);
         |             ^~
   In file included from net/bluetooth/hci_sync.c:11:
   include/net/bluetooth/hci_core.h:827:9: error: expected identifier or '(' before 'do'
     827 |         do {                                                    \
         |         ^~
   net/bluetooth/hci_sync.c:4862:9: note: in expansion of macro 'hci_dev_clear_volatile_flags'
    4862 |         hci_dev_clear_volatile_flags(hdev);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/net/bluetooth/hci_core.h:833:11: error: expected identifier or '(' before 'while'
     833 |         } while (0)
         |           ^~~~~
   net/bluetooth/hci_sync.c:4862:9: note: in expansion of macro 'hci_dev_clear_volatile_flags'
    4862 |         hci_dev_clear_volatile_flags(hdev);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   net/bluetooth/hci_sync.c:4865:13: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
    4865 |         hdev->amp_status = AMP_STATUS_POWERED_DOWN;
         |             ^~
   net/bluetooth/hci_sync.c:4867:20: error: expected ')' before '->' token
    4867 |         memset(hdev->eir, 0, sizeof(hdev->eir));
         |                    ^~
         |                    )
   net/bluetooth/hci_sync.c:4868:20: error: expected ')' before '->' token
    4868 |         memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
         |                    ^~
         |                    )
   net/bluetooth/hci_sync.c:4869:15: error: expected declaration specifiers or '...' before '&' token
    4869 |         bacpy(&hdev->random_addr, BDADDR_ANY);
         |               ^
   In file included from net/bluetooth/hci_sync.c:10:
   include/net/bluetooth/bluetooth.h:341:21: error: expected declaration specifiers or '...' before '(' token
     341 | #define BDADDR_ANY  (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
         |                     ^
   net/bluetooth/hci_sync.c:4869:35: note: in expansion of macro 'BDADDR_ANY'
    4869 |         bacpy(&hdev->random_addr, BDADDR_ANY);
         |                                   ^~~~~~~~~~
   net/bluetooth/hci_sync.c:4871:9: error: expected identifier or '(' before 'if'
    4871 |         if (was_userchannel)
         |         ^~
>> net/bluetooth/hci_sync.c:4874:9: warning: data definition has no type or storage class
    4874 |         hci_dev_put(hdev);
         |         ^~~~~~~~~~~
   net/bluetooth/hci_sync.c:4874:9: error: type defaults to 'int' in declaration of 'hci_dev_put' [-Werror=implicit-int]
>> net/bluetooth/hci_sync.c:4874:9: warning: parameter names (without types) in function declaration
   net/bluetooth/hci_sync.c:4874:9: error: conflicting types for 'hci_dev_put'; have 'int()'
   include/net/bluetooth/hci_core.h:1422:20: note: previous definition of 'hci_dev_put' with type 'void(struct hci_dev *)'
    1422 | static inline void hci_dev_put(struct hci_dev *d)
         |                    ^~~~~~~~~~~
   net/bluetooth/hci_sync.c:4875:9: error: expected identifier or '(' before 'return'
    4875 |         return err;
         |         ^~~~~~
   net/bluetooth/hci_sync.c:4876:1: error: expected identifier or '(' before '}' token
    4876 | }
         | ^
   net/bluetooth/hci_sync.c: In function 'hci_dev_close_sync':
   net/bluetooth/hci_sync.c:4824:9: error: control reaches end of non-void function [-Werror=return-type]
    4824 |         }
         |         ^
   cc1: some warnings being treated as errors


vim +/if +4821 net/bluetooth/hci_sync.c

  4729	
  4730	int hci_dev_close_sync(struct hci_dev *hdev)
  4731	{
  4732		bool auto_off;
  4733		int err = 0;
  4734		bool was_userchannel;
  4735	
  4736		bt_dev_dbg(hdev, "");
  4737	
  4738		/* Similar to how we first do setup and then set the exclusive access
  4739		 * bit for userspace, we must first unset userchannel and then clean up.
  4740		 * Otherwise, the kernel can't properly use the hci channel to clean up
  4741		 * the controller (some shutdown routines require sending additional
  4742		 * commands to the controller for example).
  4743		 */
  4744		was_userchannel = hci_dev_test_and_clear_flag(hdev, HCI_USER_CHANNEL);
  4745	
  4746		cancel_delayed_work(&hdev->power_off);
  4747		cancel_delayed_work(&hdev->ncmd_timer);
  4748		cancel_delayed_work(&hdev->le_scan_disable);
  4749		cancel_delayed_work(&hdev->le_scan_restart);
  4750	
  4751		hci_request_cancel_all(hdev);
  4752	
  4753		if (hdev->adv_instance_timeout) {
  4754			cancel_delayed_work_sync(&hdev->adv_instance_expire);
  4755			hdev->adv_instance_timeout = 0;
  4756		}
  4757	
  4758		if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
  4759		    test_bit(HCI_UP, &hdev->flags)) {
  4760			/* Execute vendor specific shutdown routine */
  4761			if (hdev->shutdown)
  4762				err = hdev->shutdown(hdev);
  4763		}
  4764	
  4765		if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
  4766			cancel_delayed_work_sync(&hdev->cmd_timer);
  4767			if (was_userchannel)
  4768				hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
  4769			return err;
  4770		}
  4771	
  4772		hci_leds_update_powered(hdev, false);
  4773	
  4774		/* Flush RX and TX works */
  4775		flush_work(&hdev->tx_work);
  4776		flush_work(&hdev->rx_work);
  4777	
  4778		if (hdev->discov_timeout > 0) {
  4779			hdev->discov_timeout = 0;
  4780			hci_dev_clear_flag(hdev, HCI_DISCOVERABLE);
  4781			hci_dev_clear_flag(hdev, HCI_LIMITED_DISCOVERABLE);
  4782		}
  4783	
  4784		if (hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE))
  4785			cancel_delayed_work(&hdev->service_cache);
  4786	
  4787		if (hci_dev_test_flag(hdev, HCI_MGMT)) {
  4788			struct adv_info *adv_instance;
  4789	
  4790			cancel_delayed_work_sync(&hdev->rpa_expired);
  4791	
  4792			list_for_each_entry(adv_instance, &hdev->adv_instances, list)
  4793				cancel_delayed_work_sync(&adv_instance->rpa_expired_cb);
  4794		}
  4795	
  4796		/* Avoid potential lockdep warnings from the *_flush() calls by
  4797		 * ensuring the workqueue is empty up front.
  4798		 */
  4799		drain_workqueue(hdev->workqueue);
  4800	
  4801		hci_dev_lock(hdev);
  4802	
  4803		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
  4804	
  4805		auto_off = hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF);
  4806	
  4807		if (!auto_off && hdev->dev_type == HCI_PRIMARY &&
  4808		    !was_userchannel &&
  4809		    hci_dev_test_flag(hdev, HCI_MGMT))
  4810			__mgmt_power_off(hdev);
  4811	
  4812		hci_inquiry_cache_flush(hdev);
  4813		hci_pend_le_actions_clear(hdev);
  4814		hci_conn_hash_flush(hdev);
  4815		/* Prevent data races on hdev->smp_data or hdev->smp_bredr_data */
  4816		smp_unregister(hdev);
  4817		hci_dev_unlock(hdev);
  4818	
  4819		hci_sock_dev_event(hdev, HCI_DEV_DOWN);
  4820	
> 4821		if (!was_userchannel)
  4822			aosp_do_close(hdev);
  4823			msft_do_close(hdev);
  4824		}
  4825	
  4826		if (hdev->flush)
  4827			hdev->flush(hdev);
  4828	
  4829		/* Reset device */
  4830		skb_queue_purge(&hdev->cmd_q);
  4831		atomic_set(&hdev->cmd_cnt, 1);
  4832		if (test_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks) &&
  4833		    !auto_off && !hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
  4834			set_bit(HCI_INIT, &hdev->flags);
  4835			hci_reset_sync(hdev);
  4836			clear_bit(HCI_INIT, &hdev->flags);
  4837		}
  4838	
  4839		/* flush cmd  work */
  4840		flush_work(&hdev->cmd_work);
  4841	
  4842		/* Drop queues */
  4843		skb_queue_purge(&hdev->rx_q);
  4844		skb_queue_purge(&hdev->cmd_q);
  4845		skb_queue_purge(&hdev->raw_q);
  4846	
  4847		/* Drop last sent command */
  4848		if (hdev->sent_cmd) {
  4849			cancel_delayed_work_sync(&hdev->cmd_timer);
  4850			kfree_skb(hdev->sent_cmd);
  4851			hdev->sent_cmd = NULL;
  4852		}
  4853	
  4854		clear_bit(HCI_RUNNING, &hdev->flags);
  4855		hci_sock_dev_event(hdev, HCI_DEV_CLOSE);
  4856	
  4857		/* After this point our queues are empty and no tasks are scheduled. */
  4858		hdev->close(hdev);
  4859	
  4860		/* Clear flags */
  4861		hdev->flags &= BIT(HCI_RAW);
  4862		hci_dev_clear_volatile_flags(hdev);
  4863	
  4864		/* Controller radio is available but is currently powered down */
  4865		hdev->amp_status = AMP_STATUS_POWERED_DOWN;
  4866	
  4867		memset(hdev->eir, 0, sizeof(hdev->eir));
  4868		memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
  4869		bacpy(&hdev->random_addr, BDADDR_ANY);
  4870	
  4871		if (was_userchannel)
  4872			hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
  4873	
> 4874		hci_dev_put(hdev);
  4875		return err;
  4876	}
  4877
diff mbox series

Patch

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 422f7c6911d9..f9591fcefb8d 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4731,9 +4731,18 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 {
 	bool auto_off;
 	int err = 0;
+	bool was_userchannel;
 
 	bt_dev_dbg(hdev, "");
 
+	/* Similar to how we first do setup and then set the exclusive access
+	 * bit for userspace, we must first unset userchannel and then clean up.
+	 * Otherwise, the kernel can't properly use the hci channel to clean up
+	 * the controller (some shutdown routines require sending additional
+	 * commands to the controller for example).
+	 */
+	was_userchannel = hci_dev_test_and_clear_flag(hdev, HCI_USER_CHANNEL);
+
 	cancel_delayed_work(&hdev->power_off);
 	cancel_delayed_work(&hdev->ncmd_timer);
 	cancel_delayed_work(&hdev->le_scan_disable);
@@ -4747,7 +4756,6 @@  int hci_dev_close_sync(struct hci_dev *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)
@@ -4756,6 +4764,8 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 
 	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
 		cancel_delayed_work_sync(&hdev->cmd_timer);
+		if (was_userchannel)
+			hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
 		return err;
 	}
 
@@ -4795,7 +4805,7 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 	auto_off = hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF);
 
 	if (!auto_off && hdev->dev_type == HCI_PRIMARY &&
-	    !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+	    !was_userchannel &&
 	    hci_dev_test_flag(hdev, HCI_MGMT))
 		__mgmt_power_off(hdev);
 
@@ -4808,7 +4818,7 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 
 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
 
-	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+	if (!was_userchannel)
 		aosp_do_close(hdev);
 		msft_do_close(hdev);
 	}
@@ -4858,6 +4868,9 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 	memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
 	bacpy(&hdev->random_addr, BDADDR_ANY);
 
+	if (was_userchannel)
+		hci_dev_set_flag(hdev, HCI_USER_CHANNEL);
+
 	hci_dev_put(hdev);
 	return err;
 }