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.
>>
>> this is the part that I don’t grok. So how do we reset the USB power while
>> still keeping it. Does this mean we see a USB Disconnect and USB Reconnect
>> happening, but the second time around we enter btusb_probe() we come from a
>> total different state?
>>
>> And how does it make sense that calling hdev->shutdown() ends up in
>> btusb_remove() + btusb_probe(). I am confused.
>
> I think I didn't explan the test case enough. There is no issue if the HCI is up
> before rebooting the system. The issue is reproducible only when the HCI
> interface is down and reboot.
>
> For example, the steps are:
> 1. Bluetooth daemon is not running (actually it doesn't matter)
> 2. Put HCI Down and it causes hdev->shutdown()->btintel_shutdown_combined()
> 3. Now StP is in SW RFKILL state
> 4. Reboot
> 5. btintel_setup_combined() is called
> 6. HCI_Intel_Read_Version command failed.
>
> So, the flag value set before the reboot is no longer available/valid after
> reboot. Also, while rebooting, I don't see USB disconnect and the device state
> is same as before the reboot.
ok, but this sounds like something we could fix internally in our btintel.c code. If hci_dev struct is still present we should be able to persist flags across ->shutdown and ->setup. If we clear them, then it is our issue. No need to get btusb.c blacklist table involved. What am I missing?
Regards
Marcel
