Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH v3] Bluetooth: le_simult_central_peripheral experimental
 feature
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <20200706154449.3828193-1-alainm@chromium.org>
Date:   Tue, 7 Jul 2020 08:43:35 +0200
Cc:     linux-bluetooth@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <0175E6D3-8547-4633-B9A8-2AB15CE25C5B@holtmann.org>
References: <20200706154449.3828193-1-alainm@chromium.org>
To:     Alain Michaud <alainm@chromium.org>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain,

> This patch adds an le_simult_central_peripheral features which allows a
> clients to determine if the controller is able to support peripheral and
> central connections separately and at the same time.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> Changes in v3:
> - Back to the original design of V1 and integrated Marcel's feedback.
> 
> Changes in v2:
> - Slight change of design based on offline feedback
> 
> net/bluetooth/mgmt.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5e9b9728eeac..b34e6f3c07ec 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3753,12 +3753,19 @@ static const u8 debug_uuid[16] = {
> };
> #endif
> 
> +/* 671b10b5-42c0-4696-9227-eb28d1b049d6 */
> +static const u8 le_simult_central_peripheral[16] = {
> +	0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92,
> +	0x96, 0x46, 0xc0, 0x42, 0xb5, 0x10, 0x1b, 0x67,
> +};
> +
> static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 				  void *data, u16 data_len)
> {
> -	char buf[42];
> +	char buf[44];
> 	struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
> 	u16 idx = 0;
> +	u32 flags;
> 
> 	bt_dev_dbg(hdev, "sock %p", sk);
> 
> @@ -3766,7 +3773,7 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 
> #ifdef CONFIG_BT_FEATURE_DEBUG
> 	if (!hdev) {
> -		u32 flags = bt_dbg_get() ? BIT(0) : 0;
> +		flags = bt_dbg_get() ? BIT(0) : 0;

I was trying to contain the used variables in the scope here so that we don’t get funny kernel warnings about unused or uninitialized variables when options are not selected and we eventually add more here.

> 
> 		memcpy(rp->features[idx].uuid, debug_uuid, 16);
> 		rp->features[idx].flags = cpu_to_le32(flags);
> @@ -3774,6 +3781,19 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 	}
> #endif
> 
> +	flags = 0;
> +	memcpy(rp->features[idx].uuid, le_simult_central_peripheral,
> +	       sizeof(le_simult_central_peripheral));
> +
> +	if (hdev && test_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks) &&
> +	    (hdev->le_states[4] & 0x08) &&	/* Central */
> +	    (hdev->le_states[4] & 0x40) &&	/* Peripheral */
> +	    (hdev->le_states[3] & 0x10)		/* Simultaneous */)
> +		flags |= BIT(0);
> +
> +	rp->features[idx].flags = cpu_to_le32(flags);
> +	idx++;
> +
> 	rp->feature_count = cpu_to_le16(idx);

You do actually need to wrap this in

	if (hdev) {
	}

so that a) this looks cleanly and b) this is only reported when the <controller-id> is provided.

The experimental feature command is special since it can be used per hdev or globally.

> 
> 	/* After reading the experimental features information, enable
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 

Regards

Marcel