Return-Path: Content-Type: text/plain; charset=US-ASCII Mime-Version: 1.0 (Mac OS X Mail 6.5 \(1508\)) Subject: Re: [PATCH 3/6] Bluetooth: Set discovery parameters From: Marcel Holtmann In-Reply-To: Date: Wed, 21 Aug 2013 15:56:19 -0700 Cc: linux-bluetooth@vger.kernel.org Message-Id: <345ED8A3-63AC-4F6A-8D42-D1CCA6532D97@holtmann.org> References: <1376089954-13639-1-git-send-email-andre.guedes@openbossa.org> <1376089954-13639-4-git-send-email-andre.guedes@openbossa.org> To: Andre Guedes Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andre, >>> This patch adds support for setting discovery parameters through >>> debugfs filesystem. >>> >>> Signed-off-by: Andre Guedes >>> --- >>> net/bluetooth/hci_sysfs.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 44 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c >>> index 90cb53b..17ebc4a 100644 >>> --- a/net/bluetooth/hci_sysfs.c >>> +++ b/net/bluetooth/hci_sysfs.c >>> @@ -553,9 +553,52 @@ static int discovery_parameters_open(struct inode *inode, struct file *file) >>> return single_open(file, discovery_parameters_show, inode->i_private); >>> } >>> >>> +static ssize_t discovery_parameters_write(struct file *file, >>> + const char __user *data, >>> + size_t count, loff_t *offset) >>> +{ >>> + struct seq_file *sfile = file->private_data; >>> + struct hci_dev *hdev = sfile->private; >>> + struct discovery_param param; >>> + char *buf; >>> + int n; >>> + ssize_t res = 0; >>> + >>> + /* We don't allow partial writes */ >>> + if (*offset != 0) >>> + return 0; >>> + >>> + buf = kzalloc(count, GFP_KERNEL); >>> + if (!buf) >>> + return 0; >>> + >>> + if (copy_from_user(buf, data, count)) >>> + goto out; >>> + >>> + memset(¶m, 0, sizeof(param)); >>> + >>> + n = sscanf(buf, "%hhx %hx %hx %u %u %hhx %hhx", >>> + ¶m.scan_type, ¶m.scan_interval, ¶m.scan_window, >>> + ¶m.le_scan_duration, ¶m.interleaved_scan_duration, >>> + ¶m.interleaved_inquiry_length, >>> + ¶m.bredr_inquiry_length); >> >> I am not a huge fan of this crazy. I need a manual to know exactly what to do here. That is just silly. Unless things are a bit self-explanatory on how you can tweak things, I am not really interested here. > > What do you think about having the following hierarchy: > > hciX/ > | > |--> discovery_param/ > | |--> scan_type > | |--> scan_window > | |--> scan_interval > | |--> le_scan_timeout > | |--> interleaved_scan_timeout > | |--> interleaved_inquiry_length > | |--> bredr_inquiry_length > | > |--> le_conn_param/ > |--> scan_window > |--> scan_interval > |--> min_conn_interval > |--> max_conn_interval > |--> min_ce_length > |--> max_ce_length > |--> supervision_timeout > |--> conn_latency > > It sounds much self-explanatory to me. don't go crazy with the nesting here. Toplevel directory entries are just fine. The one tricky part you will run into is that things like the scan_window for connections suppose to be per device and not just global. Since you can learn many of these parameters from AD or GATT. And when connecting to one specific device, you suppose to use these ones and not some global common value. >> Also intermixing LE with BR/EDR is a bit pointless here. I would prefer if they are separate and not that BR/EDR only controller exports LE knobs and and LE only controller exports BR/EDR knobs. > > Sure. I'll fix this in the v2 patchset. We can also extend this to > others debugfs entries (e.g. inquiry_cache and auto_accept_delay files > should not be exported if controller is LE-only). Of course they should be separated. > In the current code, hdev is exported during device registration > (hci_register_dev). However, in hci_register_dev(), hdev has not been > initialized yet so we are not able to know is controller is BR/EDR, LE > or dual mode. In order to fix this, we need to postpone exporting hdev > by moving hci_add_sysfs call from hci_register_dev() to > hci_dev_open(). > > As a side effect, the hciX/ directory will be added to debugfs once > the adapter is powered on and it will be removed once the adapter is > powered off. Do you see any problem with that? That is not acceptable and also not how the kernel actually works. We are initializing the adapter no matter what. So the information is available before power on. Regards Marcel