[v1] Bluetooth: qca: If memdump doesn't work, re-enable IBS

[PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Douglas Anderson 1 year, 8 months ago

On systems in the field, we are seeing this sometimes in the kernel logs:
  Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95

This means that _something_ decided that it wanted to get a memdump
but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).

The cleanup code in qca_controller_memdump() when we get back an error
from hci_devcd_init() undoes most things but forgets to clear
QCA_IBS_DISABLED. One side effect of this is that, during the next
suspend, qca_suspend() will always get a timeout.

Let's fix it so that we clear the bit.

Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I'm nowhere near an expert on this code so please give extra eyes on
this patch. I also have no idea how to reproduce the problem nor even
how to trigger a memdump to test it. I'd love any advice that folks
could give. ;-)

 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 0c9c9ee56592..1ef12f5a115d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1090,6 +1090,7 @@ static void qca_controller_memdump(struct work_struct *work)
 				qca->memdump_state = QCA_MEMDUMP_COLLECTED;
 				cancel_delayed_work(&qca->ctrl_memdump_timeout);
 				clear_bit(QCA_MEMDUMP_COLLECTION, &qca->flags);
+				clear_bit(QCA_IBS_DISABLED, &qca->flags);
 				mutex_unlock(&qca->hci_memdump_lock);
 				return;
 			}
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Doug Anderson 1 year, 8 months ago

Hi,

On Fri, May 17, 2024 at 5:03 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> On systems in the field, we are seeing this sometimes in the kernel logs:
>   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
>
> This means that _something_ decided that it wanted to get a memdump
> but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
>
> The cleanup code in qca_controller_memdump() when we get back an error
> from hci_devcd_init() undoes most things but forgets to clear
> QCA_IBS_DISABLED. One side effect of this is that, during the next
> suspend, qca_suspend() will always get a timeout.
>
> Let's fix it so that we clear the bit.
>
> Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I'm nowhere near an expert on this code so please give extra eyes on
> this patch. I also have no idea how to reproduce the problem nor even
> how to trigger a memdump to test it. I'd love any advice that folks
> could give. ;-)
>
>  drivers/bluetooth/hci_qca.c | 1 +
>  1 file changed, 1 insertion(+)

Totally fine if you just need more time, but I wanted to follow up and
check to see if there is anything you need me to do to help move this
patch forward. If not, I'll snooze this patch and check up on it again
sometime around the end of July.


Thanks!

-Doug

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Doug Anderson 1 year, 6 months ago

Hi,

On Mon, Jun 10, 2024 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, May 17, 2024 at 5:03 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > On systems in the field, we are seeing this sometimes in the kernel logs:
> >   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
> >
> > This means that _something_ decided that it wanted to get a memdump
> > but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
> >
> > The cleanup code in qca_controller_memdump() when we get back an error
> > from hci_devcd_init() undoes most things but forgets to clear
> > QCA_IBS_DISABLED. One side effect of this is that, during the next
> > suspend, qca_suspend() will always get a timeout.
> >
> > Let's fix it so that we clear the bit.
> >
> > Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I'm nowhere near an expert on this code so please give extra eyes on
> > this patch. I also have no idea how to reproduce the problem nor even
> > how to trigger a memdump to test it. I'd love any advice that folks
> > could give. ;-)
> >
> >  drivers/bluetooth/hci_qca.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> Totally fine if you just need more time, but I wanted to follow up and
> check to see if there is anything you need me to do to help move this
> patch forward. If not, I'll snooze this patch and check up on it again
> sometime around the end of July.

It being the end of July, I'm back to check up on this patch. I
checked mainline and bluetooth-next and I don't see any sign of this
patch. Is there anything blocking it? Do you need me to repost it or
take any other actions?

Thanks!

-Doug

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Doug Anderson 1 year, 5 months ago

Hi,

On Wed, Jul 31, 2024 at 1:29 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jun 10, 2024 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, May 17, 2024 at 5:03 PM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > On systems in the field, we are seeing this sometimes in the kernel logs:
> > >   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
> > >
> > > This means that _something_ decided that it wanted to get a memdump
> > > but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
> > >
> > > The cleanup code in qca_controller_memdump() when we get back an error
> > > from hci_devcd_init() undoes most things but forgets to clear
> > > QCA_IBS_DISABLED. One side effect of this is that, during the next
> > > suspend, qca_suspend() will always get a timeout.
> > >
> > > Let's fix it so that we clear the bit.
> > >
> > > Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > I'm nowhere near an expert on this code so please give extra eyes on
> > > this patch. I also have no idea how to reproduce the problem nor even
> > > how to trigger a memdump to test it. I'd love any advice that folks
> > > could give. ;-)
> > >
> > >  drivers/bluetooth/hci_qca.c | 1 +
> > >  1 file changed, 1 insertion(+)
> >
> > Totally fine if you just need more time, but I wanted to follow up and
> > check to see if there is anything you need me to do to help move this
> > patch forward. If not, I'll snooze this patch and check up on it again
> > sometime around the end of July.
>
> It being the end of July, I'm back to check up on this patch. I
> checked mainline and bluetooth-next and I don't see any sign of this
> patch. Is there anything blocking it? Do you need me to repost it or
> take any other actions?

