Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <43b68b1f48c20b1dfcd7e6663c3dcb38e4e0648c.1663020936.git.objelf@gmail.com>
 <a432abf4cf95e93783864b27bafa53d45bdd5212.1663020936.git.objelf@gmail.com> <CABBYNZ+Z+BApMOPEgVBxf6j0sTYFE0tH6Eab-hQW8FKVFqfvqA@mail.gmail.com>
In-Reply-To: <CABBYNZ+Z+BApMOPEgVBxf6j0sTYFE0tH6Eab-hQW8FKVFqfvqA@mail.gmail.com>
From:   Sean Wang <sean.wang@kernel.org>
Date:   Wed, 14 Sep 2022 18:57:21 -0700
Message-ID: <CAGp9LzrjmKDF_3+Km05eLVkr9ZHKhfMWjVxx=7GvsfRmWNp2dQ@mail.gmail.com>
Subject: Re: [PATCH 3/4] Bluetooth: btusb: mediatek: reset the device as WMT failed
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     sean.wang@mediatek.com, marcel@holtmann.org,
        johan.hedberg@gmail.com, Soul.Huang@mediatek.com,
        YN.Chen@mediatek.com, Leon.Yen@mediatek.com,
        Eric-SY.Chang@mediatek.com, Deren.Wu@mediatek.com,
        km.lin@mediatek.com, robin.chiu@mediatek.com,
        Eddie.Chen@mediatek.com, ch.yeh@mediatek.com,
        posh.sun@mediatek.com, ted.huang@mediatek.com,
        Stella.Chang@mediatek.com, Tom.Chou@mediatek.com,
        steve.lee@mediatek.com, jsiuda@google.com, frankgor@google.com,
        abhishekpandit@google.com, michaelfsun@google.com,
        abhishekpandit@chromium.org, mcchou@chromium.org,
        shawnku@google.com, linux-bluetooth@vger.kernel.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,

On Wed, Sep 14, 2022 at 3:46 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sean,
>
> On Mon, Sep 12, 2022 at 3:18 PM <sean.wang@mediatek.com> wrote:
> >
> > From: Sean Wang <sean.wang@mediatek.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@mediatek.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