Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: csonsino@gmail.com
Subject: Re: [PATCH 1/1] bluetooth: validate BLE connection interval updates
To:     Andreas Kemnade <andreas@kemnade.info>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org, Sasha Levin <sashal@kernel.org>
References: <f9176553-d4d5-c2e0-ecd7-937cac0d8098@gmail.com>
 <138296C5-49FA-475D-9618-FC8D241B8823@holtmann.org>
 <20190815104419.161177fa@kemnade.info>
 <0cda8242-304e-a073-90d8-63e656e3600c@gmail.com>
 <20190819180813.04a8e771@kemnade.info>
From:   Carey Sonsino <csonsino@gmail.com>
Message-ID: <624153c1-d462-1d6d-ddb2-ac7c03575a2c@gmail.com>
Date:   Mon, 19 Aug 2019 11:10:48 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190819180813.04a8e771@kemnade.info>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Andreas,

Considering that this patch has already made it's way into stable trees, 
and the only options are to back it out or to patch it again, I would 
obviously prefer to keep it in and patch it again.  I think that it 
would be fairly quick and painless to just set the default min/max to 
6/3200, or add an additional flag as you suggested, which should resolve 
any problems in the near term.

Longer term, I think that there are several ways to allow the system 
admin to configure the allowable min/max, but it only matters if the 
system will check requests against the configured min/max and respond 
appropriately, which is what the patch in question does.  The best 
current way that I'm aware of to control the system-wide connection 
interval is through the existing debugfs mechanism, and in my opinion it 
didn't work correctly without this patch.

Regarding power consumption, I have not done any power measurement 
testing to determine the effect of connection interval on power 
consumption, so perhaps that was not the best use case.  Here is the 
exact real world use case that caused me to write the patch in the first 
place:

I was writing a Linux-based test system for an embedded 
BLE-communication based product.  The product embedded code was written 
to attempt to renegotiate the connection interval to a fairly low number 
after the initial connection was established. My test system had a 
requirement to be able to perform the tests using various pre-defined 
connection intervals so that I could gather data throughput and product 
performance metrics.  Every time my test system attempted to perform a 
test a the desired connection interval, the device would immediately 
request to renegiate to a lower value.  Linux would accept that request 
and return a successful response, and I had no way to stop it.  Let's 
not go down the "Why didn't you just change the embedded code?" / "Why 
doesn't Linux reject values outside of the configured bounds?" rabbit 
hole...  :)

Carey

On 8/19/19 10:08 AM, Andreas Kemnade wrote:
> Hi Carey,
>
> On Mon, 19 Aug 2019 07:58:19 -0600
> Carey Sonsino <csonsino@gmail.com> wrote:
>
>> This seems like the exact "downside" situation that I described in the
>> patch writeup.
>>
>> I would still claim that as a Linux system administrator, I should have
>> control over my system.  If I am operating in a low power environment, I
>> do not want to allow a remote device to apply a setting which causes me
>> to use more power without any say in the matter.
>>
> In principle I agree here. High connection interval has its downsides,
> low connection interval also. Just curios: What are the numbers about
> power consumption here? A few mA? I have only compared these values on
> peripherals running on low-power SoCs like e.g. the nrf stuff from nordic.
> I see around 1 mA difference there with a power consumption besides of that
> usually measured in the µA range. Never tested these things on a linux machine.
>
> The point here is that with this patch there is insufficient control
> about this. Yes, there is the debugfs interface.
>
> But if you want to provide a driver to a gatt service living on top of
> bluetoothd dbus api? Ask people to not use distribution kernels?
> What options do you have?
> using the monitor interface to sniff the connection handle and then
> call hci_le_conn_update() to set things?
>
>> The connection min/max interval settings are configuration options that
>> control how my bluetooth device operates.  Why are these down in debugfs
>> anyway?  I think that a much more appropriate fix would be to migrate
>> some of the BLE configuration options to sysconfdir (some place like
>> /etc/bluetooth/ble.conf).  That would also help in the persistence of
>> these configuration settings, which is kind of a pain with the debugfs
>> mechanism that gets wiped out and recreated on bootup.
>>
> I think that these things should be part of the  dbus api provided
> by bluetoothd so that a driver could decide and having defaults outside
> of such a dark corner like the debug fs.
>
>> A quicker fix would be to simply set the default connection min interval
>> and connection max interval values to the full range (6, 3200).
> Or just maybe a flag allowing such behavior?
>
>> *think* that this could be done by simply updating the values in
>> hci_core.c, the hci_alloc_dev() function:
>>
>>       hdev->le_conn_min_interval = 0x0018;
>>       hdev->le_conn_max_interval = 0x0028;
>>
>> would become:
>>
>>       hdev->le_conn_min_interval = 0x0006;
>>       hdev->le_conn_max_interval = 0x0c80;
>>
>> This should allow all devices to negotiate whatever connection interval
>> they want by default.  If I'm running on a device with debugfs (which I
>> happen to be most of the time), then I can still override these defaults
>> to control my system.
>>
>> Please let me know if you would like me to do any further work towards
>> resolving this issue.  I'd be happy to test and submit a patch that
>> changes the default connection min/max interval values- I could probably
>> get that done in the next day or two.  If you would like me to
>> investigate migrating configuration settings to /etc then I'd be happy
>> to do that as well, but it might take a bit more effort and time.
>>
> Well, all these things are important, but are new features but there is a
> problem:
> The kernel patch has gone into the stable trees and from there into distributions,
> so how can these new features flow down the same path.
>
> Regards,
> Andreas
>