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?
>
> Yes, if hci_dev struct is valid, then we can use the flag.
> The problem is btintel.c doesn't know whcih SKU it has with until it reads the
> HCI_Intel_Read_Version command, but this command fails due to SW RFKILL. The
> only way to tell what SKU has without reading the version command is blacklist
> table.
>
> One other option is remove the LED quirk for StP/SdP. This is a cosmatic issue
> and I believe it wouldn't affect any BT functionality.
but we succeed with HCI_Intel_Read_Version on a cold boot. So that means it is just that we have to make this one flag persistent. So it is valid even when no HCI_Intel_Read_Version is read. Or just make it a bool variable in the btintel internal struct.
I am failing to see why this wouldn’t work. And I fully realize that I am pedantic here, but I really want to confine these things to internal handling and not have them bleed into the driver.
Regards
Marcel
