HI Marcel,
On Thu, 2021-12-23 at 09:36 +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.
> > >
> > > 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.
>
> Regards
>
> Marcel
>
