Re: [RFC] Bluetooth: Convert MSFT filter HCI cmd to hci_sync

["Gix, Brian" <brian.gix@intel.com>]

Hi Luiz,

On Mon, 2022-10-31 at 16:25 -0700, Luiz Augusto von Dentz wrote:
> Hi Brian,
> 
> On Mon, Oct 31, 2022 at 3:02 PM Brian Gix <brian.gix@intel.com>
> wrote:
> > 
> > The msft_set_filter_enable() command was using the deprecated
> > hci_request mechanism rather than hci_sync. This caused the warning
> > error:
> > hci0: HCI_REQ-0xfcf0
> > 
> > Signed-off-by: Brian Gix <brian.gix@intel.com>
> > ---
> >  net/bluetooth/msft.c | 36 +++++++++++-------------------------
> >  1 file changed, 11 insertions(+), 25 deletions(-)
> > 
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index bee6a4c656be..bf5cee48916c 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -743,17 +743,12 @@ __u64 msft_get_features(struct hci_dev *hdev)
> >  }
> > 
> >  static void msft_le_set_advertisement_filter_enable_cb(struct
> > hci_dev *hdev,
> > -                                                      u8 status,
> > u16 opcode,
> > -                                                      struct
> > sk_buff *skb)
> > +                                                      void
> > *user_data,
> > +                                                      u8 status)
> >  {
> > -       struct msft_cp_le_set_advertisement_filter_enable *cp;
> > -       struct msft_rp_le_set_advertisement_filter_enable *rp;
> > +       struct msft_cp_le_set_advertisement_filter_enable *cp =
> > user_data;
> >         struct msft_data *msft = hdev->msft_data;
> > 
> > -       rp = (struct msft_rp_le_set_advertisement_filter_enable
> > *)skb->data;
> > -       if (skb->len < sizeof(*rp))
> > -               return;
> > -
> >         /* Error 0x0C would be returned if the filter enabled
> > status is
> >          * already set to whatever we were trying to set.
> >          * Although the default state should be disabled, some
> > controller set
> > @@ -766,7 +761,6 @@ static void
> > msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
> > 
> >         hci_dev_lock(hdev);
> > 
> > -       cp = hci_sent_cmd_data(hdev, hdev->msft_opcode);
> >         msft->filter_enabled = cp->enable;
> > 
> >         if (status == 0x0C)
> > @@ -804,31 +798,23 @@ int msft_remove_monitor(struct hci_dev *hdev,
> > struct adv_monitor *monitor)
> >         return msft_remove_monitor_sync(hdev, monitor);
> >  }
> > 
> > -void msft_req_add_set_filter_enable(struct hci_request *req, bool
> > enable)
> > -{
> > -       struct hci_dev *hdev = req->hdev;
> > -       struct msft_cp_le_set_advertisement_filter_enable cp;
> > -
> > -       cp.sub_opcode = MSFT_OP_LE_SET_ADVERTISEMENT_FILTER_ENABLE;
> > -       cp.enable = enable;
> > -
> > -       hci_req_add(req, hdev->msft_opcode, sizeof(cp), &cp);
> > -}
> > -
> >  int msft_set_filter_enable(struct hci_dev *hdev, bool enable)
> >  {
> > -       struct hci_request req;
> > +       struct msft_cp_le_set_advertisement_filter_enable cp;
> >         struct msft_data *msft = hdev->msft_data;
> >         int err;
> > 
> >         if (!msft)
> >                 return -EOPNOTSUPP;
> > 
> > -       hci_req_init(&req, hdev);
> > -       msft_req_add_set_filter_enable(&req, enable);
> > -       err = hci_req_run_skb(&req,
> > msft_le_set_advertisement_filter_enable_cb);
> > +       cp.sub_opcode = MSFT_OP_LE_SET_ADVERTISEMENT_FILTER_ENABLE;
> > +       cp.enable = enable;
> > +       err = __hci_cmd_sync_status(hdev, hdev->msft_opcode,
> > sizeof(cp), &cp,
> > +                                   HCI_CMD_TIMEOUT);
> > +
> > +       msft_le_set_advertisement_filter_enable_cb(hdev, &cp, err);
> 
> Except if this function is only supposed to be called for a safe
> context (which can block) we should actually use hci_cmd_sync_queue.

This call is made from msft_do_open() which has one call: 
from hci_dev_init_sync(), which also calls aosp_do_open().

From within aosp_do_open(), we are also calling __hci_cmd_sync(), and
if we assume that that has been tested and is working, then making what
is essentially the identical call here should be fine.

Otherwise, we should also recode aosp_do_open to use the
hci_cmd_sync_queue as well.


> > 
> > -       return err;
> > +       return 0;
> >  }
> > 
> >  bool msft_curve_validity(struct hci_dev *hdev)
> > --
> > 2.38.1
> > 
> 
>