Hi Pedro, On Wed, Feb 26, 2025 at 2:18 PM Pedro Nishiyama <nishiyama.pedro@xxxxxxxxx> wrote: > > Hello, > > On Tue, Feb 25, 2025 at 12:55 PM Luiz Augusto von Dentz > <luiz.dentz@xxxxxxxxx> wrote: > > > > Hi Pedro, > > > > On Mon, Feb 24, 2025 at 7:27 PM Pedro Nishiyama > > <nishiyama.pedro@xxxxxxxxx> wrote: > > > > > > Hi Luiz, > > > > > > On Mon, Feb 24, 2025 at 6:36 PM Luiz Augusto von Dentz > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > Hi Pedro, > > > > > > > > On Mon, Feb 24, 2025 at 3:54 PM Pedro Nishiyama > > > > <nishiyama.pedro@xxxxxxxxx> wrote: > > > > > > > > > > This adds quirks for broken READ_VOICE_SETTING and READ_PAGE_SCAN_TYPE. > > > > > > > > > > Signed-off-by: Pedro Nishiyama <nishiyama.pedro@xxxxxxxxx> > > > > > --- > > > > > include/net/bluetooth/hci.h | 16 ++++++++++++++++ > > > > > net/bluetooth/hci_sync.c | 6 ++++++ > > > > > 2 files changed, 22 insertions(+) > > > > > > > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > > > > index 0d51970d809f..b99818df8ee7 100644 > > > > > --- a/include/net/bluetooth/hci.h > > > > > +++ b/include/net/bluetooth/hci.h > > > > > @@ -354,6 +354,22 @@ enum { > > > > > * during the hdev->setup vendor callback. > > > > > */ > > > > > HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_PHY, > > > > > + > > > > > + /* When this quirk is set, the HCI_OP_READ_VOICE_SETTING command is > > > > > + * skipped. This is required for a subset of the CSR controller clones > > > > > + * which erroneously claim to support it. > > > > > + * > > > > > + * This quirk must be set before hci_register_dev is called. > > > > > + */ > > > > > + HCI_QUIRK_BROKEN_READ_VOICE_SETTING, > > > > > > > > Lets split this in 2 parts, one for voice setting and another for page > > > > scan type. > > > > > > > > > + /* When this quirk is set, the HCI_OP_READ_PAGE_SCAN_TYPE command is > > > > > + * skipped. This is required for a subset of the CSR controller clones > > > > > + * which erroneously claim to support it. > > > > > + * > > > > > + * This quirk must be set before hci_register_dev is called. > > > > > + */ > > > > > + HCI_QUIRK_BROKEN_READ_PAGE_SCAN_TYPE, > > > > > }; > > > > > > > > > > /* HCI device flags */ > > > > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > > > > index dd770ef5ec36..e76012956020 100644 > > > > > --- a/net/bluetooth/hci_sync.c > > > > > +++ b/net/bluetooth/hci_sync.c > > > > > @@ -3696,6 +3696,9 @@ static int hci_read_local_name_sync(struct hci_dev *hdev) > > > > > /* Read Voice Setting */ > > > > > static int hci_read_voice_setting_sync(struct hci_dev *hdev) > > > > > { > > > > > + if (test_bit(HCI_QUIRK_BROKEN_READ_VOICE_SETTING, &hdev->quirks)) > > > > > + return 0; > > > > > > > > While at it I'd add the checking for the bit as well, that said > > > > perhaps we need to disable SCO link if voice settings cannot be read, > > > > is the controller able to create SCO connections? > > > > > > I did some tests on the version before the regression and with the patches. > > > It is possible to create an SCO connection, but the connection is not usable. > > > Before the regression there is no sound, and with the patches, dmesg > > > is spammed with "Bluetooth: hci0: corrupted SCO packet". > > > > Then perhaps we should disable SCO support if it doesn't support read > > voice settings. > > I'm having some trouble disabling SCO. We can't add BTUSB_BROKEN_ISOC > to the drivers quirks because not all 0a12:0001 are clones with broken > READ_VOICE_SETTING. > The fake identification is done after the btusb_probe, and we set up > the isoc during the btusb_probe. > I'm not sure if removing the isoc device file and releasing its > interface in the hdev->setup is actually safe. Well I was not thinking of disabling the ISOC endpoint if that is what you meant by adding BTUSB_BROKEN_ISOC, but disabling/fail the creation of SCO sockets in the above layers if we detect the read voice setting is not working. > > > > > > > > > > > return __hci_cmd_sync_status(hdev, HCI_OP_READ_VOICE_SETTING, > > > > > 0, NULL, HCI_CMD_TIMEOUT); > > > > > } > > > > > @@ -4132,6 +4135,9 @@ static int hci_read_page_scan_type_sync(struct hci_dev *hdev) > > > > > if (!(hdev->commands[13] & 0x01)) > > > > > return 0; > > > > > > > > > > + if (test_bit(HCI_QUIRK_BROKEN_READ_PAGE_SCAN_TYPE, &hdev->quirks)) > > > > > + return 0; > > > > > + > > > > > return __hci_cmd_sync_status(hdev, HCI_OP_READ_PAGE_SCAN_TYPE, > > > > > 0, NULL, HCI_CMD_TIMEOUT); > > > > > } > > > > > -- > > > > > 2.48.1 > > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > > -- > > > Pedro Nishiyama > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz