Hi Marcel,
On Wed, 2021-12-22 at 09:06 +0100, Marcel Holtmann wrote:
> Hi Tedd,
>
> > This patch fixes the broken LED quirk for Intel legacy ROM devices.
> > To fix the LED issue that doesn't turn off immediately, the host sends
> > the SW RFKILL command while shutting down the interface and it puts the
> > devices in an asserted state.
> >
> > Once the device is in SW RFKILL state, it can only accept HCI_Reset to
> > exit from the SW RFKILL state. This patch checks the quirk and sends the
> > HCI_Reset before sending the HCI_Intel_Read_Version command.
> >
> > The affected legacy ROM devices are
> > - 8087:0a2a
> > - 8087:0aa7
> >
> > fixes: ffcba827c0a1d ("Bluetooth: btintel: Fix the LED is not turning off
> > immediately")
> >
> > Signed-off-by: Tedd Ho-Jeong An <tedd.an@xxxxxxxxx>
> > ---
> > drivers/bluetooth/btintel.c | 13 ++++++-------
> > drivers/bluetooth/btusb.c | 10 ++++++++--
> > 2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index e1f96df847b8..75f8d7aceb35 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2355,8 +2355,13 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> > * As a workaround, send HCI Reset command first which will reset the
> > * number of completed commands and allow normal command processing
> > * from now on.
> > + *
> > + * For INTEL_BROKEN_LED, these devices have an issue with LED which
> > + * doesn't go off immediately during shutdown. Set the flag here to
> > send
> > + * the LED OFF command during shutdown.
> > */
> > - if (btintel_test_flag(hdev, INTEL_BROKEN_INITIAL_NCMD)) {
> > + if (btintel_test_flag(hdev, INTEL_BROKEN_INITIAL_NCMD) ||
> > + btintel_test_flag(hdev, INTEL_BROKEN_LED)) {
> > skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL,
> > HCI_INIT_TIMEOUT);
> > if (IS_ERR(skb)) {
> > @@ -2428,12 +2433,6 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> > set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
> > &hdev->quirks);
> >
> > - /* These devices have an issue with LED which doesn't
> > - * go off immediately during shutdown. Set the flag
> > - * here to send the LED OFF command during shutdown.
> > - */
> > - btintel_set_flag(hdev, INTEL_BROKEN_LED);
> > -
> > err = btintel_legacy_rom_setup(hdev, &ver);
> > break;
> > case 0x0b: /* SfP */
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d1bd9ee0a6ab..c6a070d5284f 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -60,6 +60,7 @@ static struct usb_driver btusb_driver;
> > #define BTUSB_WIDEBAND_SPEECH 0x400000
> > #define BTUSB_VALID_LE_STATES 0x800000
> > #define BTUSB_QCA_WCN6855 0x1000000
> > +#define BTUSB_INTEL_BROKEN_LED 0x2000000
> > #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
> >
> > static const struct usb_device_id btusb_table[] = {
> > @@ -382,9 +383,11 @@ static const struct usb_device_id blacklist_table[] = {
> > { USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },
> > { USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED |
> >
> > BTUSB_INTEL_BROKEN_INITIAL_NCMD },
> > - { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED },
> > + { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED |
> > + BTUSB_INTEL_BROKEN_LED },
> > { USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_COMBINED },
> > - { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED },
> > + { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED |
> > + BTUSB_INTEL_BROKEN_LED },
> > { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_COMBINED },
>
> this is the part that I tried to avoid.
I remembered it but I couldn't find any other way.
I already tried the method below but it didn't work especially for the reboot
(warm boot) case becase the platform keeps the USB power while rebooting the
system and BT device is still in the SW RFKILL state.
The flag sets in the btintel_shutdown_combined() doesn't stay because the HDEV
and the driver data are freed and allocated again while rebooting. So the
intel_flag_test_and_clear(INTEL_SHUTDOWN_EXECUTED) is never TRUE.
>
> >
> > /* Other Intel Bluetooth devices */
> > @@ -3724,6 +3727,9 @@ static int btusb_probe(struct usb_interface *intf,
> >
> > if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD)
> > btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);
> > +
> > + if (id->driver_info & BTUSB_INTEL_BROKEN_LED)
> > + btintel_set_flag(hdev, INTEL_BROKEN_LED);
> > }
> >
> > if (id->driver_info & BTUSB_MARVELL)
>
> If we assume that all bootloader (except WP2) operate on power up properly, then
> this should be all internal.
>
> In btintel_setup_combined() leave the setting of the INTEL_BROKEN_LED flag as it
> is. However introduce another flag internal to btintel.c that indicates shutdown
> has been run. For example INTEL_SHUTDOWN_EXECUTED. You set that in shutdown()
> and clear it in setup(). And in case it is set in setup, then you execute the
> HCI_Reset.
>
> I am thinking like this:
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index e1f96df847b8..65bb0ae05bf4 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -2368,6 +2368,10 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> kfree_skb(skb);
> }
>
> + if (btintel_test_and_clear_flag(hdev, INTEL_SHUTDOWN_EXECUTED)) {
> + /* send HCI_Reset */
> + }
> +
> /* Starting from TyP device, the command parameter and response are
> * changed even though the OCF for HCI_Intel_Read_Version command
> * remains same. The legacy devices can handle even if the
> @@ -2596,6 +2600,7 @@ static int btintel_shutdown_combined(struct hci_dev *hdev)
> return ret;
> }
> kfree_skb(skb);
> + btintel_set_flag(hdev, INTEL_SHUTDOWN_EXECUTED);
> }
>
> return 0;
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e500c0d7a729..ff2e7838c6d1 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -152,6 +152,7 @@ enum {
> INTEL_BROKEN_INITIAL_NCMD,
> INTEL_BROKEN_LED,
> INTEL_ROM_LEGACY,
> + INTEL_SHUTDOWN_EXECUTED,
>
> __INTEL_NUM_FLAGS,
> };
>
> Obviously we need to put comments around why we set these flags etc., but I
> think you get the idea.
>
> Regards
>
> Marcel
>
