linux-kernel - Re: [PATCH 3/4] Bluetooth: btusb: mediatek: reset the device as WMT failed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGp9LzrjmKDF_3+Km05eLVkr9ZHKhfMWjVxx=7GvsfRmWNp2dQ@mail.gmail.com>
Date:   Wed, 14 Sep 2022 18:57:21 -0700
From:   Sean Wang <sean.wang@...nel.org>
To:     Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc:     sean.wang@...iatek.com, marcel@...tmann.org,
        johan.hedberg@...il.com, Soul.Huang@...iatek.com,
        YN.Chen@...iatek.com, Leon.Yen@...iatek.com,
        Eric-SY.Chang@...iatek.com, Deren.Wu@...iatek.com,
        km.lin@...iatek.com, robin.chiu@...iatek.com,
        Eddie.Chen@...iatek.com, ch.yeh@...iatek.com,
        posh.sun@...iatek.com, ted.huang@...iatek.com,
        Stella.Chang@...iatek.com, Tom.Chou@...iatek.com,
        steve.lee@...iatek.com, jsiuda@...gle.com, frankgor@...gle.com,
        abhishekpandit@...gle.com, michaelfsun@...gle.com,
        abhishekpandit@...omium.org, mcchou@...omium.org,
        shawnku@...gle.com, linux-bluetooth@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] Bluetooth: btusb: mediatek: reset the device as WMT failed

Hi Luiz,

On Wed, Sep 14, 2022 at 3:46 PM Luiz Augusto von Dentz
<luiz.dentz@...il.com> wrote:
>
> Hi Sean,
>
> On Mon, Sep 12, 2022 at 3:18 PM <sean.wang@...iatek.com> wrote:
> >
> > From: Sean Wang <sean.wang@...iatek.com>
> >
> > Reset the BT device whenever the driver detected any WMT failure happened
> > to recover such kind of system-level error as soon as possible.
> >
> > Signed-off-by: Sean Wang <sean.wang@...iatek.com>
> > ---
> >  drivers/bluetooth/btusb.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 653f57a98233..dc86726c8271 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2576,6 +2576,10 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev,
> >         data->evt_skb = NULL;
> >  err_free_wc:
> >         kfree(wc);
> > +
> > +       if (err < 0)
> > +               btmtk_reset_sync(hdev);
>
> Doesn't reset itself can fail?

The reset is supposed not to fail so there is no return value is
designated in the function

>
> >         return err;
>
> It would probably be better to reset on error at the caller IMO, also
> in case it fails during firmware upload does reset even work? Also it

The reset is supposed to work even without the firmware uploaded but I
need to have further confirmation with fw folks to ensure this point.
Anyway, I will try to move the reset on the error at the caller or
based on the context in the next version because I thought again that
will also help
working out a patch to recover any error present at firmware
initialization that the driver currently cannot handle and the patch
cannot cover.

> would probably have been better to have its own file for vendor
> specific commands like this and use btmtk_ prefix as well.

I had tried to move btusb_mtk_hci_wmt_sync to btmtk.c to allow it to
be reused by all mtk bluetooth drivers but some reason stopped me from
doing that.
that is btusb_mtk_hci_wmt_sync has the reference to the data bundled
with btusb.c and it seemed a bit harder for me to split out from
btusb.c for the moment,
such as btusb data->flag the function will refer to and is shared by
all vendors, so I still temporarily leave the vendor-specific commands
there.
I think that would be easy to do if btusb.c can support a pointer in
struct btusb_data pointed to the vendor-specific data area where I can
put the flag and other
vendor-specific stuff the btmtk.c needed there.

             Sean
>
> >  }
> >
> > --
> > 2.25.1
> >
>
>
> --
> Luiz Augusto von Dentz