* [PATCH V1 3/3] Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup()
From: Dean Jenkins @ 2017-04-28 12:57 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1493384246-15381-1-git-send-email-Dean_Jenkins@mentor.com>
Before attempting to schedule a work-item onto hu->write_work in
hci_uart_tx_wakeup(), check that the Data Link protocol layer is
still bound to the HCI UART driver.
Failure to perform this protocol check causes a race condition between
the work queue hu->write_work running hci_uart_write_work() and the
Data Link protocol layer being unbound (closed) in hci_uart_tty_close().
Note hci_uart_tty_close() does have a "cancel_work_sync(&hu->write_work)"
but it is ineffective because it cannot prevent work-items being added
to hu->write_work after cancel_work_sync() has run.
Therefore, add a check for HCI_UART_PROTO_READY into hci_uart_tx_wakeup()
which prevents scheduling of the work queue when HCI_UART_PROTO_READY
is in the clear state. However, note a small race condition remains
because the hci_uart_tx_wakeup() thread can run in parallel with the
hci_uart_tty_close() thread so it is possible that a schedule of
hu->write_work can occur when HCI_UART_PROTO_READY is cleared. A complete
solution needs locking of the threads which is implemented in a future
commit.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 6f9e406..2edd305 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -125,6 +125,9 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
+ if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ return 0;
+
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
--
2.7.4
^ permalink raw reply related
* [PATCH V1 2/3] Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue()
From: Dean Jenkins @ 2017-04-28 12:57 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1493384246-15381-1-git-send-email-Dean_Jenkins@mentor.com>
Before attempting to dequeue a Data Link protocol encapsulated message,
check that the Data Link protocol is still bound to the HCI UART driver.
This makes the code consistent with the usage of the other proto
function pointers.
Therefore, add a check for HCI_UART_PROTO_READY into hci_uart_dequeue()
and return NULL if the Data Link protocol is not bound.
This is needed for robustness as there is a scheduling race condition.
hci_uart_write_work() is scheduled to run via work queue hu->write_work
from hci_uart_tx_wakeup(). Therefore, there is a delay between
scheduling hci_uart_write_work() to run and hci_uart_dequeue() running
whereby the Data Link protocol layer could become unbound during the
scheduling delay. In this case, without the check, the call to the
unbound Data Link protocol layer dequeue function can crash.
It is noted that hci_uart_tty_close() has a
"cancel_work_sync(&hu->write_work)" statement but this only reduces
the window of the race condition because it is possible for a new
work-item to be added to work queue hu->write_work after the call to
cancel_work_sync(). For example, Data Link layer retransmissions can
be added to the work queue after the cancel_work_sync() has finished.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c515aa9..6f9e406 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -113,10 +113,12 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
{
struct sk_buff *skb = hu->tx_skb;
- if (!skb)
- skb = hu->proto->dequeue(hu);
- else
+ if (!skb) {
+ if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ skb = hu->proto->dequeue(hu);
+ } else {
hu->tx_skb = NULL;
+ }
return skb;
}
--
2.7.4
^ permalink raw reply related
* [PATCH V1 1/3] Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
From: Dean Jenkins @ 2017-04-28 12:57 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1493384246-15381-1-git-send-email-Dean_Jenkins@mentor.com>
Before attempting to send a HCI message, check that the Data Link
protocol is still bound to the HCI UART driver. This makes the code
consistent with the usage of the other proto function pointers.
Therefore, add a check for HCI_UART_PROTO_READY into hci_uart_send_frame()
and return -EUNATCH if the Data Link protocol is not bound.
This also allows hci_send_frame() to report the error of an unbound
Data Link protocol layer. Therefore, it assists with diagnostics into
why HCI messages are being sent when the Data Link protocol is not
bound and avoids potential crashes.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c53513c..c515aa9 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -256,6 +256,9 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
BT_DBG("%s: type %d len %d", hdev->name, hci_skb_pkt_type(skb),
skb->len);
+ if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ return -EUNATCH;
+
hu->proto->enqueue(hu, skb);
hci_uart_tx_wakeup(hu);
--
2.7.4
^ permalink raw reply related
* [PATCH V1 0/3] hci_ldisc: inhibit "proto" function pointers, avoid crash
From: Dean Jenkins @ 2017-04-28 12:57 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
Hi Marcel,
This is my 2nd patchset in my series of resolving a design flaw in
hci_uart_tty_close() related to the proper closure of the Data Link protocol
layer which can result in a kernel crash.
The 3 patches in this patchset will unmask the design flaw in
hci_uart_tty_close() and provide a partial fix. My future patchsets based on
my original 1st patchset V2 can be discussed at a later date to get a complete
fix.
Previous Discussions
====================
Please refer to the discussion on my 1st patchset V3 that can be found here:
https://www.spinics.net/lists/linux-bluetooth/msg70315.html
You accepted the first 3 patches of my series of commits namely:
Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work()
Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev
Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY
drivers/bluetooth/hci_ldisc.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
Please refer to the discussion on my 1st patchset V2 that can be found here:
https://www.spinics.net/lists/linux-bluetooth/msg70183.html
Please refer to the discussion on my 1st RFC patchset V1 that can be found here:
https://www.spinics.net/lists/linux-bluetooth/msg70077.html
Failure test case
=================
Note this failure is probably reproducible on many series 3 and 4 kernels.
Connect a UART based Bluetooth Radio Module via a USB to serial adaptor to a
Linux PC running kernel v4.10.11. This module used the Data Link protocol called
BCSP but that is not too important because the issue is common to all the
Data Link protocols. If the Data Link protocol supports a retransmission
procedure then the probability of a kernel crash is higher.
Use the Bluez utility called btattach to try to use BCSP. Note the BCSP
implementation is incomplete in the kernel so BCSP will not establish a
connection with the Bluetooth Radio Module. However, this is ideal in exposing
the design issue in hci_uart_tty_close() because BCSP performs retransmissions.
I created this simple testcase script to trigger a kernel crash in
approximately 10 minutes of running.
#!/bin/bash
let i=1
while [ true ]
do
echo "loop $i"
./btattach -B /dev/ttyUSB0 -P bcsp -r &
sleep 5
echo "now killing..."
killall btattach
i=$(($i + 1))
done
I got the following kernel crash after 78 loops.
Here is a snippet of the dmesg log (dynamic debug used) leading up to the crash:
Note BCSP is retransmitting during hci_uart_tty_close() which increases the
probability of crashing.
[ 2932.969451] hci_uart_tty_close: tty ffff8c015f11d400
[ 2932.969455] hci_uart_close: hdev ffff8c01e3ee4000
[ 2932.969458] hci_uart_flush: hdev ffff8c01e3ee4000 tty ffff8c015f11d400
[ 2932.969460] bcsp_flush: hu ffff8c01f0bca240
[ 2932.969465] hci_unregister_dev: ffff8c01e3ee4000 name hci1 bus 3
[ 2933.154206] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 2 pkts
[ 2933.154231] hci_uart_tx_wakeup:
[ 2933.154446] bcsp_prepare_pkt: We request packet no 0 to card
[ 2933.154447] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2933.154465] bcsp_prepare_pkt: We request packet no 0 to card
[ 2933.154466] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2933.410201] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 2 pkts
[ 2933.410240] hci_uart_tx_wakeup:
[ 2933.410307] bcsp_prepare_pkt: We request packet no 0 to card
[ 2933.410308] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2933.410321] bcsp_prepare_pkt: We request packet no 0 to card
[ 2933.410322] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2933.666198] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 2 pkts
[ 2933.666225] hci_uart_tx_wakeup:
< snipped: do not show all BCSP retransmissions that occur every 250ms >
[ 2934.182902] Bluetooth: hci1 command 0x1001 tx timeout
[ 2934.182916] hci_cmd_work: hci1 cmd_cnt 1 cmd queued 1
[ 2934.182920] hci_send_frame: hci1 type 1 len 3
[ 2934.182922] hci_uart_send_frame: hci1: type 1 len 3
[ 2934.182923] hci_uart_tx_wakeup:
[ 2934.182928] bcsp_prepare_pkt: We request packet no 0 to card
[ 2934.182929] bcsp_prepare_pkt: Sending packet with seqno 2
[ 2934.434214] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 3 pkts
[ 2934.434217] hci_uart_tx_wakeup:
[ 2934.434243] bcsp_prepare_pkt: We request packet no 0 to card
[ 2934.434244] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2934.434263] bcsp_prepare_pkt: We request packet no 0 to card
[ 2934.434264] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2934.434268] bcsp_prepare_pkt: We request packet no 0 to card
[ 2934.434269] bcsp_prepare_pkt: Sending packet with seqno 2
[ 2934.690178] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 3 pkts
[ 2934.690217] hci_uart_tx_wakeup:
< snipped: do not show all BCSP retransmissions that occur every 250ms >
[ 2936.226215] Bluetooth: hci1 command 0x1009 tx timeout
[ 2936.226233] hci_cmd_work: hci1 cmd_cnt 1 cmd queued 0
[ 2936.226280] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.226282] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2936.226300] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.226301] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2936.226304] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.226306] bcsp_prepare_pkt: Sending packet with seqno 2
[ 2936.482149] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 3 pkts
[ 2936.482179] hci_uart_tx_wakeup:
[ 2936.482259] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.482261] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2936.482280] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.482281] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2936.482285] bcsp_prepare_pkt: We request packet no 0 to card
[ 2936.482286] bcsp_prepare_pkt: Sending packet with seqno 2
[ 2936.738141] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 3 pkts
[ 2936.738179] hci_uart_tx_wakeup:
< snipped: do not show all BCSP retransmissions that occur every 250ms >
[ 2940.066125] hci_uart_tx_wakeup:
[ 2940.066200] bcsp_prepare_pkt: We request packet no 0 to card
[ 2940.066202] bcsp_prepare_pkt: Sending packet with seqno 0
[ 2940.066221] bcsp_prepare_pkt: We request packet no 0 to card
[ 2940.066223] bcsp_prepare_pkt: Sending packet with seqno 1
[ 2940.066227] bcsp_prepare_pkt: We request packet no 0 to card
[ 2940.066230] bcsp_prepare_pkt: Sending packet with seqno 2
[ 2940.322113] bcsp_timed_event: hu ffff8c01f0bca240 retransmitting 3 pkts
[ 2940.322126] hci_uart_tx_wakeup:
[ 2940.322147] hci_uart_close: hdev ffff8c01e3ee4000
[ 2940.322149] hci_uart_flush: hdev ffff8c01e3ee4000 tty ffff8c015f11d400
[ 2940.322164] hci_dev_do_close: hci1 ffff8c01e3ee4000
[ 2940.322335] hci_conn_params_clear_all: All LE connection parameters were removed
[ 2940.322340] bcsp_close: hu ffff8c01f0bca240
[ 2940.322667] ftdi_set_termios: ftdi_sio ttyUSB0: Setting CS8
[ 2940.322686] BUG: unable to handle kernel NULL pointer dereference at 0000000000000044
[ 2940.322750] IP: _raw_spin_lock_irqsave+0x22/0x40
[ 2940.322779] PGD 0
[ 2940.383274] Oops: 0002 [#1] SMP
[ 2940.384001] Modules linked in: hci_uart btqca xt_set ip_set_hash_ip ip_set nf_log_ipv4 nf_log_ipv6 nf_log_common xt_LOG ip6table_nat nf_nat_ipv6 xt_recent iptable_nat nf_nat_ipv4 ip6t_REJECT nf_reject_ipv6 xt_comment ip6table_mangle ipt_REJECT nf_reject_ipv4 xt_addrtype bridge stp llc xt_mark iptable_mangle xt_tcpudp iptable_raw nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_tftp xt_CT nf_nat_snmp_basic nf_conntrack_snmp nf_nat_sip nf_nat_pptp nf_nat_proto_gre ip6table_raw nf_nat_irc xt_multiport nf_nat_h323 nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ftp nf_nat_amanda nf_nat xt_conntrack nf_conntrack_sane nf_conntrack_tftp ts_kmp nf_conntrack_sip nf_conntrack_amanda nf_conntrack_pptp nf_conntrack_proto_gre nf_conntrack_netlink nfnetlink nf_conntrack_netbios_ns nf_conntrack_broadcast nf_conntrack_irc nf_conntrack_h323
[ 2940.388162] nf_conntrack_ftp nf_conntrack ip6table_filter ip6_tables iptable_filter ip_tables x_tables lockd grace ctr ccm af_packet bnep msr arc4 brcmsmac uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 cordic btusb btrtl ftdi_sio brcmutil btbcm videobuf2_core btintel mac80211 usbserial bluetooth intel_powerclamp videodev coretemp kvm_intel kvm media snd_hda_codec_hdmi snd_hda_codec_realtek cfg80211 snd_hda_codec_generic snd_hda_intel joydev snd_hda_codec iTCO_wdt irqbypass snd_hda_core toshiba_acpi sparse_keymap iTCO_vendor_support crc32c_intel bcma industrialio snd_hwdep snd_pcm psmouse toshiba_bluetooth snd_timer input_leds serio_raw mei_me rfkill ac e1000e snd thermal wmi shpchp intel_ips battery toshiba_haps soundcore fjes lpc_ich mei ptp pps_core tpm_tis tpm_tis_core tpm cpufreq_ondemand
[ 2940.393216] cpufreq_conservative cpufreq_powersave acpi_cpufreq evdev nvram sunrpc sch_fq_codel ipv6 crc_ccitt autofs4 hid_generic usbhid hid sdhci_pci mmc_block sr_mod sdhci ehci_pci ehci_hcd mmc_core usbcore usb_common i915 video button i2c_algo_bit drm_kms_helper drm
[ 2940.395473] CPU: 3 PID: 6669 Comm: kworker/3:1 Not tainted 4.10.11_pure #1
[ 2940.396606] Hardware name: TOSHIBA Satellite R630/Portable PC, BIOS Version 1.40 07/20/2010
[ 2940.397763] Workqueue: events hci_uart_write_work [hci_uart]
[ 2940.399076] task: ffff8c01f2883600 task.stack: ffffb2a704d34000
[ 2940.400360] RIP: 0010:_raw_spin_lock_irqsave+0x22/0x40
[ 2940.401511] RSP: 0018:ffffb2a704d37da8 EFLAGS: 00010046
[ 2940.402660] RAX: 0000000000000000 RBX: 0000000000000296 RCX: ffff8c01f2883600
[ 2940.403829] RDX: 0000000000000001 RSI: ffff8c01f7cd8ba0 RDI: 0000000000000044
[ 2940.404995] RBP: ffffb2a704d37db0 R08: ffff8c01f7cd8b80 R09: 00000000000001f9
[ 2940.406157] R10: 000002ac98e2f8d0 R11: 0000000000000001 R12: 0000000000000030
[ 2940.407317] R13: 0000000000000044 R14: 0000000000000030 R15: ffff8c01f0bca780
[ 2940.408495] FS: 0000000000000000(0000) GS:ffff8c01f7cc0000(0000) knlGS:0000000000000000
[ 2940.409671] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2940.410845] CR2: 0000000000000044 CR3: 00000001306fb000 CR4: 00000000000006e0
[ 2940.412049] Call Trace:
[ 2940.413276] skb_dequeue+0x1d/0x70
[ 2940.414511] bcsp_dequeue+0x27/0x180 [hci_uart]
[ 2940.415760] hci_uart_write_work+0xc4/0x100 [hci_uart]
[ 2940.417012] process_one_work+0x151/0x410
[ 2940.418248] worker_thread+0x69/0x4b0
[ 2940.419494] kthread+0x114/0x150
[ 2940.420753] ? rescuer_thread+0x340/0x340
[ 2940.422017] ? kthread_park+0x90/0x90
[ 2940.423295] ret_from_fork+0x2c/0x40
[ 2940.424563] Code: 89 e5 e8 22 55 98 ff 5d c3 66 66 66 66 90 55 48 89 e5 53 9c 58 66 66 90 66 90 48 89 c3 fa 66 66 90 66 66 90 31 c0 ba 01 00 00 00 <f0> 0f b1 17 85 c0 75 06 48 89 d8 5b 5d c3 89 c6 e8 c9 41 98 ff
[ 2940.427448] RIP: _raw_spin_lock_irqsave+0x22/0x40 RSP: ffffb2a704d37da8
[ 2940.428956] CR2: 0000000000000044
[ 2940.435363] ---[ end trace 26e358778a5b55c5 ]---
Analysis
========
The kernel crashes because the work queue hu->write_work runs
hci_uart_write_work() which attempts to dequeue a Data Layer protocol frame
using hu->proto->dequeue(). Unfortunately, hci_uart_tty_close() has already
called hu->proto->close() which removes the resources needed by
hu->proto->dequeue() and a kernel crash occurs.
Assuming device is hci0 (my actual test used hci1).
hciattach or btattach can be used and are examples.
Example normal case scenario:
-----------------------------
1. hciconfig hci0 up ==> hci_dev_do_open() and sets HCI_UP
2. hciconfig hci0 down ==> hci_dev_do_close() and clears HCI_UP
3. killall hciattach ==> hci_dev_do_close() exits early as HCI_UP is clear
Notice that hci_dev_do_close() runs twice.
There is probably many seconds between these events so the HCI layer is likely
to be idle before hci_uart_tty_close() runs. Therefore, in an environment where
the communications with the Bluetooth Radio Module are good, the kernel crash is
very unlikely to occur.
Example failure case scenario:
------------------------------
1. hciconfig hci0 up ==> hci_dev_do_open() and sets HCI_UP
2. killall hciattach ==> hci_dev_do_close() and clears HCI_UP
In the failure case, hci_dev_do_close() attempts to perform clean-up because
HCI_UP is in the set state. hci_dev_do_close() is running in the callstack:
hci_uart_tty_close()
hci_unregister_dev()
hci_dev_do_close()
hci_uart_tty_close() is acting against hci_dev_do_close() by systematically
inhibiting various aspects of the data plane between the Data Link protocol
layer and the UART driver. This seems to be a design flaw; either allow
successful transmissions OR fully inhibit transmissions. Unfortunately, the
design is attempting both at the same time.
When hci_unregister_dev() returns to hci_uart_tty_close(), 1 or
more HCI commands might have been queued inside the Data Link protocol layer by
hu->proto->enqueue(). The Data Link protocol layer may attempt to periodically
retransmit these HCI commands.
In any case, attempts at sending any HCI commands will fail during the
execution of hci_uart_tty_close() because the data plane between the Data Link
protocol Layer and the UART driver has been cut in various places including
inside the TTY layer.
The weakness in hci_uart_tty_close() is that hu->proto->close() is not prevented
from running before or during the execution of the other "proto" function
pointers such as hu->proto->dequeue(). Therefore a race condition exists.
Solution implemented by this patchset
=====================================
When the flag HCI_UART_PROTO_READY is in the clear state, this means the "proto"
layer is no longer ready. Therefore use HCI_UART_PROTO_READY to prevent any
"proto" function pointer except hu->proto->close() from running.
Already protected:
p->open() in hci_uart_set_proto()
hu->proto->flush() in hci_uart_flush()
hu->proto->close() in hci_uart_tty_close()
p->close() in hci_uart_set_proto()
hu->proto->close() in hci_uart_init_work()
The following "proto" functions pointers are now protected by the patchset:
hu->proto->enqueue() in hci_uart_send_frame()
hu->proto->dequeue() in hci_uart_dequeue()
by the implementations in the patches:
Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue()
This protection helps to avoid the kernel crash by ensuring that a "proto"
function pointer except hu->proto->close() does not run after
HCI_UART_PROTO_READY has been cleared in hci_uart_tty_close().
However, a complete solution (not implemented in this patchset) requires locking
to prevent hu->proto->close() from running whilst another "proto" function
pointer is already running. The flag HCI_UART_PROTO_READY is insufficient
because another "proto" function pointer may already be running at the point the
flag is cleared.
In addition the patch:
Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup()
prevents the scheduling of new or retransmitted HCI commands from being queued
on the work queue hu->write_work so that hci_uart_write_work() is not able to
run when HCI_UART_PROTO_READY is in the cleared state. This inhibits
transmission attempts so reduces the probability of hci_uart_write_work()
running after hu->proto->close() has run.
Test results with patchset applied
==================================
snippet of dmesg log
[ 1122.904072] hci_uart_tty_close: tty ffff91b59c41c000
[ 1122.904075] hci_uart_close: hdev ffff91b59c456000
[ 1122.904078] hci_uart_flush: hdev ffff91b59c456000 tty ffff91b59c41c000
[ 1122.904080] bcsp_flush: hu ffff91b5a29340c0
[ 1122.904085] hci_unregister_dev: ffff91b59c456000 name hci1 bus 3
[ 1122.984911] bcsp_timed_event: hu ffff91b5a29340c0 retransmitting 2 pkts
[ 1124.520958] Bluetooth: hci1 command 0x1001 tx timeout
[ 1124.521009] hci_cmd_work: hci1 cmd_cnt 1 cmd queued 1
[ 1124.521013] hci_send_frame: hci1 type 1 len 3
[ 1124.521015] hci_uart_send_frame: hci1: type 1 len 3
[ 1124.521016] Bluetooth: hci1 sending frame failed (-49)
[ 1126.569048] Bluetooth: hci1 command 0x1009 tx timeout
[ 1126.569065] hci_cmd_work: hci1 cmd_cnt 1 cmd queued 0
[ 1130.665151] hci_uart_close: hdev ffff91b59c456000
[ 1130.665154] hci_uart_flush: hdev ffff91b59c456000 tty ffff91b59c41c000
[ 1130.665173] hci_dev_do_close: hci1 ffff91b59c456000
[ 1130.665357] hci_conn_params_clear_all: All LE connection parameters were removed
[ 1130.665361] bcsp_close: hu ffff91b5a29340c0
Notice the following error message is now output:
Bluetooth: hci1 sending frame failed (-49)
This is because the patch
Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
is preventing hci_send_frame() from calling hu->proto->enqueue() after
hci_uart_tty_close() has cleared the flag HCI_UART_PROTO_READY. Therefore,
no new HCI commands can be queued within the Data Link protocol layer because
the Data Link protocol layer is pending to be closed.
Conclusions
===========
This patchset demonstrates the following design issues:
1. Attempts are made at trying to send HCI commands during hci_uart_tty_close()
but this is in conflict with the hci_uart_tty_close() closure procedures.
2. Data Link protocol layer retransmission attempts can occur during
hci_uart_tty_close() to resend HCI commands but this is in conflict with the
hci_uart_tty_close() closure procedures.
3. Sending of HCI commands fail anyway during hci_uart_tty_close() because
the data plane between the Data Link protocol layer and the UART driver has
been cut in various places including inside the TTY layer. Therefore the
attempts at sending HCI commands during hci_uart_tty_close() are futile.
This means that, currently the design is attempting to send HCI commands AND
trying to inhibit transmissions at the same time. Therefore, there is a conflict
in the procedures.
The design of hci_uart_tty_close() needs to be changed to either
a) allow sucessful transmission of HCI commands and implement a clean controlled
closure of the Data Link protocol.
OR
b) abruptly inhibit further communications of the Data Link protocol.
Side-effects
------------
The patchset should be safe although a side-effect could be some occassional
error messages:
Bluetooth: hci1 sending frame failed (-49)
Note hci_send_frame() does not propagate the -EUNATCH (-49) error code so takes
no specific action. Therefore, hci_core.c handling is not modified by the
patchset.
Dean Jenkins (3):
Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue()
Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup()
drivers/bluetooth/hci_ldisc.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v2] Bluetooth: Add module license for HCI UART Nokia H4+
From: Frédéric Danis @ 2017-04-28 11:55 UTC (permalink / raw)
To: sre; +Cc: linux-bluetooth, frederic.danis.oss
Fix the following error preventing to load Nokia H4+ module:
kernel: [ 826.461619] hci_nokia: module license 'unspecified' taints kernel.
kernel: [ 826.461629] Disabling lock debugging due to kernel taint
kernel: [ 826.461836] hci_nokia: Unknown symbol gpiod_get_value_cansleep (err 0)
kernel: [ 826.461876] hci_nokia: Unknown symbol devm_kmalloc (err 0)
kernel: [ 826.461908] hci_nokia: Unknown symbol gpiod_set_value (err 0)
kernel: [ 826.461937] hci_nokia: Unknown symbol serdev_device_set_baudrate (err 0)
kernel: [ 826.461994] hci_nokia: Unknown symbol gpiod_set_value_cansleep (err 0)
kernel: [ 826.462021] hci_nokia: Unknown symbol hci_uart_tx_wakeup (err 0)
kernel: [ 826.462043] hci_nokia: Unknown symbol serdev_device_set_flow_control (err 0)
kernel: [ 826.462064] hci_nokia: Unknown symbol gpiod_to_irq (err 0)
kernel: [ 826.462085] hci_nokia: Unknown symbol serdev_device_open (err 0)
kernel: [ 826.462106] hci_nokia: Unknown symbol gpiod_get_value (err 0)
kernel: [ 826.462150] hci_nokia: Unknown symbol clk_prepare (err 0)
kernel: [ 826.462182] hci_nokia: Unknown symbol pm_runtime_enable (err 0)
kernel: [ 826.462204] hci_nokia: Unknown symbol h4_recv_buf (err 0)
kernel: [ 826.462246] hci_nokia: Unknown symbol serdev_device_write_flush (err 0)
kernel: [ 826.462268] hci_nokia: Unknown symbol serdev_device_get_tiocm (err 0)
kernel: [ 826.462298] hci_nokia: Unknown symbol driver_unregister (err 0)
kernel: [ 826.462318] hci_nokia: Unknown symbol serdev_device_wait_until_sent (err 0)
kernel: [ 826.462347] hci_nokia: Unknown symbol __serdev_device_driver_register (err 0)
kernel: [ 826.462384] hci_nokia: Unknown symbol serdev_device_set_tiocm (err 0)
kernel: [ 826.462417] hci_nokia: Unknown symbol clk_get_rate (err 0)
kernel: [ 826.462454] hci_nokia: Unknown symbol __pm_runtime_resume (err 0)
kernel: [ 826.462486] hci_nokia: Unknown symbol serdev_device_close (err 0)
kernel: [ 826.462524] hci_nokia: Unknown symbol cancel_work_sync (err 0)
kernel: [ 826.462546] hci_nokia: Unknown symbol btbcm_set_bdaddr (err 0)
kernel: [ 826.462567] hci_nokia: Unknown symbol clk_disable (err 0)
kernel: [ 826.462610] hci_nokia: Unknown symbol __pm_runtime_disable (err 0)
kernel: [ 826.462632] hci_nokia: Unknown symbol hci_uart_register_device (err 0)
kernel: [ 826.462653] hci_nokia: Unknown symbol clk_enable (err 0)
kernel: [ 826.462675] hci_nokia: Unknown symbol __pm_runtime_idle (err 0)
kernel: [ 826.462700] hci_nokia: Unknown symbol clk_unprepare (err 0)
Acked-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
---
Changes since PATCH v1:
* Fix typo in MODULE_AUTHOR
---
drivers/bluetooth/hci_nokia.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index 4038daf..a7d687d8 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -36,6 +36,8 @@
#include "hci_uart.h"
#include "btbcm.h"
+#define VERSION "0.1"
+
#define NOKIA_ID_BCM2048 0x04
#define NOKIA_ID_TI1271 0x31
@@ -818,3 +820,8 @@ static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
};
module_serdev_device_driver(nokia_bluetooth_serdev_driver);
+
+MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>");
+MODULE_DESCRIPTION("Bluetooth HCI UART Nokia H4+ driver ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Add module license for HCI UART Nokia H4+
From: Sebastian Reichel @ 2017-04-28 11:44 UTC (permalink / raw)
To: Frédéric Danis; +Cc: linux-bluetooth
In-Reply-To: <1493378771-22557-1-git-send-email-frederic.danis.oss@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3757 bytes --]
Hi,
On Fri, Apr 28, 2017 at 01:26:11PM +0200, Frédéric Danis wrote:
> Fix the following error preventing to load Nokia H4+ module:
> kernel: [ 826.461619] hci_nokia: module license 'unspecified' taints kernel.
> kernel: [ 826.461629] Disabling lock debugging due to kernel taint
> kernel: [ 826.461836] hci_nokia: Unknown symbol gpiod_get_value_cansleep (err 0)
> kernel: [ 826.461876] hci_nokia: Unknown symbol devm_kmalloc (err 0)
> kernel: [ 826.461908] hci_nokia: Unknown symbol gpiod_set_value (err 0)
> kernel: [ 826.461937] hci_nokia: Unknown symbol serdev_device_set_baudrate (err 0)
> kernel: [ 826.461994] hci_nokia: Unknown symbol gpiod_set_value_cansleep (err 0)
> kernel: [ 826.462021] hci_nokia: Unknown symbol hci_uart_tx_wakeup (err 0)
> kernel: [ 826.462043] hci_nokia: Unknown symbol serdev_device_set_flow_control (err 0)
> kernel: [ 826.462064] hci_nokia: Unknown symbol gpiod_to_irq (err 0)
> kernel: [ 826.462085] hci_nokia: Unknown symbol serdev_device_open (err 0)
> kernel: [ 826.462106] hci_nokia: Unknown symbol gpiod_get_value (err 0)
> kernel: [ 826.462150] hci_nokia: Unknown symbol clk_prepare (err 0)
> kernel: [ 826.462182] hci_nokia: Unknown symbol pm_runtime_enable (err 0)
> kernel: [ 826.462204] hci_nokia: Unknown symbol h4_recv_buf (err 0)
> kernel: [ 826.462246] hci_nokia: Unknown symbol serdev_device_write_flush (err 0)
> kernel: [ 826.462268] hci_nokia: Unknown symbol serdev_device_get_tiocm (err 0)
> kernel: [ 826.462298] hci_nokia: Unknown symbol driver_unregister (err 0)
> kernel: [ 826.462318] hci_nokia: Unknown symbol serdev_device_wait_until_sent (err 0)
> kernel: [ 826.462347] hci_nokia: Unknown symbol __serdev_device_driver_register (err 0)
> kernel: [ 826.462384] hci_nokia: Unknown symbol serdev_device_set_tiocm (err 0)
> kernel: [ 826.462417] hci_nokia: Unknown symbol clk_get_rate (err 0)
> kernel: [ 826.462454] hci_nokia: Unknown symbol __pm_runtime_resume (err 0)
> kernel: [ 826.462486] hci_nokia: Unknown symbol serdev_device_close (err 0)
> kernel: [ 826.462524] hci_nokia: Unknown symbol cancel_work_sync (err 0)
> kernel: [ 826.462546] hci_nokia: Unknown symbol btbcm_set_bdaddr (err 0)
> kernel: [ 826.462567] hci_nokia: Unknown symbol clk_disable (err 0)
> kernel: [ 826.462610] hci_nokia: Unknown symbol __pm_runtime_disable (err 0)
> kernel: [ 826.462632] hci_nokia: Unknown symbol hci_uart_register_device (err 0)
> kernel: [ 826.462653] hci_nokia: Unknown symbol clk_enable (err 0)
> kernel: [ 826.462675] hci_nokia: Unknown symbol __pm_runtime_idle (err 0)
> kernel: [ 826.462700] hci_nokia: Unknown symbol clk_unprepare (err 0)
>
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> ---
> drivers/bluetooth/hci_nokia.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
> index 4038daf..c563f3c 100644
> --- a/drivers/bluetooth/hci_nokia.c
> +++ b/drivers/bluetooth/hci_nokia.c
> @@ -36,6 +36,8 @@
> #include "hci_uart.h"
> #include "btbcm.h"
>
> +#define VERSION "0.1"
> +
> #define NOKIA_ID_BCM2048 0x04
> #define NOKIA_ID_TI1271 0x31
>
> @@ -818,3 +820,8 @@ static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
> };
>
> module_serdev_device_driver(nokia_bluetooth_serdev_driver);
> +
> +MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>-");
There is a superfluous - at end of string.
> +MODULE_DESCRIPTION("Bluetooth HCI UART Nokia H4+ driver ver " VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
Otherwise:
Acked-by: Sebastian Reichel <sre@kernel.org>
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] Bluetooth: Add module license for HCI UART Nokia H4+
From: Frédéric Danis @ 2017-04-28 11:26 UTC (permalink / raw)
To: sre; +Cc: linux-bluetooth, frederic.danis.oss
Fix the following error preventing to load Nokia H4+ module:
kernel: [ 826.461619] hci_nokia: module license 'unspecified' taints kernel.
kernel: [ 826.461629] Disabling lock debugging due to kernel taint
kernel: [ 826.461836] hci_nokia: Unknown symbol gpiod_get_value_cansleep (err 0)
kernel: [ 826.461876] hci_nokia: Unknown symbol devm_kmalloc (err 0)
kernel: [ 826.461908] hci_nokia: Unknown symbol gpiod_set_value (err 0)
kernel: [ 826.461937] hci_nokia: Unknown symbol serdev_device_set_baudrate (err 0)
kernel: [ 826.461994] hci_nokia: Unknown symbol gpiod_set_value_cansleep (err 0)
kernel: [ 826.462021] hci_nokia: Unknown symbol hci_uart_tx_wakeup (err 0)
kernel: [ 826.462043] hci_nokia: Unknown symbol serdev_device_set_flow_control (err 0)
kernel: [ 826.462064] hci_nokia: Unknown symbol gpiod_to_irq (err 0)
kernel: [ 826.462085] hci_nokia: Unknown symbol serdev_device_open (err 0)
kernel: [ 826.462106] hci_nokia: Unknown symbol gpiod_get_value (err 0)
kernel: [ 826.462150] hci_nokia: Unknown symbol clk_prepare (err 0)
kernel: [ 826.462182] hci_nokia: Unknown symbol pm_runtime_enable (err 0)
kernel: [ 826.462204] hci_nokia: Unknown symbol h4_recv_buf (err 0)
kernel: [ 826.462246] hci_nokia: Unknown symbol serdev_device_write_flush (err 0)
kernel: [ 826.462268] hci_nokia: Unknown symbol serdev_device_get_tiocm (err 0)
kernel: [ 826.462298] hci_nokia: Unknown symbol driver_unregister (err 0)
kernel: [ 826.462318] hci_nokia: Unknown symbol serdev_device_wait_until_sent (err 0)
kernel: [ 826.462347] hci_nokia: Unknown symbol __serdev_device_driver_register (err 0)
kernel: [ 826.462384] hci_nokia: Unknown symbol serdev_device_set_tiocm (err 0)
kernel: [ 826.462417] hci_nokia: Unknown symbol clk_get_rate (err 0)
kernel: [ 826.462454] hci_nokia: Unknown symbol __pm_runtime_resume (err 0)
kernel: [ 826.462486] hci_nokia: Unknown symbol serdev_device_close (err 0)
kernel: [ 826.462524] hci_nokia: Unknown symbol cancel_work_sync (err 0)
kernel: [ 826.462546] hci_nokia: Unknown symbol btbcm_set_bdaddr (err 0)
kernel: [ 826.462567] hci_nokia: Unknown symbol clk_disable (err 0)
kernel: [ 826.462610] hci_nokia: Unknown symbol __pm_runtime_disable (err 0)
kernel: [ 826.462632] hci_nokia: Unknown symbol hci_uart_register_device (err 0)
kernel: [ 826.462653] hci_nokia: Unknown symbol clk_enable (err 0)
kernel: [ 826.462675] hci_nokia: Unknown symbol __pm_runtime_idle (err 0)
kernel: [ 826.462700] hci_nokia: Unknown symbol clk_unprepare (err 0)
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
---
drivers/bluetooth/hci_nokia.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index 4038daf..c563f3c 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -36,6 +36,8 @@
#include "hci_uart.h"
#include "btbcm.h"
+#define VERSION "0.1"
+
#define NOKIA_ID_BCM2048 0x04
#define NOKIA_ID_TI1271 0x31
@@ -818,3 +820,8 @@ static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
};
module_serdev_device_driver(nokia_bluetooth_serdev_driver);
+
+MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>-");
+MODULE_DESCRIPTION("Bluetooth HCI UART Nokia H4+ driver ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* Re: [PATCH BlueZ] shared/att: Fix responding to unknown command opcode
From: Luiz Augusto von Dentz @ 2017-04-27 13:02 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <EDC2CC27-E6A3-4D22-8363-2C9552A599C8@holtmann.org>
Hi Marcel,
On Wed, Apr 26, 2017 at 5:49 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> In case of receiving an unknown command no response shall be generated.
>> ---
>> src/shared/att.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/shared/att.c b/src/shared/att.c
>> index 3071b51..10e182f 100644
>> --- a/src/shared/att.c
>> +++ b/src/shared/att.c
>> @@ -148,7 +148,7 @@ static enum att_op_type get_op_type(uint8_t opcode)
>> return att_opcode_type_table[i].type;
>> }
>>
>> - return ATT_OP_TYPE_UNKNOWN;
>> + return opcode & ATT_OP_CMD_MASK ? ATT_OP_TYPE_CMD : ATT_OP_TYPE_UNKNOWN;
>> }
>
> for readability I would use this:
>
> return (opcode & ATT_OP_CMD_MASK) ? ATT_OP_TYPE_CMD : ATT_OP_TYPE_UNKNOWN;
>
> Or just do this:
>
> if (opcode & ATT_OP_CMD_MASK)
> return ATT_OP_TYPE_CMD;
>
> return ATT_OP_TYPE_UNKNOWN;
Ive made these modifications above and applied the patch.
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH] Bluetooth: allocate data for kpp on heap
From: Marcel Holtmann @ 2017-04-27 10:30 UTC (permalink / raw)
To: Szymon Janc
Cc: Salvatore Benedetto, Gustavo F. Padovan, Linux Bluetooth,
Herbert Xu, Johan Hedberg
In-Reply-To: <1899161.UMzm0Rcsma@ix>
Hi Szymon,
>> Bluetooth would crash when computing ECDH keys with kpp
>> if VMAP_STACK is enabled. Fix by allocating data passed
>> to kpp on heap.
>>
>> Fixes: 58771c1c ("Bluetooth: convert smp and selftest to crypto kpp
>> API")
>> Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
>> ---
>> net/bluetooth/ecdh_helper.c | 6 ++++--
>> net/bluetooth/selftest.c | 16 +++++++++++-----
>> 2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
>> index b6d9aa1..8018447 100644
>> --- a/net/bluetooth/ecdh_helper.c
>> +++ b/net/bluetooth/ecdh_helper.c
>> @@ -59,7 +59,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8
>> private_key[32], struct ecdh p;
>> struct ecdh_completion result;
>> struct scatterlist src, dst;
>> - u8 tmp[64];
>> + u8 *tmp = kmalloc(64, GFP_KERNEL);
>
> Should this be checked for null?
possible yes, how do other crypto users did this when they converted to VMAP_STACK support. We should do exactly the same as others did.
Regards
Marcel
^ permalink raw reply
* Re: Serial Port connection with DBus API
From: Barry Byford @ 2017-04-26 22:47 UTC (permalink / raw)
To: Bluez mailing list
In-Reply-To: <20170426190728.GA14369@x1c>
Hello Johan,
Thank you very much for taking the time to respond. It is very much appreciated.
On 26 April 2017 at 20:07, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Barry,
>
> On Tue, Apr 25, 2017, Barry Byford wrote:
>> I don't feel I'm making progress on this so I've attached the output
>> from btmon in the hope that someone has the time to cast an eye over
>> it.
>>
>> The values that are received into NewConnection are:
>> object device = /org/bluez/hci0/dev_64_BC_0C_F6_22_F8
>> fd = 0
>> dict fd_properties = {}
>>
>> if I do a os.isatty(fd) then I get returned True
>> if I do a os.ttyname(fd) then I get returned /dev/pts/0
>
> Our test/test-profile script does the following:
>
> def NewConnection(self, path, fd, properties):
> self.fd = fd.take()
Yes I had seen this and it confused me. fd has a signature of 'h'
which is a gint32 according to
https://developer.gnome.org/glib/stable/gvariant-format-strings.html
Integers have no method take() as part of it.
> Are you doing something similar? Otherwise it looks like file descriptor
> passing is somehow broken in your system.
On my system I don't have dbus-python as I am using the preferred
pydbus library.
I say preferred based on the comments at the bottom of
https://www.freedesktop.org/wiki/Software/DBusBindings/
Following your posting I have configured a new system and ran
test/test-profile using dbus-python
Running on the new system fd comes in to NewConnection as type 'dbus.UnixFd'
This type of object does have a take() method
https://dbus.freedesktop.org/doc/dbus-python/api/dbus.types.UnixFd-class.html
This seems to be consistent with type 'h' defined here
https://dbus.freedesktop.org/doc/dbus-specification.html#basic-types
I think this moves my investigation away from BlueZ and on to the
various Python DBus implementations.
Thanks again for your input.
^ permalink raw reply
* Re: Serial Port connection with DBus API
From: Johan Hedberg @ 2017-04-26 19:07 UTC (permalink / raw)
To: Barry Byford; +Cc: Bluez mailing list
In-Reply-To: <CAAu3APYz+fP-f2S3XqV1EJLs4d=4-URTMG-bffNZmZNFQE+ROQ@mail.gmail.com>
Hi Barry,
On Tue, Apr 25, 2017, Barry Byford wrote:
> I don't feel I'm making progress on this so I've attached the output
> from btmon in the hope that someone has the time to cast an eye over
> it.
>
> The values that are received into NewConnection are:
> object device = /org/bluez/hci0/dev_64_BC_0C_F6_22_F8
> fd = 0
> dict fd_properties = {}
>
> if I do a os.isatty(fd) then I get returned True
> if I do a os.ttyname(fd) then I get returned /dev/pts/0
Our test/test-profile script does the following:
def NewConnection(self, path, fd, properties):
self.fd = fd.take()
Are you doing something similar? Otherwise it looks like file descriptor
passing is somehow broken in your system.
Johan
^ permalink raw reply
* Re: bluetooth 6lowpan interfaces are not virtual anymore
From: Jukka Rissanen @ 2017-04-26 16:52 UTC (permalink / raw)
To: Michael Richardson, Alexander Aring
Cc: Network Development, Luiz Augusto von Dentz,
linux-wpan@vger.kernel.org, Linux Bluetooth
In-Reply-To: <383.1493218546@dooku.sandelman.ca>
Hi Michael,
On Wed, 2017-04-26 at 10:55 -0400, Michael Richardson wrote:
> Alexander Aring <aar@pengutronix.de> wrote:
> >> In a classic SVR4 STREAMS works, it would have been just
> another
> >> module. (No, I'm not a fan of *STREAMS* or of SVR4 in
> general,
> >> although I liked some of the ideas).
> >>
>
> > ok, I see you complain about "having a virtual on top of wpan
> > interface", or?
>
> > I wanted to talk at first about the queue handling which is
> introduced
> > when 6LoWPAN is not a virtual interface anymore. Or do you want
> to have
> > a queue in front of 6lowpan adaptation (see other mail reply
> with ASCII
> > graphics).
>
> I would like to have a single queue, as close to the hardware as
> possible,
> such that BQL can do it's thing easily. Should we rethink outgoing
> fragment
> handling for 6lowpan? Clearly the BT people had a need.
> I don't think they've had a chance to respond to your complaints.
Note that the BT fragmentation (or actually it is called segmentation
in BT) is totally different what 802.15.4 is doing. I do not think
there is any need to add fragmentation handling into 6lo.
Actually the 6lowpan kernel module could probably be simplified to be a
library. We did this in Zephyr where we have compress() and
uncompress() functions that do all the magic.
>
> > We can change that you can run multiple interfaces on one
> > PHY. Currently we just allow one, because address filtering.
> Disable
> > address filtering
> > we will loose ACK handling on hardware.
>
> Yes, that's a limitation of some hardware, and if you enable multiple
> PANIDs,
> that might be the consequence....
>
> > I can try to implement all stuff in software "for fun, maybe
> see what
> > we can do to handle ACK in software, etc" Then you can run
> multiple
>
> I'm not asking you to do it, I'm asking, now that we've gotten to a
> certain
> point, we have a better idea what the various requirements are, and
> can we
> re-evaluate things and maybe tweak some things.
>
> --
> ] Never tell me the odds! | ipv6 mesh
> networks [
> ] Michael Richardson, Sandelman Software Works | network
> architect [
> ] mcr@sandelman.ca http://www.sandelman.ca/ | ruby on
> rails [
>
Cheers,
Jukka
^ permalink raw reply
* Re: bluetooth 6lowpan interfaces are not virtual anymore
From: Michael Richardson @ 2017-04-26 15:05 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Alexander Aring, Network Development, Jukka Rissanen,
linux-wpan@vger.kernel.org, Linux Bluetooth
In-Reply-To: <CABBYNZJqChBfB4jNCN3edmKSfhNd-haTKOjeYy1toK-Mn=qzoA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]
I said that BT people hadn't replied, but it was next in my inbox :-)
Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> At least if you want to try to make a more efficient qdisc handling,
>> means dropping skb's in queue when userspace sends too much. You will
>> change it back, control two queues seems to be difficult.
> It is wrong only if you look at 6LoWPAN interface as being owned by
> 6lowpan modules, but it doesn't, in fact it won't anything by itself so
> it basically acts as a library to that perform 6LoWPAN operation on the
> packet level, thus why it is possible to switch from virtual to real
> interface. Also in terms of 15.4, the 6LoWPAN interfaces you depicted
> above is also no controlled by 6LoWPAN itself, in fact it seeems to
> represent links in 15.4, and I also assume these links may not use
> 6LoWPAN.
At present I do not think we support any 15.4 links which are not 6lowpan in the Kernel.
Perhaps a userspace can open the device in raw mode and do stuff with it
directly. Do such users exist right now?
>> This queue should have the same meaning like our queue in IEEE
>> 802.15.4 interface which you mentioned above. At this point, we have
>> no difference. There exists already a queue for your real transeiver
>> hardware.
> The queue here is per channel, btw the queue size is not decided by us
> it is the remote side that provides the tx credits so to some extend
> the tx_queue is actually the remote queue in case of CoC. When testing
> this it was quite clear this does not work in practice, with IPv6 at
> least, because the remote side may not have enough credits for a single
> packet (MPS * tx_credits < MTU) which means without proper queueing
> support we would be dropping every packet.
Would a longer amortization interval help this?
It seems that if there isn't bandwidth to send the packet, that userspace has
to learn about this fact...
>> At least, having a queue and don't put anything anymore into the queue
>> when "tx credits" is at zero (because queue is full). _Is_ a kind of
>> qdisc handling.
> The tx_credits operate at L2CAP segment level (MPS), so it is at a
> completely different level, there are no guarantees the credits will
> attend to a single IP packet.
That seems like a problem.
I think that we need to either send the entire packet, or none.
Spreading the credits across two packets would just make the system stutter,
as an upper layer retransmits packets that were half-sent, but then delayed.
Is there someway we can wait, at the IP packet queue level, until there are
sufficient credits to send all the fragments? That way, the IP qdisc layer
can decide to abort(drop,defer) an IP packet from the queue in favour of a more
important packet.
--
] Never tell me the odds! | ipv6 mesh networks [
] Michael Richardson, Sandelman Software Works | network architect [
] mcr@sandelman.ca http://www.sandelman.ca/ | ruby on rails [
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply
* Re: bluetooth 6lowpan interfaces are not virtual anymore
From: Michael Richardson @ 2017-04-26 14:55 UTC (permalink / raw)
To: Alexander Aring
Cc: Network Development, Jukka Rissanen, Luiz Augusto von Dentz,
linux-wpan@vger.kernel.org, Linux Bluetooth
In-Reply-To: <11cf222a-0c1f-4136-091d-719ee68824e1@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]
Alexander Aring <aar@pengutronix.de> wrote:
>> In a classic SVR4 STREAMS works, it would have been just another
>> module. (No, I'm not a fan of *STREAMS* or of SVR4 in general,
>> although I liked some of the ideas).
>>
> ok, I see you complain about "having a virtual on top of wpan
> interface", or?
> I wanted to talk at first about the queue handling which is introduced
> when 6LoWPAN is not a virtual interface anymore. Or do you want to have
> a queue in front of 6lowpan adaptation (see other mail reply with ASCII
> graphics).
I would like to have a single queue, as close to the hardware as possible,
such that BQL can do it's thing easily. Should we rethink outgoing fragment
handling for 6lowpan? Clearly the BT people had a need.
I don't think they've had a chance to respond to your complaints.
> We can change that you can run multiple interfaces on one
> PHY. Currently we just allow one, because address filtering. Disable
> address filtering
> we will loose ACK handling on hardware.
Yes, that's a limitation of some hardware, and if you enable multiple PANIDs,
that might be the consequence....
> I can try to implement all stuff in software "for fun, maybe see what
> we can do to handle ACK in software, etc" Then you can run multiple
I'm not asking you to do it, I'm asking, now that we've gotten to a certain
point, we have a better idea what the various requirements are, and can we
re-evaluate things and maybe tweak some things.
--
] Never tell me the odds! | ipv6 mesh networks [
] Michael Richardson, Sandelman Software Works | network architect [
] mcr@sandelman.ca http://www.sandelman.ca/ | ruby on rails [
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply
* Re: [PATCH BlueZ] shared/att: Fix responding to unknown command opcode
From: Marcel Holtmann @ 2017-04-26 14:49 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <20170426141917.9727-1-luiz.dentz@gmail.com>
Hi Luiz,
> In case of receiving an unknown command no response shall be generated.
> ---
> src/shared/att.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 3071b51..10e182f 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -148,7 +148,7 @@ static enum att_op_type get_op_type(uint8_t opcode)
> return att_opcode_type_table[i].type;
> }
>
> - return ATT_OP_TYPE_UNKNOWN;
> + return opcode & ATT_OP_CMD_MASK ? ATT_OP_TYPE_CMD : ATT_OP_TYPE_UNKNOWN;
> }
for readability I would use this:
return (opcode & ATT_OP_CMD_MASK) ? ATT_OP_TYPE_CMD : ATT_OP_TYPE_UNKNOWN;
Or just do this:
if (opcode & ATT_OP_CMD_MASK)
return ATT_OP_TYPE_CMD;
return ATT_OP_TYPE_UNKNOWN;
Regards
Marcel
^ permalink raw reply
* [PATCH BlueZ] shared/att: Fix responding to unknown command opcode
From: Luiz Augusto von Dentz @ 2017-04-26 14:19 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
In case of receiving an unknown command no response shall be generated.
---
src/shared/att.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/shared/att.c b/src/shared/att.c
index 3071b51..10e182f 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -148,7 +148,7 @@ static enum att_op_type get_op_type(uint8_t opcode)
return att_opcode_type_table[i].type;
}
- return ATT_OP_TYPE_UNKNOWN;
+ return opcode & ATT_OP_CMD_MASK ? ATT_OP_TYPE_CMD : ATT_OP_TYPE_UNKNOWN;
}
static const struct {
--
2.9.3
^ permalink raw reply related
* [PATCH BlueZ v2] core/gatt-client: Add support for Includes property
From: Marcin Kraglak @ 2017-04-26 12:32 UTC (permalink / raw)
To: linux-bluetooth
Add implementation of Includes property in GATT service interface.
Include services are updated after exporting all services, when new service
has been added or service was removed.
---
src/gatt-client.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 106 insertions(+), 7 deletions(-)
diff --git a/src/gatt-client.c b/src/gatt-client.c
index 114981c..b86bfe6 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -72,6 +72,7 @@ struct service {
bt_uuid_t uuid;
char *path;
struct queue *chrcs;
+ struct queue *incl_services;
};
typedef bool (*async_dbus_op_complete_t)(void *data);
@@ -1398,10 +1399,36 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
return TRUE;
}
+static void append_incl_service_path(void *data, void *user_data)
+{
+ struct service *incl_service = data;
+ DBusMessageIter *array = user_data;
+
+ dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
+ &incl_service->path);
+}
+
+static gboolean service_get_includes(const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct service *service = data;
+ DBusMessageIter array;
+
+ dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
+
+ queue_foreach(service->incl_services, append_incl_service_path, &array);
+
+ dbus_message_iter_close_container(iter, &array);
+
+ return TRUE;
+
+}
+
static const GDBusPropertyTable service_properties[] = {
{ "UUID", "s", service_get_uuid },
{ "Device", "o", service_get_device },
{ "Primary", "b", service_get_primary },
+ { "Includes", "ao", service_get_includes },
{ }
};
@@ -1410,6 +1437,7 @@ static void service_free(void *data)
struct service *service = data;
queue_destroy(service->chrcs, NULL); /* List should be empty here */
+ queue_destroy(service->incl_services, NULL);
g_free(service->path);
free(service);
}
@@ -1423,6 +1451,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
service = new0(struct service, 1);
service->chrcs = queue_new();
+ service->incl_services = queue_new();
service->client = client;
gatt_db_attribute_get_service_data(attr, &service->start_handle,
@@ -1459,13 +1488,29 @@ static struct service *service_create(struct gatt_db_attribute *attr,
return service;
}
+static void on_service_removed(void *data, void *user_data)
+{
+ struct service *service = data;
+ struct service *removed_service = user_data;
+
+ if (queue_remove(service->incl_services, removed_service))
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ service->path,
+ GATT_SERVICE_IFACE,
+ "Includes");
+}
+
static void unregister_service(void *data)
{
struct service *service = data;
+ struct btd_gatt_client *client = service->client;
DBG("Removing GATT service: %s", service->path);
queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
+ queue_remove_all(service->incl_services, NULL, NULL, NULL);
+
+ queue_foreach(client->services, on_service_removed, service);
g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
GATT_SERVICE_IFACE);
@@ -1567,11 +1612,71 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
queue_push_tail(client->services, service);
}
+static bool match_service_handle(const void *a, const void *b)
+{
+ const struct service *service = a;
+ uint16_t start_handle = PTR_TO_UINT(b);
+
+ return service->start_handle == start_handle;
+}
+
+struct update_incl_data {
+ struct service *service;
+ bool changed;
+};
+
+static void update_included_service(struct gatt_db_attribute *attrib,
+ void *user_data)
+{
+ struct update_incl_data *update_data = user_data;
+ struct btd_gatt_client *client = update_data->service->client;
+ struct service *service = update_data->service;
+ struct service *incl_service;
+ uint16_t start_handle;
+
+ gatt_db_attribute_get_incl_data(attrib, NULL, &start_handle, NULL);
+
+ incl_service = queue_find(client->services, match_service_handle,
+ UINT_TO_PTR(start_handle));
+
+ if (!incl_service)
+ return;
+
+ /* Check if service is already on list */
+ if (queue_find(service->incl_services, NULL, incl_service))
+ return;
+
+ queue_push_tail(service->incl_services, incl_service);
+ update_data->changed = true;
+}
+
+static void update_included_services(void *data, void *user_data)
+{
+ struct btd_gatt_client *client = user_data;
+ struct service *service = data;
+ struct gatt_db_attribute *attr;
+ struct update_incl_data inc_data = {
+ .changed = false,
+ .service = service,
+ };
+
+ attr = gatt_db_get_attribute(client->db, service->start_handle);
+ gatt_db_service_foreach_incl(attr, update_included_service, &inc_data);
+
+ if (inc_data.changed)
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ service->path,
+ GATT_SERVICE_IFACE,
+ "Includes");
+}
+
static void create_services(struct btd_gatt_client *client)
{
DBG("Exporting objects for GATT services: %s", client->devaddr);
gatt_db_foreach_service(client->db, NULL, export_service, client);
+
+ queue_foreach(client->services, update_included_services, client);
}
struct btd_gatt_client *btd_gatt_client_new(struct btd_device *device)
@@ -1689,14 +1794,8 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
return;
export_service(attrib, client);
-}
-
-static bool match_service_handle(const void *a, const void *b)
-{
- const struct service *service = a;
- uint16_t start_handle = PTR_TO_UINT(b);
- return service->start_handle == start_handle;
+ queue_foreach(client->services, update_included_services, client);
}
void btd_gatt_client_service_removed(struct btd_gatt_client *client,
--
2.4.3
^ permalink raw reply related
* [PATCH BlueZ v2] core/gatt-client: Add support for Includes property
From: Marcin Kraglak @ 2017-04-26 9:51 UTC (permalink / raw)
To: linux-bluetooth
Add implementation of Includes property in GATT service interface.
Include services are updated after exporting all services, when new service
has been added or service was removed.
---
src/gatt-client.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 105 insertions(+), 7 deletions(-)
diff --git a/src/gatt-client.c b/src/gatt-client.c
index 114981c..00f9d6a 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -72,6 +72,7 @@ struct service {
bt_uuid_t uuid;
char *path;
struct queue *chrcs;
+ struct queue *incl_services;
};
typedef bool (*async_dbus_op_complete_t)(void *data);
@@ -1398,10 +1399,36 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
return TRUE;
}
+static void append_incl_service_path(void *data, void *user_data)
+{
+ struct service *incl_service = data;
+ DBusMessageIter *array = user_data;
+
+ dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
+ &incl_service->path);
+}
+
+static gboolean service_get_includes(const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct service *service = data;
+ DBusMessageIter array;
+
+ dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
+
+ queue_foreach(service->incl_services, append_incl_service_path, &array);
+
+ dbus_message_iter_close_container(iter, &array);
+
+ return TRUE;
+
+}
+
static const GDBusPropertyTable service_properties[] = {
{ "UUID", "s", service_get_uuid },
{ "Device", "o", service_get_device },
{ "Primary", "b", service_get_primary },
+ { "Includes", "ao", service_get_includes },
{ }
};
@@ -1410,6 +1437,7 @@ static void service_free(void *data)
struct service *service = data;
queue_destroy(service->chrcs, NULL); /* List should be empty here */
+ queue_destroy(service->incl_services, NULL);
g_free(service->path);
free(service);
}
@@ -1423,6 +1451,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
service = new0(struct service, 1);
service->chrcs = queue_new();
+ service->incl_services = queue_new();
service->client = client;
gatt_db_attribute_get_service_data(attr, &service->start_handle,
@@ -1459,13 +1488,29 @@ static struct service *service_create(struct gatt_db_attribute *attr,
return service;
}
+static void on_service_removed(void *data, void *user_data)
+{
+ struct service *service = data;
+ struct service *removed_service = user_data;
+
+ if (queue_remove(service->incl_services, removed_service))
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ service->path,
+ GATT_SERVICE_IFACE,
+ "Includes");
+}
+
static void unregister_service(void *data)
{
struct service *service = data;
+ struct btd_gatt_client *client = service->client;
DBG("Removing GATT service: %s", service->path);
queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
+ queue_remove_all(service->incl_services, NULL, NULL, NULL);
+
+ queue_foreach(client->services, on_service_removed, service);
g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
GATT_SERVICE_IFACE);
@@ -1567,11 +1612,70 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
queue_push_tail(client->services, service);
}
+static bool match_service_handle(const void *a, const void *b)
+{
+ const struct service *service = a;
+ uint16_t start_handle = PTR_TO_UINT(b);
+
+ return service->start_handle == start_handle;
+}
+
+struct update_incl_data {
+ struct service *service;
+ bool changed;
+};
+
+static void update_included_service(struct gatt_db_attribute *attrib,
+ void *user_data)
+{
+ struct update_incl_data *update_data = user_data;
+ struct btd_gatt_client *client = update_data->service->client;
+ struct service *service = update_data->service;
+ struct service *incl_service;
+ uint16_t start_handle;
+
+ gatt_db_attribute_get_incl_data(attrib, NULL, &start_handle, NULL);
+
+ incl_service = queue_find(client->services, match_service_handle,
+ UINT_TO_PTR(start_handle));
+
+ if (!incl_service)
+ return;
+
+ /* Check if service is already on list */
+ if (queue_find(service->incl_services, NULL, incl_service))
+ return;
+
+ queue_push_tail(service->incl_services, incl_service);
+ update_data->changed = true;
+}
+static void update_included_services(void *data, void *user_data)
+{
+ struct btd_gatt_client *client = user_data;
+ struct service *service = data;
+ struct gatt_db_attribute *attr;
+ struct update_incl_data inc_data = {
+ .changed = false,
+ .service = service,
+ };
+
+ attr = gatt_db_get_attribute(client->db, service->start_handle);
+ gatt_db_service_foreach_incl(attr, update_included_service, &inc_data);
+
+ if (inc_data.changed)
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ service->path,
+ GATT_SERVICE_IFACE,
+ "Includes");
+}
+
static void create_services(struct btd_gatt_client *client)
{
DBG("Exporting objects for GATT services: %s", client->devaddr);
gatt_db_foreach_service(client->db, NULL, export_service, client);
+
+ queue_foreach(client->services, update_included_services, client);
}
struct btd_gatt_client *btd_gatt_client_new(struct btd_device *device)
@@ -1689,14 +1793,8 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
return;
export_service(attrib, client);
-}
-static bool match_service_handle(const void *a, const void *b)
-{
- const struct service *service = a;
- uint16_t start_handle = PTR_TO_UINT(b);
-
- return service->start_handle == start_handle;
+ queue_foreach(client->services, update_included_services, client);
}
void btd_gatt_client_service_removed(struct btd_gatt_client *client,
--
2.4.3
^ permalink raw reply related
* Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication
From: Marcin Kraglak @ 2017-04-26 9:16 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKRdO61BS698xQwivFQtU-Q5tt5hByG2i7DhjxBpoHrRg@mail.gmail.com>
Hi Luiz,
On 26 April 2017 at 11:07, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Wed, Apr 26, 2017 at 8:16 AM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Hi Luiz,
>>
>> On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> Hi Marcin,
>>>
>>> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
>>> <marcin.kraglak@tieto.com> wrote:
>>>> Clear services in range of Service Changed indication.
>>>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>>>> "The client, upon receiving a Handle Value Indication containing the range of
>>>> affected Attribute Handles, shall consider the attribute cache invalid over
>>>> the affected Attribute Handle range".
>>>> ---
>>>> src/shared/gatt-client.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>>> index 0134721..7d23702 100644
>>>> --- a/src/shared/gatt-client.c
>>>> +++ b/src/shared/gatt-client.c
>>>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>>>> util_debug(client->debug_callback, client->debug_data,
>>>> "Failed to discover services within changed range - "
>>>> "error: 0x%02x", att_ecode);
>>>> -
>>>> - gatt_db_clear_range(client->db, start_handle, end_handle);
>>>> }
>>>>
>>>> /* Notify the upper layer of changed services */
>>>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>>>> {
>>>> struct bt_gatt_client *client = op->client;
>>>>
>>>> - gatt_db_clear_range(client->db, op->start, op->end);
>>>> + util_debug(client->debug_callback, client->debug_data,
>>>> + "Failed to process Service Changed");
>>>> }
>>>>
>>>> static void process_service_changed(struct bt_gatt_client *client,
>>>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>>>> {
>>>> struct discovery_op *op;
>>>>
>>>> + gatt_db_clear_range(client->db, start_handle, end_handle);
>>>> +
>>>> op = discovery_op_create(client, start_handle, end_handle,
>>>> service_changed_complete,
>>>> service_changed_failure);
>>>> --
>>>> 2.4.3
>>>
>>> The code used to do exactly that, but it turn out it very inefficient
>>> with devices that just loose the entire database when rebooted so we
>>> started doing cache validation so we don't remove the services upfront
>>> but check if the service range has changed so we prevent rediscovering
>>> everything.
>>>
>> I see. Currently cached services, which were not found in new
>> discovery after reconnect/service change (they were simply removed),
>> are not unregistered. I guess it is not what we expect? If not I can
>> fix it in another patch.
>
> There is are calls to gatt_db_clear_range, so you are saying these do
> not affect the D-Bus objects? If that is the case then yes we need to
> fix it, but is it so that the services really changed because if they
> don't there is no reason to remove the objects.
I mean situation when you had A, B service and remove B (no new
service will be put in range of B) - then B will be still be exposed.
If there will be new one in range of B, B will be removed.
>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>>
>> --
>> BR
>> Marcin Kraglak
>
>
>
> --
> Luiz Augusto von Dentz
--
BR
Marcin Kraglak
^ permalink raw reply
* Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication
From: Luiz Augusto von Dentz @ 2017-04-26 9:07 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABD6X-+_06m-B23zYOZEC5LHL07Sd1PnJ0oLGNUfRvy3Y4COqA@mail.gmail.com>
Hi Marcin,
On Wed, Apr 26, 2017 at 8:16 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Luiz,
>
> On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> Hi Marcin,
>>
>> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
>> <marcin.kraglak@tieto.com> wrote:
>>> Clear services in range of Service Changed indication.
>>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>>> "The client, upon receiving a Handle Value Indication containing the range of
>>> affected Attribute Handles, shall consider the attribute cache invalid over
>>> the affected Attribute Handle range".
>>> ---
>>> src/shared/gatt-client.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>> index 0134721..7d23702 100644
>>> --- a/src/shared/gatt-client.c
>>> +++ b/src/shared/gatt-client.c
>>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>>> util_debug(client->debug_callback, client->debug_data,
>>> "Failed to discover services within changed range - "
>>> "error: 0x%02x", att_ecode);
>>> -
>>> - gatt_db_clear_range(client->db, start_handle, end_handle);
>>> }
>>>
>>> /* Notify the upper layer of changed services */
>>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>>> {
>>> struct bt_gatt_client *client = op->client;
>>>
>>> - gatt_db_clear_range(client->db, op->start, op->end);
>>> + util_debug(client->debug_callback, client->debug_data,
>>> + "Failed to process Service Changed");
>>> }
>>>
>>> static void process_service_changed(struct bt_gatt_client *client,
>>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>>> {
>>> struct discovery_op *op;
>>>
>>> + gatt_db_clear_range(client->db, start_handle, end_handle);
>>> +
>>> op = discovery_op_create(client, start_handle, end_handle,
>>> service_changed_complete,
>>> service_changed_failure);
>>> --
>>> 2.4.3
>>
>> The code used to do exactly that, but it turn out it very inefficient
>> with devices that just loose the entire database when rebooted so we
>> started doing cache validation so we don't remove the services upfront
>> but check if the service range has changed so we prevent rediscovering
>> everything.
>>
> I see. Currently cached services, which were not found in new
> discovery after reconnect/service change (they were simply removed),
> are not unregistered. I guess it is not what we expect? If not I can
> fix it in another patch.
There is are calls to gatt_db_clear_range, so you are saying these do
not affect the D-Bus objects? If that is the case then yes we need to
fix it, but is it so that the services really changed because if they
don't there is no reason to remove the objects.
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> BR
> Marcin Kraglak
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication
From: Marcin Kraglak @ 2017-04-26 5:16 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJ9E8Dt=OiAisRf1fZ4CewLfY=Axi82wpxSpCogpcgd5Q@mail.gmail.com>
Hi Luiz,
On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Clear services in range of Service Changed indication.
>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>> "The client, upon receiving a Handle Value Indication containing the range of
>> affected Attribute Handles, shall consider the attribute cache invalid over
>> the affected Attribute Handle range".
>> ---
>> src/shared/gatt-client.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>> index 0134721..7d23702 100644
>> --- a/src/shared/gatt-client.c
>> +++ b/src/shared/gatt-client.c
>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>> util_debug(client->debug_callback, client->debug_data,
>> "Failed to discover services within changed range - "
>> "error: 0x%02x", att_ecode);
>> -
>> - gatt_db_clear_range(client->db, start_handle, end_handle);
>> }
>>
>> /* Notify the upper layer of changed services */
>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>> {
>> struct bt_gatt_client *client = op->client;
>>
>> - gatt_db_clear_range(client->db, op->start, op->end);
>> + util_debug(client->debug_callback, client->debug_data,
>> + "Failed to process Service Changed");
>> }
>>
>> static void process_service_changed(struct bt_gatt_client *client,
>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>> {
>> struct discovery_op *op;
>>
>> + gatt_db_clear_range(client->db, start_handle, end_handle);
>> +
>> op = discovery_op_create(client, start_handle, end_handle,
>> service_changed_complete,
>> service_changed_failure);
>> --
>> 2.4.3
>
> The code used to do exactly that, but it turn out it very inefficient
> with devices that just loose the entire database when rebooted so we
> started doing cache validation so we don't remove the services upfront
> but check if the service range has changed so we prevent rediscovering
> everything.
>
I see. Currently cached services, which were not found in new
discovery after reconnect/service change (they were simply removed),
are not unregistered. I guess it is not what we expect? If not I can
fix it in another patch.
>
> --
> Luiz Augusto von Dentz
--
BR
Marcin Kraglak
^ permalink raw reply
* Re: Serial Port connection with DBus API
From: Barry Byford @ 2017-04-25 21:48 UTC (permalink / raw)
To: Bluez mailing list
In-Reply-To: <CAAu3APZxRDoZhCU7QtWVNd05HbKzadoX=886r_REiebvmMj6vQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]
I don't feel I'm making progress on this so I've attached the output
from btmon in the hope that someone has the time to cast an eye over
it.
The values that are received into NewConnection are:
object device = /org/bluez/hci0/dev_64_BC_0C_F6_22_F8
fd = 0
dict fd_properties = {}
if I do a os.isatty(fd) then I get returned True
if I do a os.ttyname(fd) then I get returned /dev/pts/0
That device is the terminal I'm running the Python code in and not the
serial port.
I feel like I'm missing something obvious but I can't spot it so any
suggestions would be welcome.
Thanks,
Barry
On 22 April 2017 at 21:46, Barry Byford <31baz66@gmail.com> wrote:
> On 22 April 2017 at 06:26, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Hi Barry,
>>
>> On Fri, Apr 21, 2017, Barry Byford wrote:
>>> >> > 'Channel': GLib.Variant('i', 1),
>> ...
>>> However I'm now seeing this error appear in btmon when I try to bind
>>> to the RFCOMM socket:
>>>
>>> = bluetoothd: RFCOMM server failed for SerialPort: rfcomm_bind:
>>> Address already in use (98)
>>
>> I think that means that you already have some other service listening on
>> RFCOMM channel 1. Try using some other channel, or identify and stop the
>> other service that's using channel 1.
>
> Turns out it was me that was creating the other service. :-(
>
> Depending which order I bind to the socket or RegisterProfile, depends
> if I get the error in Bluetoothd or sockets.
>
> I can register the Serial Port profile successfully which allows me to
> connect from the (Bluedot) Android app and I can see information being
> sent to my Linux board in btmon. However I don't seem to be able to
> get at that information in my script.
>
> Looking at this a bit more, maybe the socket connection should be
> created from the 'fd' parameter passed to NewConnection. However when
> I try to get the socket with this:
>
> def NewConnection(self, path, fd, properties):
> self.server_sock = socket.fromfd(fd, socket.AF_BLUETOOTH, socket.SOCK_STREAM,
> socket.BTPROTO_RFCOMM)
> data = self.server_sock.recv(1024)
>
> It gives the following:
>
> Exception while handling org.bluez.Profile1.NewConnection()
> Traceback (most recent call last):
> File "/usr/local/lib/python3.5/dist-packages/pydbus/registration.py",
> line 81, in call_method
> result = method(*parameters, **kwargs)
> File "BTclassic.py", line 67, in NewConnection
> data = self.server_sock.recv(1024)
> OSError: [Errno 88] Socket operation on non-socket
>
> I had been following test/test-hfp as the basic structure but that
> doesn't seem to be working for me.
>
> Not sure where to go next with this
[-- Attachment #2: btmon.log --]
[-- Type: application/octet-stream, Size: 11238 bytes --]
^ permalink raw reply
* Re: [PATCH BlueZ 2/2] core/gatt-client: Add support for Includes property
From: Marcin Kraglak @ 2017-04-25 19:26 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKT+et1yaaxBZjHXumb2zDc3VUL7pVQ6wVGXZRiHOzFTw@mail.gmail.com>
Hi Luiz,
On 25 April 2017 at 20:51, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Add implementation of Includes property in GATT service interface.
>> ---
>> src/gatt-client.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 136 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 114981c..15219e2 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -72,6 +72,15 @@ struct service {
>> bt_uuid_t uuid;
>> char *path;
>> struct queue *chrcs;
>> + struct queue *incl_services;
>> +};
>> +
>> +struct incl_service {
>> + uint16_t handle;
>> + uint16_t start_handle;
>> + uint16_t end_handle;
>> + struct service *service;
>> + struct service *incl_service;
>
> This looks wrong, either we have the struct service representing the
> including or we create a new as above but not both.
>
>> };
>>
>> typedef bool (*async_dbus_op_complete_t)(void *data);
>> @@ -1398,10 +1407,39 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
>> return TRUE;
>> }
>>
>> +static void append_incl_service_path(void *data, void *user_data)
>> +{
>> + struct incl_service *incl_service = data;
>> + DBusMessageIter *array = user_data;
>> +
>> + if (!incl_service->incl_service)
>> + return;
>> +
>> + dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
>> + &incl_service->incl_service->path);
>> +}
>> +
>> +static gboolean service_get_includes(const GDBusPropertyTable *property,
>> + DBusMessageIter *iter, void *data)
>> +{
>> + struct service *service = data;
>> + DBusMessageIter array;
>> +
>> + dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
>> +
>> + queue_foreach(service->incl_services, append_incl_service_path, &array);
>> +
>> + dbus_message_iter_close_container(iter, &array);
>> +
>> + return TRUE;
>> +
>> +}
>> +
>> static const GDBusPropertyTable service_properties[] = {
>> { "UUID", "s", service_get_uuid },
>> { "Device", "o", service_get_device },
>> { "Primary", "b", service_get_primary },
>> + { "Includes", "ao", service_get_includes },
>> { }
>> };
>>
>> @@ -1410,10 +1448,40 @@ static void service_free(void *data)
>> struct service *service = data;
>>
>> queue_destroy(service->chrcs, NULL); /* List should be empty here */
>> + queue_destroy(service->incl_services, NULL);
>> g_free(service->path);
>> free(service);
>> }
>>
>> +static bool match_service_handle(const void *a, const void *b)
>> +{
>> + const struct service *service = a;
>> + uint16_t start_handle = PTR_TO_UINT(b);
>> +
>> + return service->start_handle == start_handle;
>> +}
>> +
>> +static void add_included_service(struct gatt_db_attribute *attrib,
>> + void *user_data)
>> +{
>> + struct service *service = user_data;
>> + struct incl_service *incl_service;
>> + struct btd_gatt_client *client = service->client;
>> +
>> + incl_service = new0(struct incl_service, 1);
>> + incl_service->service = service;
>> +
>> + gatt_db_attribute_get_incl_data(attrib, &incl_service->handle,
>> + &incl_service->start_handle,
>> + &incl_service->end_handle);
>
> This only seems to be useful for matching the included services,
> actually once we find what is the service why don't we keep the list
> of only the struct service as the only thing we use is the path.
Because included service may not be in the service list yet. We can
omit handle, end handle and not store it, but start handle is needed
for matching when included service will be registered.
>
>> + incl_service->incl_service = queue_find(client->services,
>> + match_service_handle,
>> + UINT_TO_PTR(incl_service->start_handle));
>> +
>> + queue_push_tail(service->incl_services, incl_service);
>> +}
>> +
>> static struct service *service_create(struct gatt_db_attribute *attr,
>> struct btd_gatt_client *client)
>> {
>> @@ -1423,6 +1491,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>>
>> service = new0(struct service, 1);
>> service->chrcs = queue_new();
>> + service->incl_services = queue_new();
>> service->client = client;
>>
>> gatt_db_attribute_get_service_data(attr, &service->start_handle,
>> @@ -1434,6 +1503,8 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>> service->path = g_strdup_printf("%s/service%04x", device_path,
>> service->start_handle);
>>
>> + gatt_db_service_foreach_incl(attr, add_included_service, service);
>> +
>> if (!g_dbus_register_interface(btd_get_dbus_connection(), service->path,
>> GATT_SERVICE_IFACE,
>> NULL, NULL,
>> @@ -1459,6 +1530,63 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>> return service;
>> }
>>
>> +static void incl_service_on_unregister(void *data, void *user_data)
>> +{
>> + struct incl_service *incl_service = (struct incl_service *) data;
>> + struct service *service = (struct service *) user_data;
>> +
>> + if (incl_service->incl_service == service) {
>> + incl_service->incl_service = NULL;
>> +
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> + incl_service->service->path,
>> + GATT_SERVICE_IFACE,
>> + "Includes");
>> + }
>> +}
>> +
>> +static void incl_service_on_register(void *data, void *user_data)
>> +{
>> + struct incl_service *incl_service = (struct incl_service *) data;
>> + struct service *service = (struct service *) user_data;
>> +
>> + if (!incl_service->incl_service &&
>> + incl_service->start_handle == service->start_handle) {
>> + incl_service->incl_service = service;
>> +
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> + incl_service->service->path,
>> + GATT_SERVICE_IFACE,
>> + "Includes");
>> + }
>> +}
>> +
>> +struct foreach_incl_service_data {
>> + queue_foreach_func_t func;
>> + void *user_data;
>> +};
>> +
>> +static void incl_service_cb(void *data, void *user_data)
>> +{
>> + struct service *service = (struct service *) data;
>> + struct foreach_incl_service_data *incl_data =
>> + (struct foreach_incl_service_data *) user_data;
>> +
>> + queue_foreach(service->incl_services, incl_data->func,
>> + incl_data->user_data);
>> +}
>> +
>> +static void client_foreach_incl_service(struct btd_gatt_client *client,
>> + queue_foreach_func_t func, void *user_data)
>> +{
>> + struct foreach_incl_service_data incl_data = {
>> + .user_data = user_data,
>> + .func = func,
>> + };
>> +
>> + queue_foreach(client->services, incl_service_cb, &incl_data);
>> +}
>> +
>> static void unregister_service(void *data)
>> {
>> struct service *service = data;
>> @@ -1466,6 +1594,11 @@ static void unregister_service(void *data)
>> DBG("Removing GATT service: %s", service->path);
>>
>> queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
>> + queue_remove_all(service->incl_services, NULL, NULL, free);
>> +
>> + /* Make sure that no one included service points to this one */
>> + client_foreach_incl_service(service->client,
>> + incl_service_on_unregister, service);
>>
>> g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
>> GATT_SERVICE_IFACE);
>> @@ -1564,6 +1697,9 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
>> return;
>> }
>>
>> + /* Check if any included service points to this one */
>> + client_foreach_incl_service(client, incl_service_on_register, service);
>> +
>> queue_push_tail(client->services, service);
>> }
>>
>> @@ -1691,14 +1827,6 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
>> export_service(attrib, client);
>> }
>>
>> -static bool match_service_handle(const void *a, const void *b)
>> -{
>> - const struct service *service = a;
>> - uint16_t start_handle = PTR_TO_UINT(b);
>> -
>> - return service->start_handle == start_handle;
>> -}
>> -
>> void btd_gatt_client_service_removed(struct btd_gatt_client *client,
>> struct gatt_db_attribute *attrib)
>> {
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
--
BR
Marcin Kraglak
^ permalink raw reply
* Re: [PATCH BlueZ 2/2] core/gatt-client: Add support for Includes property
From: Luiz Augusto von Dentz @ 2017-04-25 18:51 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1493121371-29377-2-git-send-email-marcin.kraglak@tieto.com>
Hi Marcin,
On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Add implementation of Includes property in GATT service interface.
> ---
> src/gatt-client.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 136 insertions(+), 8 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 114981c..15219e2 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -72,6 +72,15 @@ struct service {
> bt_uuid_t uuid;
> char *path;
> struct queue *chrcs;
> + struct queue *incl_services;
> +};
> +
> +struct incl_service {
> + uint16_t handle;
> + uint16_t start_handle;
> + uint16_t end_handle;
> + struct service *service;
> + struct service *incl_service;
This looks wrong, either we have the struct service representing the
including or we create a new as above but not both.
> };
>
> typedef bool (*async_dbus_op_complete_t)(void *data);
> @@ -1398,10 +1407,39 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
> return TRUE;
> }
>
> +static void append_incl_service_path(void *data, void *user_data)
> +{
> + struct incl_service *incl_service = data;
> + DBusMessageIter *array = user_data;
> +
> + if (!incl_service->incl_service)
> + return;
> +
> + dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
> + &incl_service->incl_service->path);
> +}
> +
> +static gboolean service_get_includes(const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct service *service = data;
> + DBusMessageIter array;
> +
> + dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
> +
> + queue_foreach(service->incl_services, append_incl_service_path, &array);
> +
> + dbus_message_iter_close_container(iter, &array);
> +
> + return TRUE;
> +
> +}
> +
> static const GDBusPropertyTable service_properties[] = {
> { "UUID", "s", service_get_uuid },
> { "Device", "o", service_get_device },
> { "Primary", "b", service_get_primary },
> + { "Includes", "ao", service_get_includes },
> { }
> };
>
> @@ -1410,10 +1448,40 @@ static void service_free(void *data)
> struct service *service = data;
>
> queue_destroy(service->chrcs, NULL); /* List should be empty here */
> + queue_destroy(service->incl_services, NULL);
> g_free(service->path);
> free(service);
> }
>
> +static bool match_service_handle(const void *a, const void *b)
> +{
> + const struct service *service = a;
> + uint16_t start_handle = PTR_TO_UINT(b);
> +
> + return service->start_handle == start_handle;
> +}
> +
> +static void add_included_service(struct gatt_db_attribute *attrib,
> + void *user_data)
> +{
> + struct service *service = user_data;
> + struct incl_service *incl_service;
> + struct btd_gatt_client *client = service->client;
> +
> + incl_service = new0(struct incl_service, 1);
> + incl_service->service = service;
> +
> + gatt_db_attribute_get_incl_data(attrib, &incl_service->handle,
> + &incl_service->start_handle,
> + &incl_service->end_handle);
This only seems to be useful for matching the included services,
actually once we find what is the service why don't we keep the list
of only the struct service as the only thing we use is the path.
> + incl_service->incl_service = queue_find(client->services,
> + match_service_handle,
> + UINT_TO_PTR(incl_service->start_handle));
> +
> + queue_push_tail(service->incl_services, incl_service);
> +}
> +
> static struct service *service_create(struct gatt_db_attribute *attr,
> struct btd_gatt_client *client)
> {
> @@ -1423,6 +1491,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>
> service = new0(struct service, 1);
> service->chrcs = queue_new();
> + service->incl_services = queue_new();
> service->client = client;
>
> gatt_db_attribute_get_service_data(attr, &service->start_handle,
> @@ -1434,6 +1503,8 @@ static struct service *service_create(struct gatt_db_attribute *attr,
> service->path = g_strdup_printf("%s/service%04x", device_path,
> service->start_handle);
>
> + gatt_db_service_foreach_incl(attr, add_included_service, service);
> +
> if (!g_dbus_register_interface(btd_get_dbus_connection(), service->path,
> GATT_SERVICE_IFACE,
> NULL, NULL,
> @@ -1459,6 +1530,63 @@ static struct service *service_create(struct gatt_db_attribute *attr,
> return service;
> }
>
> +static void incl_service_on_unregister(void *data, void *user_data)
> +{
> + struct incl_service *incl_service = (struct incl_service *) data;
> + struct service *service = (struct service *) user_data;
> +
> + if (incl_service->incl_service == service) {
> + incl_service->incl_service = NULL;
> +
> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
> + incl_service->service->path,
> + GATT_SERVICE_IFACE,
> + "Includes");
> + }
> +}
> +
> +static void incl_service_on_register(void *data, void *user_data)
> +{
> + struct incl_service *incl_service = (struct incl_service *) data;
> + struct service *service = (struct service *) user_data;
> +
> + if (!incl_service->incl_service &&
> + incl_service->start_handle == service->start_handle) {
> + incl_service->incl_service = service;
> +
> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
> + incl_service->service->path,
> + GATT_SERVICE_IFACE,
> + "Includes");
> + }
> +}
> +
> +struct foreach_incl_service_data {
> + queue_foreach_func_t func;
> + void *user_data;
> +};
> +
> +static void incl_service_cb(void *data, void *user_data)
> +{
> + struct service *service = (struct service *) data;
> + struct foreach_incl_service_data *incl_data =
> + (struct foreach_incl_service_data *) user_data;
> +
> + queue_foreach(service->incl_services, incl_data->func,
> + incl_data->user_data);
> +}
> +
> +static void client_foreach_incl_service(struct btd_gatt_client *client,
> + queue_foreach_func_t func, void *user_data)
> +{
> + struct foreach_incl_service_data incl_data = {
> + .user_data = user_data,
> + .func = func,
> + };
> +
> + queue_foreach(client->services, incl_service_cb, &incl_data);
> +}
> +
> static void unregister_service(void *data)
> {
> struct service *service = data;
> @@ -1466,6 +1594,11 @@ static void unregister_service(void *data)
> DBG("Removing GATT service: %s", service->path);
>
> queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
> + queue_remove_all(service->incl_services, NULL, NULL, free);
> +
> + /* Make sure that no one included service points to this one */
> + client_foreach_incl_service(service->client,
> + incl_service_on_unregister, service);
>
> g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
> GATT_SERVICE_IFACE);
> @@ -1564,6 +1697,9 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
> return;
> }
>
> + /* Check if any included service points to this one */
> + client_foreach_incl_service(client, incl_service_on_register, service);
> +
> queue_push_tail(client->services, service);
> }
>
> @@ -1691,14 +1827,6 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
> export_service(attrib, client);
> }
>
> -static bool match_service_handle(const void *a, const void *b)
> -{
> - const struct service *service = a;
> - uint16_t start_handle = PTR_TO_UINT(b);
> -
> - return service->start_handle == start_handle;
> -}
> -
> void btd_gatt_client_service_removed(struct btd_gatt_client *client,
> struct gatt_db_attribute *attrib)
> {
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication
From: Luiz Augusto von Dentz @ 2017-04-25 18:21 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1493121371-29377-1-git-send-email-marcin.kraglak@tieto.com>
Hi Marcin,
On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Clear services in range of Service Changed indication.
> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
> "The client, upon receiving a Handle Value Indication containing the range of
> affected Attribute Handles, shall consider the attribute cache invalid over
> the affected Attribute Handle range".
> ---
> src/shared/gatt-client.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 0134721..7d23702 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
> util_debug(client->debug_callback, client->debug_data,
> "Failed to discover services within changed range - "
> "error: 0x%02x", att_ecode);
> -
> - gatt_db_clear_range(client->db, start_handle, end_handle);
> }
>
> /* Notify the upper layer of changed services */
> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
> {
> struct bt_gatt_client *client = op->client;
>
> - gatt_db_clear_range(client->db, op->start, op->end);
> + util_debug(client->debug_callback, client->debug_data,
> + "Failed to process Service Changed");
> }
>
> static void process_service_changed(struct bt_gatt_client *client,
> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
> {
> struct discovery_op *op;
>
> + gatt_db_clear_range(client->db, start_handle, end_handle);
> +
> op = discovery_op_create(client, start_handle, end_handle,
> service_changed_complete,
> service_changed_failure);
> --
> 2.4.3
The code used to do exactly that, but it turn out it very inefficient
with devices that just loose the entire database when rebooted so we
started doing cache validation so we don't remove the services upfront
but check if the service range has changed so we prevent rediscovering
everything.
--
Luiz Augusto von Dentz
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox