| Message ID | 20211216210958.62129-1-hj.tedd.an@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2] Bluetooth: btintel: Fix broken LED quirk for legacy ROM devices | expand |
Hi Tedd, On Thu, Dec 16, 2021 at 4:04 PM Tedd Ho-Jeong An <hj.tedd.an@gmail.com> wrote: > > From: Tedd Ho-Jeong An <tedd.an@intel.com> > > 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@intel.com> > --- > 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 I wonder why we haven't been using BIT macro here and did we make a mistake and leave one bit behind? Or something else was at this bit position? > #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 }, > > /* 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); I wonder if wouldn't be simples to have a flag e.g. INTEL_NEED_RESET instead of replicating the quirks as flags since in either case the actual outcome is to send a reset. > } > > if (id->driver_info & BTUSB_MARVELL) > -- > 2.25.1 >
Hi Luiz, On Thu, 2021-12-16 at 16:47 -0800, Luiz Augusto von Dentz wrote: > Hi Tedd, > > On Thu, Dec 16, 2021 at 4:04 PM Tedd Ho-Jeong An <hj.tedd.an@gmail.com> wrote: > > > > From: Tedd Ho-Jeong An <tedd.an@intel.com> > > > > 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@intel.com> > > --- > > 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 > > I wonder why we haven't been using BIT macro here and did we make a > mistake and leave one bit behind? Or something else was at this bit > position? There used to be a flag BTUSB_INTEL_NEWGEN, and it was removed while refactoring the btintel, and the NCMD was added before removing the NEWGEN flag. > > > #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 }, > > > > /* 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); > > I wonder if wouldn't be simples to have a flag e.g. INTEL_NEED_RESET > instead of replicating the quirks as flags since in either case the > actual outcome is to send a reset. The result is same though, I think it is better to be more specific. I am fine with it also. > > > } > > > > if (id->driver_info & BTUSB_MARVELL) > > -- > > 2.25.1 > > > >
Hi Tedd, On Thu, Dec 16, 2021 at 6:47 PM An, Tedd <tedd.an@intel.com> wrote: > > Hi Luiz, > > On Thu, 2021-12-16 at 16:47 -0800, Luiz Augusto von Dentz wrote: > > Hi Tedd, > > > > On Thu, Dec 16, 2021 at 4:04 PM Tedd Ho-Jeong An <hj.tedd.an@gmail.com> wrote: > > > > > > From: Tedd Ho-Jeong An <tedd.an@intel.com> > > > > > > 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@intel.com> > > > --- > > > 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 > > > > I wonder why we haven't been using BIT macro here and did we make a > > mistake and leave one bit behind? Or something else was at this bit > > position? > > There used to be a flag BTUSB_INTEL_NEWGEN, and it was removed while > refactoring the btintel, and the NCMD was added before removing the > NEWGEN flag. Ok that explains it then, luckily this is just used internally by the driver so it shouldn't break anything if we reuse, but I'd still think using BIT macro is clear here. > > > > > #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 }, > > > > > > /* 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); > > > > I wonder if wouldn't be simples to have a flag e.g. INTEL_NEED_RESET > > instead of replicating the quirks as flags since in either case the > > actual outcome is to send a reset. > > The result is same though, I think it is better to be more specific. > I am fine with it also. Fair enough. > > > > > } > > > > > > if (id->driver_info & BTUSB_MARVELL) > > > -- > > > 2.25.1 > > > > > > > >
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@intel.com> > --- > 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. > > /* 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
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@intel.com> > > --- > > 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 >
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@intel.com> >>> --- >>> 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. Regards Marcel
Hi Marcel, On Thu, 2021-12-23 at 00:08 +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@intel.com> > > > > --- > > > > 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. > > Regards > > Marcel >
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@intel.com> >>>>> --- >>>>> 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
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@intel.com> > > > > > > --- > > > > > > 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 >
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@intel.com> >>>>>>> --- >>>>>>> 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
Hi Marcel, On Thu, 2021-12-23 at 22:24 +0100, Marcel Holtmann wrote: > > 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. > Yes, there is no problem with cold boot. The problem is warm boot. Here are the log after making the changes with some debug messages. [COLD BOOT] Dec 24 16:16:27 han1-XPS-13-9350 kernel: microcode: microcode updated early to revision 0xea, date = 2021-01-25 Dec 24 16:16:27 han1-XPS-13-9350 kernel: Linux version 5.16.0-rc1+ (han1@han1-XPS-13-9350) (gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.37) #1 SMP PREEMPT Thu Dec 23 20:52:12 PST 2021 Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: new full-speed USB device number 2 using xhci_hcd Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: New USB device found, idVendor=8087, idProduct=0a2a, bcdDevice= 0.01 Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: New USB device strings: Mfr=0, Product=0, SerialNumber=0 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: Core ver 2.22 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btusb_probe: intf 00000000074889f5 id 00000000c09ad578 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Allocate hci_dev=00000000a477ec45 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set hci_set_drvdata(hdev=00000000a477ec45, data=0000000077ec88bc) Dec 24 16:16:28 han1-XPS-13-9350 kernel: usbcore: registered new interface driver btusb Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_setup_combined(hdev=00000000a477ec45) Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Test flag(hdev=00000000a477ec45, INTEL_SHUTDOWN_EXECUTED) Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> INTEL_SHUTDOWN_EXECUTED is NOT SET Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Legacy ROM 2.5 revision 1.0 build 3 week 17 2014 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Intel Bluetooth firmware file: intel/ibt-hw-37.8.10-fw-1.10.3.11.e.bseq Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: unexpected event 0xff length: 2 > 0 Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Intel BT fw patch 0x32 completed & activated [HCI DOWN] Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_shutdown_combined(hdev=00000000a477ec45) Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Send HCI_Intel_SWRFKILL Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set INTEL_SHUTDOWN_EXECUTED Dec 24 16:17:35 han1-XPS-13-9350 systemd-shutdown[1]: Sending SIGTERM to remaining processes... [REBOOT...] Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: new full-speed USB device number 2 using xhci_hcd Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: New USB device found, idVendor=8087, idProduct=0a2a, bcdDevice= 0.01 Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: New USB device strings: Mfr=0, Product=0, SerialNumber=0 Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: Core ver 2.22 Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btusb_probe: intf 0000000016d7e789 id 0000000058418b5d Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Allocate hci_dev=0000000052758830 Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set hci_set_drvdata(hdev=0000000052758830, data=0000000074e43445) Dec 24 16:18:41 han1-XPS-13-9350 kernel: usbcore: registered new interface driver btusb Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_setup_combined(hdev=0000000052758830) Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Test flag(hdev=0000000052758830, INTEL_SHUTDOWN_EXECUTED) Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> INTEL_SHUTDOWN_EXECUTED is NOT SET Dec 24 16:18:43 han1-XPS-13-9350 kernel: Bluetooth: hci0: Reading Intel version command failed (-110) Dec 24 16:18:43 han1-XPS-13-9350 kernel: Bluetooth: hci0: command tx timeout After [REBOOT], the INTEL_SHUTDOWN_EXECUTED flag that was set before the reboot is gone. So, how can I make the flag persisten between rebooting the system? > 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 >
Hi Tedd, >> 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. >> > > Yes, there is no problem with cold boot. The problem is warm boot. Here are the log after making the changes with some debug messages. > > [COLD BOOT] > > Dec 24 16:16:27 han1-XPS-13-9350 kernel: microcode: microcode updated early to revision 0xea, date = 2021-01-25 > Dec 24 16:16:27 han1-XPS-13-9350 kernel: Linux version 5.16.0-rc1+ (han1@han1-XPS-13-9350) (gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.37) #1 SMP PREEMPT Thu Dec 23 > 20:52:12 PST 2021 > > Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: new full-speed USB device number 2 using xhci_hcd > Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: New USB device found, idVendor=8087, idProduct=0a2a, bcdDevice= 0.01 > Dec 24 16:16:27 han1-XPS-13-9350 kernel: usb 1-3: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: Core ver 2.22 > > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btusb_probe: intf 00000000074889f5 id 00000000c09ad578 > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Allocate hci_dev=00000000a477ec45 > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set hci_set_drvdata(hdev=00000000a477ec45, data=0000000077ec88bc) > Dec 24 16:16:28 han1-XPS-13-9350 kernel: usbcore: registered new interface driver btusb > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_setup_combined(hdev=00000000a477ec45) > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Test flag(hdev=00000000a477ec45, INTEL_SHUTDOWN_EXECUTED) > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: >>>>> INTEL_SHUTDOWN_EXECUTED is NOT SET > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Legacy ROM 2.5 revision 1.0 build 3 week 17 2014 > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Intel Bluetooth firmware file: intel/ibt-hw-37.8.10-fw-1.10.3.11.e.bseq > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: unexpected event 0xff length: 2 > 0 > Dec 24 16:16:28 han1-XPS-13-9350 kernel: Bluetooth: hci0: Intel BT fw patch 0x32 completed & activated > > [HCI DOWN] > > Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_shutdown_combined(hdev=00000000a477ec45) > Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Send HCI_Intel_SWRFKILL > Dec 24 16:17:19 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set INTEL_SHUTDOWN_EXECUTED > > Dec 24 16:17:35 han1-XPS-13-9350 systemd-shutdown[1]: Sending SIGTERM to remaining processes... > > [REBOOT...] > > Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: new full-speed USB device number 2 using xhci_hcd > Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: New USB device found, idVendor=8087, idProduct=0a2a, bcdDevice= 0.01 > Dec 24 16:18:40 han1-XPS-13-9350 kernel: usb 1-3: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: Core ver 2.22 > > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btusb_probe: intf 0000000016d7e789 id 0000000058418b5d > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Allocate hci_dev=0000000052758830 > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Set hci_set_drvdata(hdev=0000000052758830, data=0000000074e43445) > Dec 24 16:18:41 han1-XPS-13-9350 kernel: usbcore: registered new interface driver btusb > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> btintel_setup_combined(hdev=0000000052758830) > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> Test flag(hdev=0000000052758830, INTEL_SHUTDOWN_EXECUTED) > Dec 24 16:18:41 han1-XPS-13-9350 kernel: Bluetooth: >>>>> INTEL_SHUTDOWN_EXECUTED is NOT SET > > Dec 24 16:18:43 han1-XPS-13-9350 kernel: Bluetooth: hci0: Reading Intel version command failed (-110) > Dec 24 16:18:43 han1-XPS-13-9350 kernel: Bluetooth: hci0: command tx timeout > > > After [REBOOT], the INTEL_SHUTDOWN_EXECUTED flag that was set before the reboot is gone. > So, how can I make the flag persisten between rebooting the system? thanks for being patient with me. I finally understand the issue here. The system reboots and thus we get a new hci_dev, but the controller sticks in its runtime suspend operational mode. So yes, the only way to make this right is to list the quirk in btusb.c blacklist table. I review the original patch once again after fully groking the issue. Regards Marcel
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@intel.com> > --- > 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)) { make sure to indent this as this: if (btintel_test..(..) || btintel_test..(..)) { And rename this into INTEL_BROKEN_SHUTDOWN_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 Use BTUSB_INTEL_BROKEN_SHUTDOWN_LED here. > #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 }, This one needs both. I know it is just going to work, but for simplicity set it both so that WP2 uses this quirk in shutdown as well. So when you remove the set_flag from setup, you fail to run ->shutdown on WP2. I know the BROKEN_INITIAL_NCMD is now pointless, but lets keep it for documentation sake. > - { 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 }, > > /* 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); > } Regards Marcel
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 }, /* 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)