2019-01-28 08:54:51

by Hegde, Raghuram

[permalink] [raw]
Subject: [PATCH] btusb: Add shutdown routine for BTUSB_INTEL_NEW devices

From: Raghuram Hegde <[email protected]>

btusb_shutdown_intel routine shall reset the controller
and stop all BT operation.
Advantages:
1. Power save on the platform
2. Host and controller will be in Sync.

Signed-off-by: Raghuram Hegde <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
---
drivers/bluetooth/btusb.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 5de0c2e59b97..66483ca3d870 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3139,6 +3139,7 @@ static int btusb_probe(struct usb_interface *intf,
hdev->manufacturer = 2;
hdev->send = btusb_send_frame_intel;
hdev->setup = btusb_setup_intel_new;
+ hdev->shutdown = btusb_shutdown_intel;
hdev->hw_error = btintel_hw_error;
hdev->set_diag = btintel_set_diag;
hdev->set_bdaddr = btintel_set_bdaddr;
--
2.7.4


2019-01-28 12:05:04

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] btusb: Add shutdown routine for BTUSB_INTEL_NEW devices

Hi Raghuram,

> btusb_shutdown_intel routine shall reset the controller
> and stop all BT operation.
> Advantages:
> 1. Power save on the platform
> 2. Host and controller will be in Sync.
>
> Signed-off-by: Raghuram Hegde <[email protected]>
> Signed-off-by: Chethan T N <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 5de0c2e59b97..66483ca3d870 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3139,6 +3139,7 @@ static int btusb_probe(struct usb_interface *intf,
> hdev->manufacturer = 2;
> hdev->send = btusb_send_frame_intel;
> hdev->setup = btusb_setup_intel_new;
> + hdev->shutdown = btusb_shutdown_intel;
> hdev->hw_error = btintel_hw_error;
> hdev->set_diag = btintel_set_diag;
> hdev->set_bdaddr = btintel_set_bdaddr;

I assumed that this was only needed for the older ROM versions of the Intel controllers and not the newer RAM versions. I have been told they don’t inherit the LED issue that we tried to fix with this. Please read the comments in btusb_shutdown_intel and amend comments if needed and provide a detailed commit message.

Regards

Marcel


2019-01-28 15:37:02

by chethan tn

[permalink] [raw]
Subject: Re: [PATCH] btusb: Add shutdown routine for BTUSB_INTEL_NEW devices

Hi Marcel,

On Mon, Jan 28, 2019 at 5:37 PM Marcel Holtmann <[email protected]> wrote:
>
> Hi Raghuram,
>
> > btusb_shutdown_intel routine shall reset the controller
> > and stop all BT operation.
> > Advantages:
> > 1. Power save on the platform
> > 2. Host and controller will be in Sync.
> >
> > Signed-off-by: Raghuram Hegde <[email protected]>
> > Signed-off-by: Chethan T N <[email protected]>
> > ---
> > drivers/bluetooth/btusb.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 5de0c2e59b97..66483ca3d870 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3139,6 +3139,7 @@ static int btusb_probe(struct usb_interface *intf,
> > hdev->manufacturer = 2;
> > hdev->send = btusb_send_frame_intel;
> > hdev->setup = btusb_setup_intel_new;
> > + hdev->shutdown = btusb_shutdown_intel;
> > hdev->hw_error = btintel_hw_error;
> > hdev->set_diag = btintel_set_diag;
> > hdev->set_bdaddr = btintel_set_bdaddr;
>
> I assumed that this was only needed for the older ROM versions of the Intel controllers and not the newer RAM versions. I have been told they don’t inherit the LED issue that we tried to fix with this. Please read the comments in btusb_shutdown_intel and amend comments if needed and provide a detailed commit message.
Yes you're absolutely right about the LED issue on the older ROM products.
But in the recent day we have observed that in case BT
operation(Inquiry/LE Scan) were triggered through the stack and
further BT was turned off through "hciconfig hci0 down". In this case
controller would active doing BT operation and consume power and also
might cause race condition on the next BT on as the controller might
try to push the events that were queued up before processing the reset
command. So to make sure when BT is turned off either through stack or
through command line thought it would be better approach to reset the
controller(This is applicable for ROM or RAM products).
>
> Regards
>
> Marcel
>

2019-01-28 17:22:26

by chethan tn

[permalink] [raw]
Subject: Re: [PATCH] btusb: Add shutdown routine for BTUSB_INTEL_NEW devices

Hi Marcel

On Mon, Jan 28, 2019 at 9:30 PM Marcel Holtmann <[email protected]> wrote:
>
> Hi Chethan,
>
> >>> btusb_shutdown_intel routine shall reset the controller
> >>> and stop all BT operation.
> >>> Advantages:
> >>> 1. Power save on the platform
> >>> 2. Host and controller will be in Sync.
> >>>
> >>> Signed-off-by: Raghuram Hegde <[email protected]>
> >>> Signed-off-by: Chethan T N <[email protected]>
> >>> ---
> >>> drivers/bluetooth/btusb.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>> index 5de0c2e59b97..66483ca3d870 100644
> >>> --- a/drivers/bluetooth/btusb.c
> >>> +++ b/drivers/bluetooth/btusb.c
> >>> @@ -3139,6 +3139,7 @@ static int btusb_probe(struct usb_interface *intf,
> >>> hdev->manufacturer = 2;
> >>> hdev->send = btusb_send_frame_intel;
> >>> hdev->setup = btusb_setup_intel_new;
> >>> + hdev->shutdown = btusb_shutdown_intel;
> >>> hdev->hw_error = btintel_hw_error;
> >>> hdev->set_diag = btintel_set_diag;
> >>> hdev->set_bdaddr = btintel_set_bdaddr;
> >>
> >> I assumed that this was only needed for the older ROM versions of the Intel controllers and not the newer RAM versions. I have been told they don’t inherit the LED issue that we tried to fix with this. Please read the comments in btusb_shutdown_intel and amend comments if needed and provide a detailed commit message.
> > Yes you're absolutely right about the LED issue on the older ROM products.
> > But in the recent day we have observed that in case BT
> > operation(Inquiry/LE Scan) were triggered through the stack and
> > further BT was turned off through "hciconfig hci0 down". In this case
> > controller would active doing BT operation and consume power and also
> > might cause race condition on the next BT on as the controller might
> > try to push the events that were queued up before processing the reset
> > command. So to make sure when BT is turned off either through stack or
> > through command line thought it would be better approach to reset the
> > controller(This is applicable for ROM or RAM products).
>
> what about creating a btusb_shutdown_intel_new() that just sends the HCI Reset command. That should be enough to ensure that anything in the radio scheduler gets cancelled and cleaned up.
Sure, will make the changes accordingly and send the new patch.
>
> Regards
>
> Marcel
>
Thanks and Regards
Chethan

2019-01-28 17:27:22

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] btusb: Add shutdown routine for BTUSB_INTEL_NEW devices

Hi Chethan,

>>> btusb_shutdown_intel routine shall reset the controller
>>> and stop all BT operation.
>>> Advantages:
>>> 1. Power save on the platform
>>> 2. Host and controller will be in Sync.
>>>
>>> Signed-off-by: Raghuram Hegde <[email protected]>
>>> Signed-off-by: Chethan T N <[email protected]>
>>> ---
>>> drivers/bluetooth/btusb.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 5de0c2e59b97..66483ca3d870 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -3139,6 +3139,7 @@ static int btusb_probe(struct usb_interface *intf,
>>> hdev->manufacturer = 2;
>>> hdev->send = btusb_send_frame_intel;
>>> hdev->setup = btusb_setup_intel_new;
>>> + hdev->shutdown = btusb_shutdown_intel;
>>> hdev->hw_error = btintel_hw_error;
>>> hdev->set_diag = btintel_set_diag;
>>> hdev->set_bdaddr = btintel_set_bdaddr;
>>
>> I assumed that this was only needed for the older ROM versions of the Intel controllers and not the newer RAM versions. I have been told they don’t inherit the LED issue that we tried to fix with this. Please read the comments in btusb_shutdown_intel and amend comments if needed and provide a detailed commit message.
> Yes you're absolutely right about the LED issue on the older ROM products.
> But in the recent day we have observed that in case BT
> operation(Inquiry/LE Scan) were triggered through the stack and
> further BT was turned off through "hciconfig hci0 down". In this case
> controller would active doing BT operation and consume power and also
> might cause race condition on the next BT on as the controller might
> try to push the events that were queued up before processing the reset
> command. So to make sure when BT is turned off either through stack or
> through command line thought it would be better approach to reset the
> controller(This is applicable for ROM or RAM products).

what about creating a btusb_shutdown_intel_new() that just sends the HCI Reset command. That should be enough to ensure that anything in the radio scheduler gets cancelled and cleaned up.

Regards

Marcel