Re: [PATCH v2 3/4] Bluetooth: btusb: mediatek: add intf release flow when usb disconnect

["Chris Lu (陸稚泓)" <Chris.Lu@mediatek.com>]

Hi Luiz,

On Fri, 2024-09-20 at 10:54 -0400, Luiz Augusto von Dentz wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Chris,
> 
> On Thu, Sep 19, 2024 at 3:49 AM Chris Lu <chris.lu@mediatek.com>
> wrote:
> >
> > MediaTek claim an special usb intr interface for ISO data
> transmission.
> > The interface need to be released before unregistering hci device
> when
> > usb disconnect. Removing BT usb dongle without properly releasing
> the
> > interface may cause Kernel panic while unregister hci device.
> >
> > Signed-off-by: Chris Lu <chris.lu@mediatek.com>
> > ---
> >  drivers/bluetooth/btusb.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index dfc42bdc8aaf..37e67b451b34 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2614,9 +2614,9 @@ static void btusb_mtk_claim_iso_intf(struct
> btusb_data *data)
> >         set_bit(BTMTK_ISOPKT_OVER_INTR, &btmtk_data->flags);
> >  }
> >
> > -static void btusb_mtk_release_iso_intf(struct btusb_data *data)
> > +static int btusb_mtk_release_iso_intf(struct hci_dev *hdev)
> >  {
> > -       struct btmtk_data *btmtk_data = hci_get_priv(data->hdev);
> > +       struct btmtk_data *btmtk_data = hci_get_priv(hdev);
> >
> >         if (btmtk_data->isopkt_intf) {
> >                 usb_kill_anchored_urbs(&btmtk_data->isopkt_anchor);
> > @@ -2630,6 +2630,8 @@ static void btusb_mtk_release_iso_intf(struct
> btusb_data *data)
> >         }
> >
> >         clear_bit(BTMTK_ISOPKT_OVER_INTR, &btmtk_data->flags);
> > +
> > +       return 0;
> >  }
> >
> >  static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
> > @@ -2649,7 +2651,7 @@ static int btusb_mtk_reset(struct hci_dev
> *hdev, void *rst_data)
> >                 return err;
> >
> >         if (test_bit(BTMTK_ISOPKT_RUNNING, &btmtk_data->flags))
> > -               btusb_mtk_release_iso_intf(data);
> > +               btusb_mtk_release_iso_intf(hdev);
> 
> We can probably move the check for BTMTK_ISOPKT_RUNNING into
> btusb_mtk_release_iso_intf to avoid having to duplicate it whenever
> calling btusb_mtk_release_iso_intf.
> 

Yes, condition check here is redundant, Keep the check in
btusb_mtk_release_iso_intf is better.

> >
> >         btusb_stop_traffic(data);
> >         usb_kill_anchored_urbs(&data->tx_anchor);
> > @@ -2703,14 +2705,13 @@ static int btusb_mtk_setup(struct hci_dev
> *hdev)
> >
> >  static int btusb_mtk_shutdown(struct hci_dev *hdev)
> >  {
> > -       struct btusb_data *data = hci_get_drvdata(hdev);
> >         struct btmtk_data *btmtk_data = hci_get_priv(hdev);
> >         int ret;
> >
> >         ret = btmtk_usb_shutdown(hdev);
> >
> >         if (test_bit(BTMTK_ISOPKT_RUNNING, &btmtk_data->flags))
> > -               btusb_mtk_release_iso_intf(data);
> > +               btusb_mtk_release_iso_intf(hdev);
> 
> Ditto.
> 
remove redundant check.

> >         return ret;
> >  }
> > @@ -3824,6 +3825,7 @@ static int btusb_probe(struct usb_interface
> *intf,
> >                 data->recv_acl = btmtk_usb_recv_acl;
> >                 data->suspend = btmtk_usb_suspend;
> >                 data->resume = btmtk_usb_resume;
> > +               data->disconnect = btusb_mtk_release_iso_intf;
> 
> I'd wrap (e.g. btmtk_usb_disconnect) the call to
> btusb_mtk_release_iso_intf since that is only meant to release the
> ISO
> endpoint.
> 
Add a new function "btusb_mtk_disconnect". Since this function will use
btusb_data which is defined in btusb.c only, I'll also define this
function in btusb.c.

Thanks for your advice. I'll submit v3 later.

Best regards,
Chris Lu

> >         }
> >
> >         if (id->driver_info & BTUSB_SWAVE) {
> > --
> > 2.18.0
> >
> 
> 
> -- 
> Luiz Augusto von Dentz