Re: [PATCH 2/7] Bluetooth: add a quirk for controllers that hang on Write Current IAC LAP

[Yang Li <yang.li@amlogic.com>]

Hi,

>
> [ EXTERNAL EMAIL ]
>
> From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
>
> Some controllers list Write Current IAC LAP among their supported
> commands and answer Read Current IAC LAP, but stop responding to HCI
> altogether as soon as the write is issued, even when the value written
> is the general inquiry access code the controller already reports.
> Nothing short of a power cycle recovers the part.
>
> The write is only sent on the way to becoming discoverable, so such a
> controller dies exactly when something asks it to become visible, and
> the failure looks like a dead radio rather than a refused command.
>
> Add HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP and skip the write when it
> is set. This costs limited discoverable mode, which needs the LIAC
> programmed. General discoverable mode is unaffected, as the controller
> already holds the GIAC.
>
> Seen on the Amlogic W1 (W155S2); set the quirk in hci_aml.


This appears to be an anomaly with the firmware binary. Could you 
provide the FW version in use? It is logged during Bluetooth driver 
initialization.


> Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
> ---
>   drivers/bluetooth/hci_aml.c |  6 ++++++
>   include/net/bluetooth/hci.h | 18 ++++++++++++++++++
>   net/bluetooth/hci_sync.c    |  8 ++++++++
>   3 files changed, 32 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
> index 990e701da4be..5979b114f2e4 100644
> --- a/drivers/bluetooth/hci_aml.c
> +++ b/drivers/bluetooth/hci_aml.c
> @@ -603,6 +603,12 @@ static int aml_setup(struct hci_uart *hu)
>           */
>          hci_set_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT);
>
> +       /* Write Current IAC LAP is advertised, but the controller stops
> +        * answering HCI as soon as it is sent, until the combo chip is power
> +        * cycled. It is sent when the adapter is made discoverable.
> +        */
> +       hci_set_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP);
> +
>          err = aml_power_on(amldev);
>          if (err)
>                  return err;
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index bcbe76c84d1b..a59f48f387ac 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -389,6 +389,24 @@ enum {
>           */
>          HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT,
>
> +       /* When this quirk is set, HCI_OP_WRITE_CURRENT_IAC_LAP is never sent.
> +        *
> +        * Some controllers list the command among the ones they support, and
> +        * answer Read Current IAC LAP quite happily, but wedge and stop
> +        * responding to anything at all the moment the write is issued - even
> +        * when the value being written is the general inquiry access code the
> +        * controller already reports. Going discoverable is what triggers the
> +        * write, so on such a controller the adapter dies precisely when it is
> +        * asked to become visible.
> +        *
> +        * Skipping the write costs limited discoverable mode, which needs the
> +        * LIAC programmed. General discoverable mode is unaffected: the GIAC
> +        * is what these controllers already have.
> +        *
> +        * This quirk must be set before hci_register_dev is called.
> +        */
> +       HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP,
> +
>          __HCI_NUM_QUIRKS,
>   };
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index f1dfef81ca96..92c442910d6a 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5258,6 +5258,8 @@ static const struct {
>                           "and will not be used."),
>          HCI_QUIRK_BROKEN(WRITE_SC_SUPPORT,
>                           "HCI BR/EDR Secure Connections is advertised, but cannot be enabled."),
> +       HCI_QUIRK_BROKEN(WRITE_CURRENT_IAC_LAP,
> +                        "HCI Write Current IAC LAP is advertised, but hangs the controller."),
>          HCI_QUIRK_BROKEN(LE_CODED,
>                           "HCI LE Coded PHY feature bit is set, "
>                           "but its usage is not supported.")
> @@ -6127,6 +6129,12 @@ static int hci_write_iac_sync(struct hci_dev *hdev)
>          if (!hci_dev_test_flag(hdev, HCI_DISCOVERABLE))
>                  return 0;
>
> +       /* Controllers that wedge on the write are left alone. They already
> +        * hold the GIAC, so general discoverable mode works without it.
> +        */
> +       if (hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP))
> +               return 0;
> +
>          memset(&cp, 0, sizeof(cp));
>
>          if (hci_dev_test_flag(hdev, HCI_LIMITED_DISCOVERABLE)) {
> --
> 2.56.0
>