| Message ID | a432abf4cf95e93783864b27bafa53d45bdd5212.1663020936.git.objelf@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] Bluetooth: btusb: mediatek: use readx_poll_timeout instead of open coding | expand |
Il 13/09/22 00:18, sean.wang@mediatek.com ha scritto: > 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> This looks like a fix, so you probably want a Fixes tag for backport. Regards, Angelo > --- > 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); > + > return err; > } >
Hi, Angelo On Tue, Sep 13, 2022 at 1:23 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 13/09/22 00:18, sean.wang@mediatek.com ha scritto: > > 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> > > This looks like a fix, so you probably want a Fixes tag for backport. I didn't add the fix tag because there is not a previous patch that had issues the patch needs to fix. It would be looking more like an enhancement patch for me to fix up the potential issue happening in the firmware where the existing driver cannot detect and recover in time with .cmd_timeout callback but actually, the kind of potential issue in firmware I was worried about in the firmware didn't happen or being reported so far. Sean > > Regards, > Angelo > > > --- > > 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); > > + > > return err; > > } > > >
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? > 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 would probably have been better to have its own file for vendor specific commands like this and use btmtk_ prefix as well. > } > > -- > 2.25.1 >
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
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); + return err; }