I feel like I'm shouting into the wind. Am I following some incorrect
process for submitting this patch? Can anyone suggest something I
should do differently to get a response here?

Thanks!

-Doug

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Luiz Augusto von Dentz 1 year, 5 months ago

Hi Doug,

On Wed, Aug 21, 2024 at 5:19 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Jul 31, 2024 at 1:29 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Jun 10, 2024 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, May 17, 2024 at 5:03 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > >
> > > > On systems in the field, we are seeing this sometimes in the kernel logs:
> > > >   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
> > > >
> > > > This means that _something_ decided that it wanted to get a memdump
> > > > but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
> > > >
> > > > The cleanup code in qca_controller_memdump() when we get back an error
> > > > from hci_devcd_init() undoes most things but forgets to clear
> > > > QCA_IBS_DISABLED. One side effect of this is that, during the next
> > > > suspend, qca_suspend() will always get a timeout.
> > > >
> > > > Let's fix it so that we clear the bit.
> > > >
> > > > Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > > I'm nowhere near an expert on this code so please give extra eyes on
> > > > this patch. I also have no idea how to reproduce the problem nor even
> > > > how to trigger a memdump to test it. I'd love any advice that folks
> > > > could give. ;-)
> > > >
> > > >  drivers/bluetooth/hci_qca.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > >
> > > Totally fine if you just need more time, but I wanted to follow up and
> > > check to see if there is anything you need me to do to help move this
> > > patch forward. If not, I'll snooze this patch and check up on it again
> > > sometime around the end of July.
> >
> > It being the end of July, I'm back to check up on this patch. I
> > checked mainline and bluetooth-next and I don't see any sign of this
> > patch. Is there anything blocking it? Do you need me to repost it or
> > take any other actions?
>
> I feel like I'm shouting into the wind. Am I following some incorrect
> process for submitting this patch? Can anyone suggest something I
> should do differently to get a response here?

Just resend it since it is no longer listed on patchwork it felt off my radar.

> Thanks!
>
> -Doug



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Guenter Roeck 1 year, 8 months ago

On Fri, May 17, 2024 at 5:03 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> On systems in the field, we are seeing this sometimes in the kernel logs:
>   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
>
> This means that _something_ decided that it wanted to get a memdump
> but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
>
> The cleanup code in qca_controller_memdump() when we get back an error
> from hci_devcd_init() undoes most things but forgets to clear
> QCA_IBS_DISABLED. One side effect of this is that, during the next
> suspend, qca_suspend() will always get a timeout.
>
> Let's fix it so that we clear the bit.
>
> Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> I'm nowhere near an expert on this code so please give extra eyes on
> this patch. I also have no idea how to reproduce the problem nor even
> how to trigger a memdump to test it. I'd love any advice that folks
> could give. ;-)
>
>  drivers/bluetooth/hci_qca.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 0c9c9ee56592..1ef12f5a115d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1090,6 +1090,7 @@ static void qca_controller_memdump(struct work_struct *work)
>                                 qca->memdump_state = QCA_MEMDUMP_COLLECTED;
>                                 cancel_delayed_work(&qca->ctrl_memdump_timeout);
>                                 clear_bit(QCA_MEMDUMP_COLLECTION, &qca->flags);
> +                               clear_bit(QCA_IBS_DISABLED, &qca->flags);
>                                 mutex_unlock(&qca->hci_memdump_lock);
>                                 return;
>                         }
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>

Re: [PATCH] Bluetooth: qca: If memdump doesn't work, re-enable IBS

Posted by Stephen Boyd 1 year, 8 months ago

Quoting Douglas Anderson (2024-05-17 17:02:46)
> On systems in the field, we are seeing this sometimes in the kernel logs:
>   Bluetooth: qca_controller_memdump() hci0: hci_devcd_init Return:-95
>
> This means that _something_ decided that it wanted to get a memdump
> but then hci_devcd_init() returned -EOPNOTSUPP (AKA -95).
>
> The cleanup code in qca_controller_memdump() when we get back an error
> from hci_devcd_init() undoes most things but forgets to clear
> QCA_IBS_DISABLED. One side effect of this is that, during the next
> suspend, qca_suspend() will always get a timeout.
>
> Let's fix it so that we clear the bit.
>
> Fixes: 06d3fdfcdf5c ("Bluetooth: hci_qca: Add qcom devcoredump support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>