linux-kernel - Re: [PATCH v4 3/4] Bluetooth: hci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Nov 2019 06:29:50 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Cc:     Johan Hedberg <johan.hedberg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-bluetooth@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] Bluetooth: hci_bcm: Support pcm params in dts

Hi Abhishek,

>>> BCM chips may require configuration of PCM to operate correctly and
>>> there is a vendor specific HCI command to do this. Add support in the
>>> hci_bcm driver to parse this from devicetree and configure the chip.
>>> 
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
>>> ---
>>> 
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>> 
>>> drivers/bluetooth/hci_bcm.c | 32 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>> index 6134bff58748..4ee0b45be7e2 100644
>>> --- a/drivers/bluetooth/hci_bcm.c
>>> +++ b/drivers/bluetooth/hci_bcm.c
>>> @@ -88,6 +88,8 @@ struct bcm_device_data {
>>> *    used to disable flow control during runtime suspend and system sleep
>>> * @is_suspended: whether flow control is currently disabled
>>> * @disallow_set_baudrate: don't allow set_baudrate
>>> + * @has_pcm_params: whether PCM parameters need to be configured
>>> + * @pcm_params: PCM and routing parameters
>>> */
>>> struct bcm_device {
>>>      /* Must be the first member, hci_serdev.c expects this. */
>>> @@ -122,6 +124,9 @@ struct bcm_device {
>>>      bool                    is_suspended;
>>> #endif
>>>      bool                    disallow_set_baudrate;
>>> +
>>> +     bool                            has_pcm_params;
>>> +     struct bcm_set_pcm_int_params   pcm_params;
>>> };
>>> 
>>> /* generic bcm uart resources */
>>> @@ -596,6 +601,16 @@ static int bcm_setup(struct hci_uart *hu)
>>>                      host_set_baudrate(hu, speed);
>>>      }
>>> 
>>> +     /* PCM parameters if any*/
>>> +     if (bcm->dev && bcm->dev->has_pcm_params) {
>>> +             err = btbcm_set_pcm_int_params(hu->hdev, &bcm->dev->pcm_params);
>>> +
>>> +             if (err) {
>>> +                     bt_dev_info(hu->hdev, "BCM: Set pcm params failed (%d)",
>>> +                                 err);
>>> +             }
>>> +     }
>>> +
>>> finalize:
>>>      release_firmware(fw);
>>> 
>>> @@ -1132,7 +1147,24 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>> 
>>> static int bcm_of_probe(struct bcm_device *bdev)
>>> {
>>> +     int err;
>>> +
>>>      device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
>>> +
>>> +     err = device_property_read_u8(bdev->dev, "brcm,bt-sco-routing",
>>> +                                   &bdev->pcm_params.routing);
>>> +     if (!err)
>>> +             bdev->has_pcm_params = true;
>> 
>> I think in case of HCI as routing path, these should be using the default or zero values as defined by Broadcom.
> 
> I'm not sure what these default values should be. Wouldn't it be
> reasonable to expect the user/developer to set the various brcm
> parameters in device tree?
> If unset, it's just 0.

if that works with the hardware I am fine with that. The other option is to actually first read the current values. And then only change the ones that are supplied by the DT.

Regards

Marcel