* Re: [PATCH 0/2] Bluetooth: fix hci-uart crashes
From: Marcel Holtmann @ 2017-03-29 18:57 UTC (permalink / raw)
To: Johan Hovold
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-kernel
In-Reply-To: <20170329161528.17843-1-johan@kernel.org>
Hi Johan,
> Not every tty has a class device and this could be used to trigger
> NULL-pointer dereferences in two hci-uart drivers that lacked the
> required sanity checks.
>
> Johan
>
>
> Johan Hovold (2):
> Bluetooth: hci_bcm: add missing tty-device sanity check
> Bluetooth: hci_intel: add missing tty-device sanity check
>
> drivers/bluetooth/hci_bcm.c | 5 ++++-
> drivers/bluetooth/hci_intel.c | 13 ++++++++++++-
> 2 files changed, 16 insertions(+), 2 deletions(-)
bot patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH 2/2] Bluetooth: hci_intel: add missing tty-device sanity check
From: Johan Hovold @ 2017-03-29 16:15 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth, linux-kernel, Johan Hovold, stable, Loic Poulain
In-Reply-To: <20170329161528.17843-1-johan@kernel.org>
Make sure to check the tty-device pointer before looking up the sibling
platform device to avoid dereferencing a NULL-pointer when the tty is
one end of a Unix98 pty.
Fixes: 74cdad37cd24 ("Bluetooth: hci_intel: Add runtime PM support")
Fixes: 1ab1f239bf17 ("Bluetooth: hci_intel: Add support for platform driver")
Cc: stable <stable@vger.kernel.org> # 4.3
Cc: Loic Poulain <loic.poulain@intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/bluetooth/hci_intel.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 9e271286c5e5..73306384af6c 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -307,6 +307,9 @@ static int intel_set_power(struct hci_uart *hu, bool powered)
struct list_head *p;
int err = -ENODEV;
+ if (!hu->tty->dev)
+ return err;
+
mutex_lock(&intel_device_list_lock);
list_for_each(p, &intel_device_list) {
@@ -379,6 +382,9 @@ static void intel_busy_work(struct work_struct *work)
struct intel_data *intel = container_of(work, struct intel_data,
busy_work);
+ if (!intel->hu->tty->dev)
+ return;
+
/* Link is busy, delay the suspend */
mutex_lock(&intel_device_list_lock);
list_for_each(p, &intel_device_list) {
@@ -889,6 +895,8 @@ static int intel_setup(struct hci_uart *hu)
list_for_each(p, &intel_device_list) {
struct intel_device *dev = list_entry(p, struct intel_device,
list);
+ if (!hu->tty->dev)
+ break;
if (hu->tty->dev->parent == dev->pdev->dev.parent) {
if (device_may_wakeup(&dev->pdev->dev)) {
set_bit(STATE_LPM_ENABLED, &intel->flags);
@@ -1056,6 +1064,9 @@ static int intel_enqueue(struct hci_uart *hu, struct sk_buff *skb)
BT_DBG("hu %p skb %p", hu, skb);
+ if (!hu->tty->dev)
+ goto out_enqueue;
+
/* Be sure our controller is resumed and potential LPM transaction
* completed before enqueuing any packet.
*/
@@ -1072,7 +1083,7 @@ static int intel_enqueue(struct hci_uart *hu, struct sk_buff *skb)
}
}
mutex_unlock(&intel_device_list_lock);
-
+out_enqueue:
skb_queue_tail(&intel->txq, skb);
return 0;
--
2.12.2
^ permalink raw reply related
* [PATCH 1/2] Bluetooth: hci_bcm: add missing tty-device sanity check
From: Johan Hovold @ 2017-03-29 16:15 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth, linux-kernel, Johan Hovold, stable,
Frederic Danis
In-Reply-To: <20170329161528.17843-1-johan@kernel.org>
Make sure to check the tty-device pointer before looking up the sibling
platform device to avoid dereferencing a NULL-pointer when the tty is
one end of a Unix98 pty.
Fixes: 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
Cc: stable <stable@vger.kernel.org> # 4.3
Cc: Frederic Danis <frederic.danis@linux.intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/bluetooth/hci_bcm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 5262a2077d7a..11f30e5cec2c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -287,6 +287,9 @@ static int bcm_open(struct hci_uart *hu)
hu->priv = bcm;
+ if (!hu->tty->dev)
+ goto out;
+
mutex_lock(&bcm_device_lock);
list_for_each(p, &bcm_device_list) {
struct bcm_device *dev = list_entry(p, struct bcm_device, list);
@@ -307,7 +310,7 @@ static int bcm_open(struct hci_uart *hu)
}
mutex_unlock(&bcm_device_lock);
-
+out:
return 0;
}
--
2.12.2
^ permalink raw reply related
* [PATCH 0/2] Bluetooth: fix hci-uart crashes
From: Johan Hovold @ 2017-03-29 16:15 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth, linux-kernel, Johan Hovold
Not every tty has a class device and this could be used to trigger
NULL-pointer dereferences in two hci-uart drivers that lacked the
required sanity checks.
Johan
Johan Hovold (2):
Bluetooth: hci_bcm: add missing tty-device sanity check
Bluetooth: hci_intel: add missing tty-device sanity check
drivers/bluetooth/hci_bcm.c | 5 ++++-
drivers/bluetooth/hci_intel.c | 13 ++++++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
--
2.12.2
^ permalink raw reply
* Re: [PATCH] Bluetooth: btmrvl: cleanup code in return from btmrvl_sdio_suspend()
From: Marcel Holtmann @ 2017-03-29 6:58 UTC (permalink / raw)
To: prasanna karthik
Cc: Gustavo F. Padovan, Johan Hedberg,
linux-bluetooth@vger.kernel.org, Prasanna Karthik
In-Reply-To: <SG2PR0302MB257551E9937B577EF80ACA15CA320@SG2PR0302MB2575.apcprd03.prod.outlook.com>
Hi Prasanna,
> Else is not generally useful after a break or return
>
> Signed-off-by: Prasanna Karthik <pkarthik@outlook.com>
> ---
> drivers/bluetooth/btmrvl_sdio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH] bluetooth: 6lowpan: fix delay work init in add_peer_chan()
From: Michael Scott @ 2017-03-29 6:10 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: David S . Miller, Jukka Rissanen, linux-bluetooth, linux-wpan,
netdev, linux-kernel, Michael Scott
When adding 6lowpan devices very rapidly we sometimes see a crash:
[23122.306615] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.9.0-43-arm64 #1 Debian 4.9.9.linaro.43-1
[23122.315400] Hardware name: HiKey Development Board (DT)
[23122.320623] task: ffff800075443080 task.stack: ffff800075484000
[23122.326551] PC is at expire_timers+0x70/0x150
[23122.330907] LR is at run_timer_softirq+0xa0/0x1a0
[23122.335616] pc : [<ffff000008142dd8>] lr : [<ffff000008142f58>] pstate: 600001c5
This was due to add_peer_chan() unconditionally initializing the
lowpan_btle_dev->notify_peers delayed work structure, even if the
lowpan_btle_dev passed into add_peer_chan() had previously been
initialized.
Normally, this would go unnoticed as the delayed work timer is set for
100 msec, however when calling add_peer_chan() faster than 100 msec it
clears out a previously queued delay work causing the crash above.
To fix this, let add_peer_chan() know when a new lowpan_btle_dev is passed
in so that it only performs the delay work initialization when needed.
Signed-off-by: Michael Scott <michael.scott@linaro.org>
---
net/bluetooth/6lowpan.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index e27be3ca0a0c..c282482edc2c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -754,7 +754,8 @@ static void set_ip_addr_bits(u8 addr_type, u8 *addr)
}
static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
- struct lowpan_btle_dev *dev)
+ struct lowpan_btle_dev *dev,
+ bool new_netdev)
{
struct lowpan_peer *peer;
@@ -785,7 +786,8 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
spin_unlock(&devices_lock);
/* Notifying peers about us needs to be done without locks held */
- INIT_DELAYED_WORK(&dev->notify_peers, do_notify_peers);
+ if (new_netdev)
+ INIT_DELAYED_WORK(&dev->notify_peers, do_notify_peers);
schedule_delayed_work(&dev->notify_peers, msecs_to_jiffies(100));
return peer->chan;
@@ -842,6 +844,7 @@ static int setup_netdev(struct l2cap_chan *chan, struct lowpan_btle_dev **dev)
static inline void chan_ready_cb(struct l2cap_chan *chan)
{
struct lowpan_btle_dev *dev;
+ bool new_netdev = false;
dev = lookup_dev(chan->conn);
@@ -852,12 +855,13 @@ static inline void chan_ready_cb(struct l2cap_chan *chan)
l2cap_chan_del(chan, -ENOENT);
return;
}
+ new_netdev = true;
}
if (!try_module_get(THIS_MODULE))
return;
- add_peer_chan(chan, dev);
+ add_peer_chan(chan, dev, new_netdev);
ifup(dev->netdev);
}
--
2.11.0
^ permalink raw reply related
* [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume
From: Michael Scott @ 2017-03-29 6:10 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: David S . Miller, Jukka Rissanen, linux-bluetooth, linux-wpan,
netdev, linux-kernel, Michael Scott
A status field in the skb_cb struct was storing a channel status
based on channel suspend/resume events. This stored status was
then used to return EAGAIN if there were packet sending issues
in snd_pkt().
The issue is that the skb has been freed by the time the callback
to 6lowpan's suspend/resume was called. So, this generates a
"use after free" issue that was noticed while running kernel tests
with KASAN debug enabled.
Let's eliminate the status field entirely as we can use the channel
tx_credits to indicate whether we should return EAGAIN when handling
packets.
Signed-off-by: Michael Scott <michael.scott@linaro.org>
---
net/bluetooth/6lowpan.c | 21 +++------------------
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index d491529332f4..e27be3ca0a0c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -38,7 +38,6 @@ struct skb_cb {
struct in6_addr addr;
struct in6_addr gw;
struct l2cap_chan *chan;
- int status;
};
#define lowpan_cb(skb) ((struct skb_cb *)((skb)->cb))
@@ -528,7 +527,7 @@ static int send_pkt(struct l2cap_chan *chan, struct sk_buff *skb,
}
if (!err)
- err = lowpan_cb(skb)->status;
+ err = (!chan->tx_credits ? -EAGAIN : 0);
if (err < 0) {
if (err == -EAGAIN)
@@ -964,26 +963,12 @@ static struct sk_buff *chan_alloc_skb_cb(struct l2cap_chan *chan,
static void chan_suspend_cb(struct l2cap_chan *chan)
{
- struct sk_buff *skb = chan->data;
-
- BT_DBG("chan %p conn %p skb %p", chan, chan->conn, skb);
-
- if (!skb)
- return;
-
- lowpan_cb(skb)->status = -EAGAIN;
+ BT_DBG("chan %p suspend", chan);
}
static void chan_resume_cb(struct l2cap_chan *chan)
{
- struct sk_buff *skb = chan->data;
-
- BT_DBG("chan %p conn %p skb %p", chan, chan->conn, skb);
-
- if (!skb)
- return;
-
- lowpan_cb(skb)->status = 0;
+ BT_DBG("chan %p resume", chan);
}
static long chan_get_sndtimeo_cb(struct l2cap_chan *chan)
--
2.11.0
^ permalink raw reply related
* [PATCH] lib: Add company names and identifiers upto 1190
From: Seulki Shin @ 2017-03-29 2:25 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Seulki Shin
This adds company names with reference to Bluetooth SIG's assigned
numbers
---
lib/bluetooth.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 306 insertions(+)
diff --git a/lib/bluetooth.c b/lib/bluetooth.c
index 3276b68a5..f6ebe84d6 100644
--- a/lib/bluetooth.c
+++ b/lib/bluetooth.c
@@ -2337,6 +2337,312 @@ const char *bt_compidtostr(int compid)
return "Senix Corporation";
case 1037:
return "NorthStar Battery Company, LLC";
+ case 1038:
+ return "SKF (U.K.) Limited";
+ case 1039:
+ return "CO-AX Technology, Inc.";
+ case 1040:
+ return "Fender Musical Instruments";
+ case 1041:
+ return "Luidia Inc";
+ case 1042:
+ return "SEFAM";
+ case 1043:
+ return "Wireless Cables Inc";
+ case 1044:
+ return "Lightning Protection International Pty Ltd";
+ case 1045:
+ return "Uber Technologies Inc";
+ case 1046:
+ return "SODA GmbH";
+ case 1047:
+ return "Fatigue Science";
+ case 1048:
+ return "Alpine Electronics Inc.";
+ case 1049:
+ return "Novalogy LTD";
+ case 1050:
+ return "Friday Labs Limited";
+ case 1051:
+ return "OrthoAccel Technologies";
+ case 1052:
+ return "WaterGuru, Inc.";
+ case 1053:
+ return "Benning Elektrotechnik und Elektronik GmbH & Co. KG";
+ case 1054:
+ return "Dell Computer Corporation";
+ case 1055:
+ return "Kopin Corporation";
+ case 1056:
+ return "TecBakery GmbH";
+ case 1057:
+ return "Backbone Labs, Inc.";
+ case 1058:
+ return "DELSEY SA";
+ case 1059:
+ return "Chargifi Limited";
+ case 1060:
+ return "Trainesense Ltd.";
+ case 1061:
+ return "Unify Software and Solutions GmbH & Co. KG";
+ case 1062:
+ return "Husqvarna AB";
+ case 1063:
+ return "Focus fleet and fuel management inc";
+ case 1064:
+ return "SmallLoop, LLC";
+ case 1065:
+ return "Prolon Inc.";
+ case 1066:
+ return "BD Medical";
+ case 1067:
+ return "iMicroMed Incorporated";
+ case 1068:
+ return "Ticto N.V.";
+ case 1069:
+ return "Meshtech AS";
+ case 1070:
+ return "MemCachier Inc.";
+ case 1071:
+ return "Danfoss A/S";
+ case 1072:
+ return "SnapStyk Inc.";
+ case 1073:
+ return "Amway Corporation";
+ case 1074:
+ return "Silk Labs, Inc.";
+ case 1075:
+ return "Pillsy Inc.";
+ case 1076:
+ return "Hatch Baby, Inc.";
+ case 1077:
+ return "Blocks Wearables Ltd.";
+ case 1078:
+ return "Drayson Technologies (Europe) Limited";
+ case 1079:
+ return "eBest IOT Inc.";
+ case 1080:
+ return "Helvar Ltd";
+ case 1081:
+ return "Radiance Technologies";
+ case 1082:
+ return "Nuheara Limited";
+ case 1083:
+ return "Appside co., ltd.";
+ case 1084:
+ return "DeLaval";
+ case 1085:
+ return "Coiler Corporation";
+ case 1086:
+ return "Thermomedics, Inc.";
+ case 1087:
+ return "Tentacle Sync GmbH";
+ case 1088:
+ return "Valencell, Inc.";
+ case 1089:
+ return "iProtoXi Oy";
+ case 1090:
+ return "SECOM CO., LTD.";
+ case 1091:
+ return "Tucker International LLC";
+ case 1092:
+ return "Metanate Limited";
+ case 1093:
+ return "Kobian Canada Inc.";
+ case 1094:
+ return "NETGEAR, Inc.";
+ case 1095:
+ return "Fabtronics Australia Pty Ltd";
+ case 1096:
+ return "Grand Centrix GmbH";
+ case 1097:
+ return "1UP USA.com llc";
+ case 1098:
+ return "SHIMANO INC.";
+ case 1099:
+ return "Nain Inc.";
+ case 1100:
+ return "LifeStyle Lock, LLC";
+ case 1101:
+ return "VEGA Grieshaber KG";
+ case 1102:
+ return "Xtrava Inc.";
+ case 1103:
+ return "TTS Tooltechnic Systems AG & Co. KG";
+ case 1104:
+ return "Teenage Engineering AB";
+ case 1105:
+ return "Tunstall Nordic AB";
+ case 1106:
+ return "Svep Design Center AB";
+ case 1107:
+ return "GreenPeak Technologies BV";
+ case 1108:
+ return "Sphinx Electronics GmbH & Co KG";
+ case 1109:
+ return "Atomation";
+ case 1110:
+ return "Nemik Consulting Inc";
+ case 1111:
+ return "RF INNOVATION";
+ case 1112:
+ return "Mini Solution Co., Ltd.";
+ case 1113:
+ return "Lumenetix, Inc";
+ case 1114:
+ return "2048450 Ontario Inc";
+ case 1115:
+ return "SPACEEK LTD";
+ case 1116:
+ return "Delta T Corporation";
+ case 1117:
+ return "Boston Scientific Corporation";
+ case 1118:
+ return "Nuviz, Inc.";
+ case 1119:
+ return "Real Time Automation, Inc.";
+ case 1120:
+ return "Kolibree";
+ case 1121:
+ return "vhf elektronik GmbH";
+ case 1122:
+ return "Bonsai Systems GmbH";
+ case 1123:
+ return "Fathom Systems Inc.";
+ case 1124:
+ return "Bellman & Symfon";
+ case 1125:
+ return "International Forte Group LLC";
+ case 1126:
+ return "CycleLabs Solutions inc.";
+ case 1127:
+ return "Codenex Oy";
+ case 1128:
+ return "Kynesim Ltd";
+ case 1129:
+ return "Palago AB";
+ case 1130:
+ return "INSIGMA INC.";
+ case 1131:
+ return "PMD Solutions";
+ case 1132:
+ return "Qingdao Realtime Technology Co., Ltd.";
+ case 1133:
+ return "BEGA Gantenbrink-Leuchten KG";
+ case 1134:
+ return "Pambor Ltd.";
+ case 1135:
+ return "Develco Products A/S";
+ case 1136:
+ return "iDesign s.r.l.";
+ case 1137:
+ return "TiVo Corp";
+ case 1138:
+ return "Control-J Pty Ltd";
+ case 1139:
+ return "Steelcase, Inc.";
+ case 1140:
+ return "iApartment co., ltd.";
+ case 1141:
+ return "Icom inc.";
+ case 1142:
+ return "Oxstren Wearable Technologies Private Limited";
+ case 1143:
+ return "Blue Spark Technologies";
+ case 1144:
+ return "FarSite Communications Limited";
+ case 1145:
+ return "mywerk system GmbH";
+ case 1146:
+ return "Sinosun Technology Co., Ltd.";
+ case 1147:
+ return "MIYOSHI ELECTRONICS CORPORATION";
+ case 1148:
+ return "POWERMAT LTD";
+ case 1149:
+ return "Occly LLC";
+ case 1150:
+ return "OurHub Dev IvS";
+ case 1151:
+ return "Pro-Mark, Inc.";
+ case 1152:
+ return "Dynometrics Inc.";
+ case 1153:
+ return "Quintrax Limited";
+ case 1154:
+ return "POS Tuning Udo Vosshenrich GmbH & Co. KG";
+ case 1155:
+ return "Multi Care Systems B.V.";
+ case 1156:
+ return "Revol Technologies Inc";
+ case 1157:
+ return "SKIDATA AG";
+ case 1158:
+ return "DEV TECNOLOGIA INDUSTRIA, COMERCIO E MANUTENCAO DE EQUIPAMENTOS LTDA. - ME";
+ case 1159:
+ return "Centrica Connected Home";
+ case 1160:
+ return "Automotive Data Solutions Inc";
+ case 1161:
+ return "Igarashi Engineering";
+ case 1162:
+ return "Taelek Oy";
+ case 1163:
+ return "CP Electronics Limited";
+ case 1164:
+ return "Vectronix AG";
+ case 1165:
+ return "S-Labs Sp. z o.o.";
+ case 1166:
+ return "Companion Medical, Inc.";
+ case 1167:
+ return "BlueKitchen GmbH";
+ case 1168:
+ return "Matting AB";
+ case 1169:
+ return "SOREX - Wireless Solutions GmbH";
+ case 1170:
+ return "ADC Technology, Inc.";
+ case 1171:
+ return "Lynxemi Pte Ltd";
+ case 1172:
+ return "SENNHEISER electronic GmbH & Co. KG";
+ case 1173:
+ return "LMT Mercer Group, Inc";
+ case 1174:
+ return "Polymorphic Labs LLC";
+ case 1175:
+ return "Cochlear Limited";
+ case 1176:
+ return "METER Group, Inc. USA";
+ case 1177:
+ return "Ruuvi Innovations Ltd.";
+ case 1178:
+ return "Situne AS";
+ case 1179:
+ return "nVisti, LLC";
+ case 1180:
+ return "DyOcean";
+ case 1181:
+ return "Uhlmann & Zacher GmbH";
+ case 1182:
+ return "AND!XOR LLC";
+ case 1183:
+ return "tictote AB";
+ case 1184:
+ return "Vypin, LLC";
+ case 1185:
+ return "PNI Sensor Corporation";
+ case 1186:
+ return "ovrEngineered, LLC";
+ case 1187:
+ return "GTronix";
+ case 1188:
+ return "Herbert Waldmann GmbH & Co. KG";
+ case 1189:
+ return "Guangzhou FiiO Electronics Technology Co.,Ltd";
+ case 1190:
+ return "Vinetech Co., Ltd";
case 65535:
return "internal use";
default:
--
2.11.1.windows.1
^ permalink raw reply related
* Clarification on tools/eddystone.c - how to advertise the simplest way
From: Grégoire Gentil @ 2017-03-29 1:31 UTC (permalink / raw)
To: linux-bluetooth; +Cc: szymon.janc
Hello,
I have an embedded device and I want to advertise using BLE. This code
compiles against the bluetooth library and actually works (even without
bluetoothd running):
https://github.com/carsonmcdonald/bluez-experiments/blob/master/experiments/advertisetest.c
I see the advertising on an Android phone.
I was looking for a more comprehensive and cleaner example. I was told
to use tools/eddystone.c
I have compiled the following files together:
src/shared/hci.c
src/shared/io-mainloop.c
src/shared/mainloop.c
src/shared/queue.c
src/shared/timeout-mainloop.c
src/shared/util.c
tools/eddystone.c
but when I run eddystone, nothing happens. Nothing is advertised. What
am I missing? Note that I have to "hciconfig hci0 down" to run
eddystone. Am I misunderstanding what eddystone is supposed to do?
My objective is to advertise and to listen to advertising the simplest
way (without d-bus or bluetoothd daemon)? I'm fine to compile against
the bluetooth library if needed. What is the best starting point?
Grégoire
^ permalink raw reply
* Re: [PATCH 1/3] soc: qcom: smd: Transition client drivers from smd to rpmsg
From: David Miller @ 2017-03-29 0:58 UTC (permalink / raw)
To: bjorn.andersson
Cc: marcel, gustavo, johan.hedberg, k.eugene.e, kvalo, andy.gross,
david.brown, arnd, linux-bluetooth, linux-kernel, wcn36xx,
linux-wireless, netdev, linux-arm-msm, linux-soc
In-Reply-To: <20170328054922.GH70446@Bjorns-MacBook-Pro-2.local>
From: Bjorn Andersson <bjorn.andersson@linaro.org>
Date: Mon, 27 Mar 2017 22:49:22 -0700
> On Mon 27 Mar 16:04 PDT 2017, David Miller wrote:
>
>> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Date: Mon, 27 Mar 2017 15:58:37 -0700
>>
>> > I'm sorry, but I can't figure out how to reproduce this.
>>
>> All of my builds are "make allmodconfig" so it should be easy to reproduce.
>
> Thanks, turns out that while it was possible to select CONFIG_SMD_RPM
> and CONFIG_QCOM_WCNSS_CTRL drivers/soc/Makefile does not traverse into
> qcom/ unless CONFIG_ARCH_QCOM was set.
>
> So I just sent out version 2 of the three patches, where I add an
> explicit dependency on ARCH_QCOM for those two options.
Looks great, all three applied.
^ permalink raw reply
* [bluetooth-next:master 25/28] drivers/net/ieee802154/ca8210.c:918:9-12: ERROR: spi is NULL but dereferenced. (fwd)
From: Julia Lawall @ 2017-03-28 19:55 UTC (permalink / raw)
To: Harry Morris; +Cc: Marcel Holtmann, linux-bluetooth, kbuild-all
If the call to dev_crit on line 917 is to do something, it should need a
valid pointer as th first argument.
Also, separating the () from the argument list with newlines does not look
like normal kernel coding style.
julia
---------- Forwarded message ----------
Date: Wed, 29 Mar 2017 00:26:36 +0800
From: kbuild test robot <fengguang.wu@intel.com>
To: kbuild@01.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Subject: [bluetooth-next:master 25/28] drivers/net/ieee802154/ca8210.c:918:9-12:
ERROR: spi is NULL but dereferenced.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
head: 50d1455b61a78d1f2dfca4b3366ac5925fb04838
commit: d931acd575d6a36730192756bf2cbd14c77fa1bc [25/28] ieee802154: Add CA8210 IEEE 802.15.4 device driver
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
>> drivers/net/ieee802154/ca8210.c:918:9-12: ERROR: spi is NULL but dereferenced.
git remote add bluetooth-next https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
git remote update bluetooth-next
git checkout d931acd575d6a36730192756bf2cbd14c77fa1bc
vim +918 drivers/net/ieee802154/ca8210.c
d931acd5 Harry Morris 2017-03-28 902 * @len: length of the buffer being sent
d931acd5 Harry Morris 2017-03-28 903 *
d931acd5 Harry Morris 2017-03-28 904 * Return: 0 or linux error code
d931acd5 Harry Morris 2017-03-28 905 */
d931acd5 Harry Morris 2017-03-28 906 static int ca8210_spi_transfer(
d931acd5 Harry Morris 2017-03-28 907 struct spi_device *spi,
d931acd5 Harry Morris 2017-03-28 908 const u8 *buf,
d931acd5 Harry Morris 2017-03-28 909 size_t len
d931acd5 Harry Morris 2017-03-28 910 )
d931acd5 Harry Morris 2017-03-28 911 {
d931acd5 Harry Morris 2017-03-28 912 int i, status = 0;
d931acd5 Harry Morris 2017-03-28 913 struct ca8210_priv *priv = spi_get_drvdata(spi);
d931acd5 Harry Morris 2017-03-28 914 struct cas_control *cas_ctl;
d931acd5 Harry Morris 2017-03-28 915
d931acd5 Harry Morris 2017-03-28 916 if (!spi) {
d931acd5 Harry Morris 2017-03-28 917 dev_crit(
d931acd5 Harry Morris 2017-03-28 @918 &spi->dev,
d931acd5 Harry Morris 2017-03-28 919 "NULL spi device passed to ca8210_spi_transfer\n"
d931acd5 Harry Morris 2017-03-28 920 );
d931acd5 Harry Morris 2017-03-28 921 return -ENODEV;
d931acd5 Harry Morris 2017-03-28 922 }
d931acd5 Harry Morris 2017-03-28 923
d931acd5 Harry Morris 2017-03-28 924 reinit_completion(&priv->spi_transfer_complete);
d931acd5 Harry Morris 2017-03-28 925
d931acd5 Harry Morris 2017-03-28 926 dev_dbg(&spi->dev, "ca8210_spi_transfer called\n");
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [PATCH] Bluetooth: btmrvl: cleanup code in return from btmrvl_sdio_suspend()
From: prasanna karthik @ 2017-03-28 19:44 UTC (permalink / raw)
To: marcel@holtmann.org, gustavo@padovan.org, johan.hedberg@gmail.com
Cc: linux-bluetooth@vger.kernel.org, Prasanna Karthik
Else is not generally useful after a break or return
Signed-off-by: Prasanna Karthik <pkarthik@outlook.com>
---
drivers/bluetooth/btmrvl_sdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdi=
o.c
index 08e01f0..9a5e2e8 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1637,10 +1637,10 @@ static int btmrvl_sdio_suspend(struct device *dev)
if (priv->adapter->hs_state =3D=3D HS_ACTIVATED) {
BT_DBG("suspend with MMC_PM_KEEP_POWER");
return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
- } else {
- BT_DBG("suspend without MMC_PM_KEEP_POWER");
- return 0;
}
+
+ BT_DBG("suspend without MMC_PM_KEEP_POWER");
+ return 0;
}
=20
static int btmrvl_sdio_resume(struct device *dev)
--=20
1.9.1
^ permalink raw reply related
* [RFC V1 16/16] Bluetooth: hci_ldisc: Add ioctl_mutex avoiding concurrency
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
There is a concurrency risk with hci_uart_tty_ioctl() running with
itself. Also there is a race condition between hci_uart_tty_close()
clearing the HCI_UART_PROTO_SET flag whilst hci_uart_tty_ioctl() runs.
Even though the HCI_UART_PROTO_SET flag is atomic, this is not
sufficient to prevent hci_uart_tty_ioctl() from potentially accessing
freed or stale variables when hci_uart_tty_close() completes. This is
because the hci_uart_tty_ioctl() thread can run in parallel with the
hci_uart_tty_close() thread. This means the HCI_UART_PROTO_SET flag
can change at any time during the execution of hci_uart_tty_ioctl()
which can lead to a malfunction.
Therefore, add a mutex lock called ioctl_mutex in hci_uart_tty_ioctl()
to prevent self concurrency of hci_uart_tty_ioctl(). In
hci_uart_tty_close() add ioctl_mutex around the clearing of the hu
value from tty->disc_data and clearing the HCI_UART_PROTO_SET flag.
This mutex lock ensures that tty->disc_data does not become NULL
and the HCI_UART_PROTO_SET flag cannot become clear whilst the main
body of hci_uart_tty_ioctl() executes.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 5 +++++
drivers/bluetooth/hci_uart.h | 2 ++
2 files changed, 7 insertions(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 0a31629..307fe3b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -489,6 +489,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
INIT_WORK(&hu->write_work, hci_uart_write_work);
rwlock_init(&hu->proto_lock);
+ mutex_init(&hu->ioctl_mutex);
/* Flush any pending characters in the driver */
tty_driver_flush_buffer(tty);
@@ -531,10 +532,12 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hu->proto->close(hu);
}
+ mutex_lock(&hu->ioctl_mutex);
/* Detach from the tty */
tty->disc_data = NULL;
clear_bit(HCI_UART_PROTO_SET, &hu->flags);
+ mutex_unlock(&hu->ioctl_mutex);
if (test_and_clear_bit(HCI_UART_REGISTERED, &hu->flags)) {
hci_uart_flush(hdev);
@@ -745,6 +748,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
if (!hu)
return -EBADF;
+ mutex_lock(&hu->ioctl_mutex);
switch (cmd) {
case HCIUARTSETPROTO:
if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) {
@@ -784,6 +788,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
err = n_tty_ioctl_helper(tty, file, cmd, arg);
break;
}
+ mutex_unlock(&hu->ioctl_mutex);
return err;
}
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 580ca98..25cd93b 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -91,6 +91,8 @@ struct hci_uart {
struct sk_buff *tx_skb;
unsigned long tx_state;
+ struct mutex ioctl_mutex;
+
unsigned int init_speed;
unsigned int oper_speed;
};
--
2.7.4
^ permalink raw reply related
* [RFC V1 15/16] Bluetooth: hci_ldisc: Use rwlocking to avoid closing proto races
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
When HCI_UART_PROTO_READY is in the set state, the Data Link protocol
layer (proto) is bound to the HCI UART driver. This state allows the
registered proto function pointers to be used by the HCI UART driver.
When unbinding (closing) the Data Link protocol layer, the proto
function pointers much be prevented from being used immediately before
running the proto close function pointer. Otherwise, there is a risk
that a proto non-close function pointer is used during or after the
proto close function pointer is used. The consequences are likely to
be a kernel crash because the proto close function pointer will free
resources used in the Data Link protocol layer.
Therefore, add a reader writer lock (rwlock) solution to prevent the
close proto function pointer from running by using write_lock_irqsave()
whilst the other proto function pointers are protected using
read_lock(). This means HCI_UART_PROTO_READY can safely be cleared
in the knowledge that no proto function pointers are running.
When flag HCI_UART_PROTO_READY is put into the clear state,
proto close function pointer can safely be run. Note
flag HCI_UART_PROTO_SET being in the set state prevents the proto
open function pointer from being run so there is no race condition
between proto open and close function pointers.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 33 +++++++++++++++++++++++++++++++--
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 0a10ee1..0a31629 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -114,8 +114,12 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
struct sk_buff *skb = hu->tx_skb;
if (!skb) {
+ read_lock(&hu->proto_lock);
+
if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
skb = hu->proto->dequeue(hu);
+
+ read_unlock(&hu->proto_lock);
} else {
hu->tx_skb = NULL;
}
@@ -125,6 +129,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
+ read_lock(&hu->proto_lock);
+
if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
goto no_schedule;
@@ -138,6 +144,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
schedule_work(&hu->write_work);
no_schedule:
+ read_unlock(&hu->proto_lock);
+
return 0;
}
@@ -233,9 +241,13 @@ static int hci_uart_flush(struct hci_dev *hdev)
tty_ldisc_flush(tty);
tty_driver_flush_buffer(tty);
+ read_lock(&hu->proto_lock);
+
if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
hu->proto->flush(hu);
+ read_unlock(&hu->proto_lock);
+
return 0;
}
@@ -256,11 +268,17 @@ 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))
+ read_lock(&hu->proto_lock);
+
+ if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+ read_unlock(&hu->proto_lock);
return -EUNATCH;
+ }
hu->proto->enqueue(hu, skb);
+ read_unlock(&hu->proto_lock);
+
hci_uart_tx_wakeup(hu);
return 0;
@@ -470,6 +488,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
INIT_WORK(&hu->init_ready, hci_uart_init_work);
INIT_WORK(&hu->write_work, hci_uart_write_work);
+ rwlock_init(&hu->proto_lock);
+
/* Flush any pending characters in the driver */
tty_driver_flush_buffer(tty);
@@ -486,6 +506,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
struct hci_uart *hu = tty->disc_data;
struct hci_dev *hdev;
struct sk_buff *temp_skb;
+ unsigned long flags;
BT_DBG("tty %p", tty);
@@ -502,7 +523,10 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_unregister_dev(hdev);
if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+ write_lock_irqsave(&hu->proto_lock, flags);
clear_bit(HCI_UART_PROTO_READY, &hu->flags);
+ write_unlock_irqrestore(&hu->proto_lock, flags);
+
cancel_work_sync(&hu->write_work);
hu->proto->close(hu);
}
@@ -573,13 +597,18 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
if (!hu || tty != hu->tty)
return;
- if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ read_lock(&hu->proto_lock);
+
+ if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+ read_unlock(&hu->proto_lock);
return;
+ }
/* It does not need a lock here as it is already protected by a mutex in
* tty caller
*/
hu->proto->recv(hu, data, count);
+ read_unlock(&hu->proto_lock);
if (hu->hdev)
hu->hdev->stat.byte_rx += count;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 0701395..580ca98 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -85,6 +85,7 @@ struct hci_uart {
struct work_struct write_work;
const struct hci_uart_proto *proto;
+ rwlock_t proto_lock; /* Stop work for proto close */
void *priv;
struct sk_buff *tx_skb;
--
2.7.4
^ permalink raw reply related
* [RFC V1 14/16] Bluetooth: hci_ldisc: Simplify flushing
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
The flushing due to the hci_uart_close() call probably corrupts the
data flow between the Data Link protocol layer and the UART port
(transmission direction). In any case, the flushing activity is
asynchronous to the state of the Data Link protocol layer so is
undesirable.
Modify hci_uart_close() to not perform any flushing because
hci_uart_flush() is explicitly called in hci_dev_do_close() which
is synchronous to the closure of the Data Link protocol layer so
it is OK. In other words, there is no point in flushing multiple
times with the Data Link protocol layer being in the bound state.
In hci_uart_tty_close() call hci_uart_flush() instead of
hci_uart_close() before freeing hdev. This final flush is after the
Data Link protocol layer has been unbound so ensures that the
transmission path is empty.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ad2dec5..0a10ee1 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -244,8 +244,7 @@ static int hci_uart_close(struct hci_dev *hdev)
{
BT_DBG("hdev %p", hdev);
- hci_uart_flush(hdev);
- hdev->flush = NULL;
+ /* Nothing to do for UART driver */
return 0;
}
@@ -514,7 +513,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
clear_bit(HCI_UART_PROTO_SET, &hu->flags);
if (test_and_clear_bit(HCI_UART_REGISTERED, &hu->flags)) {
- hci_uart_close(hdev);
+ hci_uart_flush(hdev);
hu->hdev = NULL;
hci_free_dev(hdev);
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 13/16] Bluetooth: hci_ldisc: hci_uart_tty_close() free hu->tx_skb
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
There is a race condition for accessing hu->tx_skb between
hci_uart_flush() and hci_uart_dequeue() which runs in
hci_uart_write_work() from the work queue hu->write_work. This race
condition exists because there is no locking between these 2 threads
to protect hu->tx_skb. Consequently a call to hci_uart_flush() might
be able to free hu->tx_skb whilst hci_uart_write_work() is using
hu->tx_skb which is undesirable as a crash could occur.
Performing any flushing in the transmission path between the Data Link
protocol layer and the UART port may corrupt the data. So freeing
hu->tx_skb or not freeing hu->tx_skb makes little difference to having
intact data or corrupted data. So it is OK not to free hu->tx_skb.
Instead, move the freeing of hu->tx_skb to the end of
hci_uart_tty_close() from hci_uart_flush(). This eliminates the race
condition because the Data Link protocol layer is in the unbound state
and ensures hu->tx_skb is freed before hu is freed. Also use a temporary
pointer to allow hu->tx_skb to be set to NULL before freeing hu->tx_skb.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 61e03dd4..ad2dec5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -229,10 +229,6 @@ static int hci_uart_flush(struct hci_dev *hdev)
BT_DBG("hdev %p tty %p", hdev, tty);
- if (hu->tx_skb) {
- kfree_skb(hu->tx_skb); hu->tx_skb = NULL;
- }
-
/* Flush any pending characters in the driver and discipline. */
tty_ldisc_flush(tty);
tty_driver_flush_buffer(tty);
@@ -490,6 +486,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
{
struct hci_uart *hu = tty->disc_data;
struct hci_dev *hdev;
+ struct sk_buff *temp_skb;
BT_DBG("tty %p", tty);
@@ -522,6 +519,12 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_free_dev(hdev);
}
+ if (hu->tx_skb) {
+ temp_skb = hu->tx_skb;
+ hu->tx_skb = NULL;
+ kfree_skb(temp_skb);
+ }
+
kfree(hu);
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 12/16] Bluetooth: hci_ldisc: hci_uart_tty_close() move cancel_work_sync()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
The current location of cancel_work_sync() in hci_uart_tty_close()
before the call to hci_unregister_dev(hdev) may momentarily interfere
with the execution of hci_uart_tty_close() by waiting for an active call
to hci_uart_write_work() to finish. However, its current location is
ineffective at preventing a race condition described next.
cancel_work_sync() is needed to try to prevent hci_uart_write_work() from
running whilst the Data Link protocol layer is being unbound (closed).
However, it is weak as a race condition exists between
hci_uart_write_work() being scheduled via hci_uart_tx_wakeup() and
closure of the Data Link protocol layer. In particular, the Data Link
protocol layer may have a retransmission timer which will independently
call hci_uart_tx_wakeup().
To minimise the race condition, cancel_work_sync() needs to be as close
as possible to the code that closes down the Data Link protocol layer.
Therefore, move the cancel_work_sync() statement to be just before
the code which unbinds (closes) the Data Link protocol layer.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index acb08c6..61e03dd4 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -498,8 +498,6 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hdev = hu->hdev;
- cancel_work_sync(&hu->write_work);
-
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
/* Note hci_unregister_dev() may try to send a HCI RESET
* command. If the transmission fails then hci_unregister_dev()
@@ -509,6 +507,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
clear_bit(HCI_UART_PROTO_READY, &hu->flags);
+ cancel_work_sync(&hu->write_work);
hu->proto->close(hu);
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 11/16] Bluetooth: hci_ldisc: hci_uart_tty_close() move hci_uart_close()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
In hci_uart_tty_close() move hci_uart_close() to after the call to
hci_unregister_dev() to avoid the tty flush in hci_uart_close()
and clearance of HCI_RUNNING from corrupting the Data Link protocol
layer's communication stream.
In fact, perform hci_uart_close() at the of hci_uart_tty_close()
where the Data Link protocol layer has been closed so that no
more data can be written in the tty layer. This makes the flush
clear out any non-sent data from the tty layer although this
is probably unnecessary.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index e7d1b8b..acb08c6 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -497,8 +497,6 @@ static void hci_uart_tty_close(struct tty_struct *tty)
return;
hdev = hu->hdev;
- if (test_bit(HCI_UART_REGISTERED, &hu->flags))
- hci_uart_close(hdev);
cancel_work_sync(&hu->write_work);
@@ -520,6 +518,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
clear_bit(HCI_UART_PROTO_SET, &hu->flags);
if (test_and_clear_bit(HCI_UART_REGISTERED, &hu->flags)) {
+ hci_uart_close(hdev);
hu->hdev = NULL;
hci_free_dev(hdev);
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 10/16] Bluetooth: hci_ldisc: hci_uart_tty_close() detach tty after last msg
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
hci_uart_tty_close() is detaching the tty prematurely which prevents
the Data Link protocol layer from using the tty serial port in the
call to hci_unregister_dev().
Consequently, when hci_unregister_dev() attempts to send a HCI RESET
command, the BCSP layer attempts to retransmit the message many times
and the command is timed-out after HCI_CMD_TIMEOUT (2) seconds.
Nothing was physically transmitted because the tty was detached too
early.
Therefore, move the detach statement "tty->disc_data = NULL" to after
hci_unregister_dev() so that the HCI RESET command maybe sent and
acknowledged.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 2d5c6f0..e7d1b8b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -493,9 +493,6 @@ static void hci_uart_tty_close(struct tty_struct *tty)
BT_DBG("tty %p", tty);
- /* Detach from the tty */
- tty->disc_data = NULL;
-
if (!hu)
return;
@@ -517,6 +514,9 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hu->proto->close(hu);
}
+ /* Detach from the tty */
+ tty->disc_data = NULL;
+
clear_bit(HCI_UART_PROTO_SET, &hu->flags);
if (test_and_clear_bit(HCI_UART_REGISTERED, &hu->flags)) {
--
2.7.4
^ permalink raw reply related
* [RFC V1 09/16] Bluetooth: hci_ldisc: Tidy-up HCI_UART_REGISTERED in hci_uart_tty_close()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
The code in hci_uart_tty_close is over complex in handling flag
HCI_UART_REGISTERED as it is unnecessary to check that hdev is NULL.
This is because hdev is only valid when HCI_UART_REGISTERED is in
the set state.
Therefore, remove all "if (hdev)" checks and instead check for flag
HCI_UART_REGISTERED being in the set state.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 9e3604d..2d5c6f0 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -500,20 +500,17 @@ static void hci_uart_tty_close(struct tty_struct *tty)
return;
hdev = hu->hdev;
- if (hdev)
+ if (test_bit(HCI_UART_REGISTERED, &hu->flags))
hci_uart_close(hdev);
cancel_work_sync(&hu->write_work);
- if (hdev) {
- if (test_bit(HCI_UART_REGISTERED, &hu->flags))
- /* Note hci_unregister_dev() may try to send a
- * HCI RESET command. If the transmission fails then
- * hci_unregister_dev() waits HCI_CMD_TIMEOUT
- * (2) seconds for the timeout to occur.
- */
- hci_unregister_dev(hdev);
- }
+ if (test_bit(HCI_UART_REGISTERED, &hu->flags))
+ /* Note hci_unregister_dev() may try to send a HCI RESET
+ * command. If the transmission fails then hci_unregister_dev()
+ * waits HCI_CMD_TIMEOUT (2) seconds for the timeout to occur.
+ */
+ hci_unregister_dev(hdev);
if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
clear_bit(HCI_UART_PROTO_READY, &hu->flags);
--
2.7.4
^ permalink raw reply related
* [RFC V1 08/16] Bluetooth: hci_ldisc: Separate flag handling in hci_uart_tty_close()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
The treatment of flags HCI_UART_REGISTERED and HCI_UART_PROTO_READY
in hci_uart_tty_close() was combined in multiple if statements. This
is over-complex and can be simplified.
The simplification is to separate the processing of the flags
HCI_UART_REGISTERED and HCI_UART_PROTO_READY in hci_uart_tty_close()
because registration of the HCI UART device is actually independent of
the binding of the Data Link protocol layer. The Data Link protocol
layer is bound before the HCI UART device is registered and therefore
unregister the HCI UART device before unbinding the Data Link protocol
layer. However, the software should not break if the Data Link protocol
somehow becomes unbound whilst the HCI UART device is still registered.
One of the reasons the software got over complex was because
hci_uart_send_frame() did not check the status of the
HCI_UART_PROTO_READY flag which meant that the hdev->workqueue could
operate on an unbound Data Link protocol layer. The complexity of the
logic prevented hci_uart_send_frame() from crashing.
The commit
"Bluetooth: Add protocol check to hci_uart_send_frame()"
for adding the HCI_UART_PROTO_READY flag check to hci_uart_send_frame()
is necessary to allow this simplification to work. Otherwise,
hci_uart_send_frame() can attempt to write to the unbound Data Link
protocol layer resulting in a crash.
Note hci_uart_write_work() races with hci_uart_tty_close() so it is
important that freeing of hdev is moved to the end of
hci_uart_tty_close() to reduce the risk of use after free of hdev.
For completeness set hu->hdev to NULL before freeing hdev so that
hu no longer references the freed hdev.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index a1bb4d64b9..9e3604d 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -505,22 +505,28 @@ static void hci_uart_tty_close(struct tty_struct *tty)
cancel_work_sync(&hu->write_work);
- if (test_and_clear_bit(HCI_UART_PROTO_READY, &hu->flags)) {
- if (hdev) {
- if (test_bit(HCI_UART_REGISTERED, &hu->flags))
- /* Note hci_unregister_dev() may try to send
- * a HCI RESET command. If the transmission
- * fails then hci_unregister_dev() waits
- * HCI_CMD_TIMEOUT (2) seconds for the timeout
- * to occur.
- */
- hci_unregister_dev(hdev);
- hci_free_dev(hdev);
- }
+ if (hdev) {
+ if (test_bit(HCI_UART_REGISTERED, &hu->flags))
+ /* Note hci_unregister_dev() may try to send a
+ * HCI RESET command. If the transmission fails then
+ * hci_unregister_dev() waits HCI_CMD_TIMEOUT
+ * (2) seconds for the timeout to occur.
+ */
+ hci_unregister_dev(hdev);
+ }
+
+ if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+ clear_bit(HCI_UART_PROTO_READY, &hu->flags);
hu->proto->close(hu);
}
+
clear_bit(HCI_UART_PROTO_SET, &hu->flags);
+ if (test_and_clear_bit(HCI_UART_REGISTERED, &hu->flags)) {
+ hu->hdev = NULL;
+ hci_free_dev(hdev);
+ }
+
kfree(hu);
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 07/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-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 | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c4b801b..a1bb4d64b9 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -125,15 +125,19 @@ 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))
+ goto no_schedule;
+
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
- return 0;
+ goto no_schedule;
}
BT_DBG("");
schedule_work(&hu->write_work);
+no_schedule:
return 0;
}
--
2.7.4
^ permalink raw reply related
* [RFC V1 06/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-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 eeea305..c4b801b 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
* [RFC V1 05/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-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 c78c9dc..eeea305 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -255,6 +255,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
* [RFC V1 04/16] Bluetooth: hci_ldisc: Add HCI RESET comment to hci_unregister_dev() call
From: Dean Jenkins @ 2017-03-28 17:50 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
In-Reply-To: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com>
This commit relates to the HCI UART quirk HCI_QUIRK_RESET_ON_CLOSE
which is enabled by default and can be disabled by setting hdev_flags
flag HCI_UART_RESET_ON_INIT via ioctl HCIUARTSETFLAGS from userland.
The implementation of HCI_QUIRK_RESET_ON_CLOSE is broken for the BCSP
Data Link protocol layer because it can be seen that
hci_unregister_dev() takes 2 seconds to run due to BCSP communications
failure. Suspect that sending of the HCI RESET command is broken
for the other Data Link protocols as well.
Therefore, add a comment to inform that the call to
hci_unregister_dev() in hci_uart_tty_close() may send a HCI RESET
command. This transmission needs the HCI UART driver to remain
operational including having the Data Link protocol being bound to
the HCI UART driver. If the transmission fails, then
hci_unregister_dev() waits HCI_CMD_TIMEOUT (2) seconds for the
timeout to occur which is undesirable.
The current software has a call to hci_unregister_dev(hdev) in
hci_uart_tty_close() which can cause an attempt at sending the
command HCI RESET to occur through the following callstack:
hci_uart_tty_close()
hci_unregister_dev()
hci_dev_do_close()
__hci_req_sync() to queue up command HCI RESET
which adds a work-item to the hdev->workqueue and waits 2 seconds
for a response
Subsequently
hdev->workqueue runs and processes the work-item by calling
hci_cmd_work()
hci_send_frame()
hci_uart_send_frame() to enqueue into the Data Link protocol layer
No protocol response comes so hci_unregister_dev() takes 2 seconds to
run!
In fact, this hidden transmission of the HCI RESET command is most
likely the root cause of why hci_uart_tty_close() crashed in the past
and was "fixed" with multiple workarounds in the past.
The underlying design flaw is that hci_uart_tty_close() is interfering
with various aspects of transmitting and receiving HCI messages
whilst hci_unregister_dev() is trying to use the Data Link protocol
to send the HCI RESET command. Consequently, the design flaw
causes the Data Link protocol to retransmit as no reply comes from
the Bluetooth Radio module over the UART link.
Over the many years of "fixes", this caused the logic in
hci_uart_tty_close() to become over-complex.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 1887ad4..c78c9dc 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -499,6 +499,12 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (test_and_clear_bit(HCI_UART_PROTO_READY, &hu->flags)) {
if (hdev) {
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
+ /* Note hci_unregister_dev() may try to send
+ * a HCI RESET command. If the transmission
+ * fails then hci_unregister_dev() waits
+ * HCI_CMD_TIMEOUT (2) seconds for the timeout
+ * to occur.
+ */
hci_unregister_dev(hdev);
hci_free_dev(hdev);
}
--
2.7.4
^ permalink raw reply related
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