* [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-13 11:43 UTC (permalink / raw)
To: linux-bluetooth
This patch adds btd_adapter_recreate_bonding() which rebonds a device,
i.e. it performs an unbonding operation inmediately followed by a
bonding operation.
Although there is no internal use for btd_adapter_recreate_bonding()
yet, it is useful for plugins dealing with devices which require
renegotiaing their keys. For instance, when dealing with devices
without persistent storage which lose their keys on reset.
Certain caveats have been taken into account when rebonding with a LE
device:
* If the device to rebond is already connected, the rebonding is done
without disconnecting to avoid the extra latency of reestablishing
the connection.
* If no connection exists, we connect before unbonding anyways to
avoid losing the device's autoconnection and connection parameters,
which are removed by the kernel when unpairing if no connection
exists.
* Not closing the connection requires defering attribute discovery
until the rebonding is done. Otherwise, the security level could be
elavated with the old LTK, which may be invalid since we are
rebonding. When rebonding, attribute discovery is suspended and the
ATT socket is saved to allow resuming it once bonding is finished.
---
src/adapter.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
src/adapter.h | 7 ++++++-
src/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
src/device.h | 4 ++++
4 files changed, 119 insertions(+), 7 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 92ee1a0..81e8f22 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -5195,14 +5195,15 @@ int btd_adapter_read_clock(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
}
int btd_adapter_remove_bonding(struct btd_adapter *adapter,
- const bdaddr_t *bdaddr, uint8_t bdaddr_type)
+ const bdaddr_t *bdaddr, uint8_t bdaddr_type,
+ uint8_t disconnect)
{
struct mgmt_cp_unpair_device cp;
memset(&cp, 0, sizeof(cp));
bacpy(&cp.addr.bdaddr, bdaddr);
cp.addr.type = bdaddr_type;
- cp.disconnect = 1;
+ cp.disconnect = disconnect;
if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE,
adapter->dev_id, sizeof(cp), &cp,
@@ -5212,6 +5213,53 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter,
return -EIO;
}
+int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr,
+ uint8_t bdaddr_type,
+ uint8_t io_cap)
+{
+ struct btd_device *device;
+ int err;
+
+ device = btd_adapter_get_device(adapter, bdaddr, bdaddr_type);
+
+ if (!device || !device_is_bonded(device, bdaddr_type))
+ return -EINVAL;
+
+ device_set_rebonding(device, bdaddr_type, true);
+
+ /* Make sure the device is connected before unbonding to avoid
+ * losing the device's autoconnection and connection
+ * parameters, which are removed by the kernel when unpairing
+ * if no connection exists. We would had anyways implicitly
+ * connected when bonding later on, so we might as well just
+ * do it explicitly now.
+ */
+ if (bdaddr_type != BDADDR_BREDR && !btd_device_is_connected(device)) {
+ err = device_connect_le(device);
+ if (err < 0)
+ goto failed;
+ }
+
+ /* Unbond without disconnecting to avoid the connection
+ * re-establishment latency
+ */
+ err = btd_adapter_remove_bonding(adapter, bdaddr, bdaddr_type, 0);
+ if (err < 0)
+ goto failed;
+
+ err = adapter_create_bonding(adapter, bdaddr, bdaddr_type, io_cap);
+ if (err < 0)
+ goto failed;
+
+ return 0;
+
+failed:
+ error("failed: %s", strerror(-err));
+ device_set_rebonding(device, bdaddr_type, false);
+ return err;
+}
+
static void pincode_reply_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -5594,8 +5642,11 @@ static void bonding_complete(struct btd_adapter *adapter,
else
device = btd_adapter_find_device(adapter, bdaddr, addr_type);
- if (device != NULL)
+ if (device != NULL) {
device_bonding_complete(device, addr_type, status);
+ if (device_is_rebonding(device, addr_type))
+ device_rebonding_complete(device, addr_type);
+ }
resume_discovery(adapter);
diff --git a/src/adapter.h b/src/adapter.h
index 6801fee..bd00d3e 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -158,7 +158,12 @@ int btd_adapter_disconnect_device(struct btd_adapter *adapter,
uint8_t bdaddr_type);
int btd_adapter_remove_bonding(struct btd_adapter *adapter,
- const bdaddr_t *bdaddr, uint8_t bdaddr_type);
+ const bdaddr_t *bdaddr, uint8_t bdaddr_type,
+ uint8_t disconnect);
+int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr,
+ uint8_t bdaddr_type,
+ uint8_t io_cap);
int btd_adapter_pincode_reply(struct btd_adapter *adapter,
const bdaddr_t *bdaddr,
diff --git a/src/device.c b/src/device.c
index 875a5c5..4aab349 100644
--- a/src/device.c
+++ b/src/device.c
@@ -158,6 +158,7 @@ struct att_callbacks {
struct bearer_state {
bool paired;
bool bonded;
+ bool rebonding;
bool connected;
bool svc_resolved;
};
@@ -221,6 +222,8 @@ struct btd_device {
int8_t rssi;
GIOChannel *att_io;
+ GIOChannel *att_rebond_io;
+
guint cleanup_id;
guint store_id;
};
@@ -522,6 +525,9 @@ static void device_free(gpointer user_data)
attio_cleanup(device);
+ if (device->att_rebond_io)
+ g_io_channel_unref(device->att_rebond_io);
+
if (device->tmp_records)
sdp_list_free(device->tmp_records,
(sdp_free_func_t) sdp_record_free);
@@ -1739,6 +1745,23 @@ long device_bonding_last_duration(struct btd_device *device)
return bonding->last_attempt_duration_ms;
}
+bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type)
+{
+ struct bearer_state *state = get_state(device, bdaddr_type);
+
+ return state->rebonding;
+}
+
+void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
+ bool rebonding)
+{
+ struct bearer_state *state = get_state(device, bdaddr_type);
+
+ state->rebonding = rebonding;
+
+ DBG("rebonding = %d", rebonding);
+}
+
static void create_bond_req_exit(DBusConnection *conn, void *user_data)
{
struct btd_device *device = user_data;
@@ -2029,7 +2052,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
if (state->paired && !state->bonded)
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- bdaddr_type);
+ bdaddr_type, 1);
if (device->bredr_state.connected || device->le_state.connected)
return;
@@ -2639,13 +2662,13 @@ static void device_remove_stored(struct btd_device *device)
if (device->bredr_state.bonded) {
device->bredr_state.bonded = false;
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- BDADDR_BREDR);
+ BDADDR_BREDR, 1);
}
if (device->le_state.bonded) {
device->le_state.bonded = false;
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- device->bdaddr_type);
+ device->bdaddr_type, 1);
}
device->bredr_state.paired = false;
@@ -3628,6 +3651,18 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
GAttrib *attrib;
BtIOSecLevel sec_level;
+ DBG("");
+
+ if (dev->le_state.rebonding) {
+ DBG("postponing due to rebonding");
+ /* Keep the att socket, and defer attaching the attributes
+ * until rebonding is done.
+ */
+ if (!dev->att_rebond_io)
+ dev->att_rebond_io = g_io_channel_ref(io);
+ return false;
+ }
+
bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
BT_IO_OPT_INVALID);
if (gerr) {
@@ -4269,6 +4304,23 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
}
}
+bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type)
+{
+ bool ret = true;
+
+ DBG("");
+
+ device_set_rebonding(device, bdaddr_type, false);
+
+ if (bdaddr_type != BDADDR_BREDR && device->att_rebond_io) {
+ ret = device_attach_attrib(device, device->att_rebond_io);
+ g_io_channel_unref(device->att_rebond_io);
+ device->att_rebond_io = NULL;
+ }
+
+ return ret;
+}
+
static gboolean svc_idle_cb(gpointer user_data)
{
struct svc_callback *cb = user_data;
diff --git a/src/device.h b/src/device.h
index 2e0473e..65b1018 100644
--- a/src/device.h
+++ b/src/device.h
@@ -94,6 +94,10 @@ bool device_is_retrying(struct btd_device *device);
void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
uint8_t status);
gboolean device_is_bonding(struct btd_device *device, const char *sender);
+bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type);
+void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
+ bool rebonding);
+bool device_complete_rebonding(struct btd_device *device, uint8_t bdaddr_type);
void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
void device_bonding_failed(struct btd_device *device, uint8_t status);
struct btd_adapter_pin_cb_iter *device_bonding_iter(struct btd_device *device);
--
1.9.1
^ permalink raw reply related
* [PATCH] android/pts: PTS tests update for RFCOMM
From: Sebastian Chlad @ 2014-10-13 11:29 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Sebastian Chlad
Test TC_RFC_BV_25_C depends on kernel patches for Non-Supported Command,
"Bluetooth: Fix RFCOMM NSC response" and "Bluetooth: Improve
RFCOMM__test_pf macro robustness"
---
android/pts-rfcomm.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/android/pts-rfcomm.txt b/android/pts-rfcomm.txt
index 5d51905..98a691b 100644
--- a/android/pts-rfcomm.txt
+++ b/android/pts-rfcomm.txt
@@ -1,8 +1,9 @@
PTS test results for RFCOMM
PTS version: 5.3
-Tested: 08-October-2014
+Tested: 13-October-2014
Android version: 4.4.4
+Kernel version: 3.18
Results:
PASS test passed
@@ -18,6 +19,7 @@ TC_RFC_BV_01_C PASS rctest -n -P 1 <btaddr>
TC_RFC_BV_02_C PASS rctest -r -P 1
TC_RFC_BV_03_C PASS rctest -r -P 1
TC_RFC_BV_04_C PASS Note: use ETS provided in PTS issue #12414
+ rctest -r -P 1
TC_RFC_BV_05_C PASS rctest -n -P 4 <btaddr>
Note: test requires IUT to connect on the given
channel. sdptool browse <btaddr> to check the
@@ -35,5 +37,5 @@ TC_RFC_BV_17_C PASS rctest -d -P 1
TC_RFC_BV_19_C PASS
TC_RFC_BV_21_C INC PTS issue #12421
TC_RFC_BV_22_C INC PTS issue #12421
-TC_RFC_BV_25_C INC PTS issue #12621
+TC_RFC_BV_25_C PASS rctest -r -P 1
-------------------------------------------------------------------------------
--
1.8.5.3
^ permalink raw reply related
* [PATCH] android/pts: PIXITs and PICS' for L2CAP on PTS 5.3
From: Sebastian Chlad @ 2014-10-13 10:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Sebastian Chlad
---
android/pics-l2cap.txt | 136 +++++++++++++++++++++++++++---------------------
android/pixit-l2cap.txt | 12 ++++-
2 files changed, 87 insertions(+), 61 deletions(-)
diff --git a/android/pics-l2cap.txt b/android/pics-l2cap.txt
index 8e726e4..56036a0 100644
--- a/android/pics-l2cap.txt
+++ b/android/pics-l2cap.txt
@@ -1,6 +1,6 @@
L2CAP PICS for the PTS tool.
-PTS version: 5.2
+PTS version: 5.3
* - different than PTS defaults
# - not yet implemented/supported
@@ -12,14 +12,21 @@ O - optional
-------------------------------------------------------------------------------
Parameter Name Selected Description
-------------------------------------------------------------------------------
-TSPC_L2CAP_1_1 True Data Channel Initiator (C.1)
+TSPC_L2CAP_1_1 True Data Channel Initiator (C.3)
TSPC_L2CAP_1_2 True Data Channel Acceptor (C.1)
-TSPC_L2CAP_1_3 True (#) LE Master (C.2)
-TSPC_L2CAP_1_4 True (#) LE Slave (C.2)
+TSPC_L2CAP_1_3 True LE Master (C.2)
+TSPC_L2CAP_1_4 True LE Slave (C.2)
+TSPC_L2CAP_1_5 True LE Data Channel Initiator (C.4)
+TSPC_L2CAP_1_6 True LE Data Channel Acceptor (C.5)
-------------------------------------------------------------------------------
-C.1: Mandatory IF BR/EDR or BR/EDR/LE is claimed, ELSE Excluded.
+C.1: Mandatory IF BR/EDR or BR/EDR/LE is supported, otherwise Excluded.
C.2: Mandatory to support (at least one of TSPC_L2CAP_1_3 or TSPC_L2CAP_1_4)
- IF LE or BR/EDR/LE claimed, ELSE Excluded.
+ IF LE or BR/EDR/LE is supported, otherwise Excluded.
+C.3: Optional IF LE or BR/EDR/LE is supported, otherwise Excluded.
+C.4: Optional IF LE or BR/EDR/LE and Core Spec v4.1 or Core Spec v4.1+HS and
+ TSPC_L2CAP_2_46 is supported, otherwise Excluded.
+C.5: Mandatory IF LE or BR/EDR/LE and Core Spec v4.1 or Core Spec v4.1+HS and
+ TSPC_L2CAP_2_46 is supported, otherwise Excluded.
-------------------------------------------------------------------------------
@@ -29,63 +36,66 @@ Parameter Name Selected Description
-------------------------------------------------------------------------------
TSPC_L2CAP_2_1 True Support of L2CAP signaling channel (C.20)
TSPC_L2CAP_2_2 True Support of configuration process (C.20)
-TSPC_L2CAP_2_4 True Support of command echo request (C.21)
TSPC_L2CAP_2_3 True Support of connection oriented data
channel (C.20)
+TSPC_L2CAP_2_4 True Support of command echo request (C.21)
TSPC_L2CAP_2_5 True Support of command echo response (C.20)
-TSPC_L2CAP_2_6 True (*) Support of command information request (C.21)
+TSPC_L2CAP_2_6 True Support of command information request (C.21)
TSPC_L2CAP_2_7 True Support of command information response (C.20)
-TSPC_L2CAP_2_8 False Support of a channel group (C.21)
-TSPC_L2CAP_2_9 False Support of packet for connectionless
+TSPC_L2CAP_2_8 False (*) Support of a channel group (C.21)
+TSPC_L2CAP_2_9 False (*) Support of packet for connectionless
channel (C.21)
-TSPC_L2CAP_2_10 False Support retransmission mode (C.21)
-TSPC_L2CAP_2_11 False Support flow control mode(C.21)
-TSPC_L2CAP_2_12 True (*) Enhanced Retransmission Mode (C.1, C.13)
-TSPC_L2CAP_2_13 True (*) Streaming Mode (C.1, C.14)
-TSPC_L2CAP_2_14 True (*) FCS Option (C.2)
-TSPC_L2CAP_2_15 True (*) Generate Local Busy Condition (C.3)
-TSPC_L2CAP_2_16 False Send Reject (C.3)
-TSPC_L2CAP_2_17 True (*) Send Selective Reject (C.3)
-TSPC_L2CAP_2_18 True (*) Mandatory use of ERTM (C.4)
-TSPC_L2CAP_2_19 True (*) Mandatory use of Streaming Mode (C.5)
-TSPC_L2CAP_2_20 True (*) Optional use of ERTM (C.4)
-TSPC_L2CAP_2_21 True (*) Optional use of Streaming Mode (C.5)
-TSPC_L2CAP_2_22 True (*) Send data using SAR in ERTM (C.6)
-TSPC_L2CAP_2_23 True (*) Send data using SAR in Streaming Mode (C.7)
-TSPC_L2CAP_2_24 True (*) Actively request Basic Mode for a PSM that
+TSPC_L2CAP_2_10 False (*) Support retransmission mode (C.21)
+TSPC_L2CAP_2_11 False (*) Support flow control mode(C.21)
+TSPC_L2CAP_2_12 True Enhanced Retransmission Mode (C.1, C.13)
+TSPC_L2CAP_2_13 True Streaming Mode (C.1, C.14)
+TSPC_L2CAP_2_14 True FCS Option (C.2)
+TSPC_L2CAP_2_15 True Generate Local Busy Condition (C.3)
+TSPC_L2CAP_2_16 False (*) Send Reject (C.3)
+TSPC_L2CAP_2_17 True Send Selective Reject (C.3)
+TSPC_L2CAP_2_18 True Mandatory use of ERTM (C.4)
+TSPC_L2CAP_2_19 True Mandatory use of Streaming Mode (C.5)
+TSPC_L2CAP_2_20 True Optional use of ERTM (C.4)
+TSPC_L2CAP_2_21 True Optional use of Streaming Mode (C.5)
+TSPC_L2CAP_2_22 True Send data using SAR in ERTM (C.6)
+TSPC_L2CAP_2_23 True Send data using SAR in Streaming Mode (C.7)
+TSPC_L2CAP_2_24 True Actively request Basic Mode for a PSM that
supports the use of ERTM or Streaming
Mode (C.8)
-TSPC_L2CAP_2_25 True (*) Supports performing L2CAP channel mode
+TSPC_L2CAP_2_25 True Supports performing L2CAP channel mode
configuration fallback from SM
to ERTM (C.9)
-TSPC_L2CAP_2_26 True (*) Supports sending more than one unacknowledged
+TSPC_L2CAP_2_26 True Supports sending more than one unacknowledged
I-Frame when operating in ERTM (C.10)
-TSPC_L2CAP_2_27 True (*) Supports sending more than three unacknowledged
+TSPC_L2CAP_2_27 True Supports sending more than three unacknowledged
I-Frame when operating in ERTM (C.10)
-TSPC_L2CAP_2_28 True (*) Supports configuring the peer TxWindow
+TSPC_L2CAP_2_28 True Supports configuring the peer TxWindow
greater than 1 (C.11)
-TSPC_L2CAP_2_29 False AMP Support (C.12)
-TSPC_L2CAP_2_30 True (*) Fixed Channel Support (C.12)
-TSPC_L2CAP_2_31 False AMP Manager Support (C.12)
-TSPC_L2CAP_2_32 False ERTM over AMP (C.12)
-TSPC_L2CAP_2_33 False Streaming Mode Source over AMP Support (C.15)
-TSPC_L2CAP_2_34 False Streaming Mode Sink over AMP Support (C.15)
-TSPC_L2CAP_2_35 True (*) Unicast Connectionless Data, Reception
+TSPC_L2CAP_2_29 False (*) AMP Support (C.12)
+TSPC_L2CAP_2_30 True Fixed Channel Support (C.12)
+TSPC_L2CAP_2_31 False (*) AMP Manager Support (C.12)
+TSPC_L2CAP_2_32 False (*) ERTM over AMP (C.12)
+TSPC_L2CAP_2_33 False (*) Streaming Mode Source over AMP Support (C.15)
+TSPC_L2CAP_2_34 False (*) Streaming Mode Sink over AMP Support (C.15)
+TSPC_L2CAP_2_35 True Unicast Connectionless Data, Reception
(C.1, C.16)
-TSPC_L2CAP_2_36 True (*) Ability to transmit an unencrypted packet over
+TSPC_L2CAP_2_36 True Ability to transmit an unencrypted packet over
a Unicast connectionless L2CAP
channel (C.16)
-TSPC_L2CAP_2_37 True (*) Ability to transmit an encrypted packet over
+TSPC_L2CAP_2_37 True Ability to transmit an encrypted packet over
a Unicast connectionless L2CAP
channel (C.16)
-TSPC_L2CAP_2_38 False Extended Flow Specification for BR/EDR (C.8)
-TSPC_L2CAP_2_39 False Extended Window Size (C.8)
-TSPC_L2CAP_2_40 True (*) Support of Low Energy signaling channel (C.17)
-TSPC_L2CAP_2_41 True (*) Support of command reject (C.17)
-TSPC_L2CAP_2_42 True (*) Send Connection Parameter Update Request (C.18)
-TSPC_L2CAP_2_43 True (*) Send Connection Parameter Update Response (C.19)
-TSPC_L2CAP_2_44 False Extended Flow Specification for AMP (C.22)
-TSPC_L2CAP_2_45 True (*) Send disconnect request command (O)
+TSPC_L2CAP_2_38 False (*) Extended Flow Specification for BR/EDR (C.8)
+TSPC_L2CAP_2_39 False (*) Extended Window Size (C.8)
+TSPC_L2CAP_2_40 True Support of Low Energy signaling channel (C.17)
+TSPC_L2CAP_2_41 True Support of command reject (C.17)
+TSPC_L2CAP_2_42 True Send Connection Parameter Update Request (C.18)
+TSPC_L2CAP_2_43 True Send Connection Parameter Update Response (C.19)
+TSPC_L2CAP_2_44 False (*) Extended Flow Specification for AMP (C.22)
+TSPC_L2CAP_2_45 True Send disconnect request command (O)
+TSCP_L2CAP_2_46 True Support LE Credit Based Flow Control
+ Mode (C.23)
+TSCP_L2CAP_2_47 True Support for LE Data Channel (C.24)
-------------------------------------------------------------------------------
C.1: Mandatory to support at least one of TSPC_L2CAP_2_12 OR TSPC_L2CAP_2_13 OR
TSPC_L2CAP_2_35 IF BR/EDR BR/EDR/LE AND SUM_ICS 31/7 (CSA1) OR
@@ -115,8 +125,12 @@ C.17: Mandatory IF LE OR BR/EDR/LE is claimed, ELSE Excluded.
C.18: Optional IF (SUM_ICS 31/10 AND 1/4) is claimed, ELSE Excluded.
C.19: Mandatory IF (SUM_ICS 31/10 AND 1/3) is claimed, ELSE Excluded.
C.20: Mandatory IF LE OR BR/EDR/LE, is claimed, ELSE Excluded
-C.21: Optional IF LE OR BR/EDR/LE, is claimed, ELSE Excluded
+C.21: Optional IF LE OR BR/EDR/LE, is claimed, ELSE Excluded.
C.22: Mandatory IF TSPC_L2CAP_2_29 is claimed, ELSE Excluded.
+C.23: Optional LE OR BR/EDR/LE AND Core Spec v4.1 OR Core Spec v4.1+HS
+ is supported, otherwise Excluded.
+C.24: Mandatory IF TSPC_L2CAP_2_46 (Support LE Credit Based Flow Control Mode)
+ is supported, otherwise Excluded.
-------------------------------------------------------------------------------
@@ -124,30 +138,32 @@ C.22: Mandatory IF TSPC_L2CAP_2_29 is claimed, ELSE Excluded.
-------------------------------------------------------------------------------
Parameter Name Selected Description
-------------------------------------------------------------------------------
-TSPC_L2CAP_3_1 True Support of RTX timer (M)
+TSPC_L2CAP_3_1 True Support of RTX timer (C.4)
TSPC_L2CAP_3_2 True Support of ERTX timer (C.4)
TSPC_L2CAP_3_3 True Support minimum MTU size 48 octets (C.4)
-TSPC_L2CAP_3_4 True (*) Support MTU size larger than 48 octets (C.5)
+TSPC_L2CAP_3_4 True Support MTU size larger than 48 octets (C.5)
TSPC_L2CAP_3_5 True Support of flush timeout value for reliable
channel (C.4)
-TSPC_L2CAP_3_6 False Support of flush timeout value for unreliable
+TSPC_L2CAP_3_6 False (*) Support of flush timeout value for unreliable
channel (C.5)
-TSPC_L2CAP_3_7 False Support of bi-directional quality of service
+TSPC_L2CAP_3_7 False (*) Support of bi-directional quality of service
(QoS) option field (C.1)
-TSPC_L2CAP_3_8 False Negotiate QoS service type (C.5)
-TSPC_L2CAP_3_9 False Negotiate and support service type ‘No
+TSPC_L2CAP_3_8 False (*) Negotiate QoS service type (C.5)
+TSPC_L2CAP_3_9 False (*) Negotiate and support service type ‘No
traffic’ (C.2)
-TSPC_L2CAP_3_10 False Negotiate and support service type ‘Best
+TSPC_L2CAP_3_10 False (*) Negotiate and support service type ‘Best
effort’ (C.3)
-TSPC_L2CAP_3_11 False Negotiate and support service type
+TSPC_L2CAP_3_11 False (*) Negotiate and support service type
‘Guaranteed’ (C.2)
-TSPC_L2CAP_3_12 True (*) Support minimum MTU size 23 octets (C.6)
-TSPC_L2CAP_3_13 False Negotiate and support service type ‘No traffic’
+TSPC_L2CAP_3_12 True Support minimum MTU size 23 octets (C.6)
+TSPC_L2CAP_3_13 False (*) Negotiate and support service type ‘No traffic’
for Extended Flow Specification (C.7)
-TSPC_L2CAP_3_14 False Negotiate and support service type ‘Best Effort'
+TSPC_L2CAP_3_14 False (*) Negotiate and support service type ‘Best Effort'
for Extended Flow Specification (C.8)
-TSPC_L2CAP_3_15 False Negotiate and support service type ‘Guaranteed’
+TSPC_L2CAP_3_15 False (*) Negotiate and support service type ‘Guaranteed’
for Extended Flow Specification (C.9)
+TSPC_L2CAP_3_16 True Support Multiple Simultaneous LE Data
+ Channels (C.10)
-------------------------------------------------------------------------------
C.1: Mandatory if TSPC_L2CAP_3_8 is supported, ELSE Optional.
C.2: Optional if TSPC_L2CAP_3_8 is supported, ELSE Excluded.
@@ -159,4 +175,6 @@ C.7: Optional if TSPC_L2CAP_2_44 OR TSPC_L2CAP_2_38 is supported, ELSE Excluded.
C.8: Mandatory if TSPC_L2CAP_2_44 OR TSPC_L2CAP_2_38 is supported,
ELSE Excluded.
C.9: Optional if TSPC_L2CAP_2_44 OR TSPC_L2CAP_2_38 is supported, ELSE Excluded.
+C.10: Optional IF TSPC_L2CAP_2_47 (Support LE Data Channel) is supported,
+ otherwise Excluded.
-------------------------------------------------------------------------------
diff --git a/android/pixit-l2cap.txt b/android/pixit-l2cap.txt
index f0d6b1d..07b24e1 100644
--- a/android/pixit-l2cap.txt
+++ b/android/pixit-l2cap.txt
@@ -1,6 +1,6 @@
L2CAP PIXIT for the PTS tool.
-PTS version: 5.2
+PTS version: 5.3
* - different than PTS defaults
& - should be set to IUT Bluetooth address
@@ -19,8 +19,15 @@ TSPX_flushto FFFF
TSPX_inmtu 02A0
TSPX_no_fail_verditcs FALSE
TSPX_oumtu 02A0
-TSPX_iut_role_initiator TRUE
+TSPX_tester_mps 0010
+TSPX_tester_mtu 02A0
+TSPX_iut_role_initiator TRUE (*)
+TSPX_le_psm 0080 (*)
TSPX_psm 1011 (*)
+TSPX_psm_unsupported 00F1
+TSPX_psm_authentication_required 00F2
+TSPX_psm_authorization_required 00F3
+TSPX_psm_encryption_key_size_required 00F4
TSPX_time_guard 180000
TSPX_timer_ertx 120000
TSPX_timer_ertx_max 300000
@@ -38,4 +45,5 @@ TSPX_use_implicit_send TRUE
TSPX_use_dynamic_pin FALSE
TSPX_iut_SDU_size_in_bytes 144
TSPX_secure_simple_pairing_pass_key_confirmation FALSE
+TSPX_iut_address_type_random FALSE
-------------------------------------------------------------------------------
--
1.8.5.3
^ permalink raw reply related
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Alexander Aring @ 2014-10-13 10:34 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend
In-Reply-To: <1413194456-26351-2-git-send-email-martin.townsend@xsilon.com>
On Mon, Oct 13, 2014 at 11:00:56AM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> skb_cow which will check to see if there is enough headroom
> first to ensure it's only done if necessary and will handle
> alignment issues for cache.
> As skb_cow uses pskb_expand_head we ensure the skb isn't shared from
> bluetooth and ieee802.15.4 code that use the IPHC decompression.
>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
Okay, we don't use the goto drop; below and currently this will always
return -EINVAL. You will fix that in your next patches, so it doesn't
matter right now. Also this will correct a little bit the errno value.
Great job Martin.
Acked-by: Alexander Aring <alex.aring@gmail.com>
Marcel, please wait for jukka's acked here, then we are sure that we
didn't break anything in bluetooth 6lowpan.
- Alex
^ permalink raw reply
* [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 10:00 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
In-Reply-To: <1413194456-26351-1-git-send-email-martin.townsend@xsilon.com>
Currently there are potentially 2 skb_copy_expand calls in IPHC
decompression. This patch replaces this with one call to
skb_cow which will check to see if there is enough headroom
first to ensure it's only done if necessary and will handle
alignment issues for cache.
As skb_cow uses pskb_expand_head we ensure the skb isn't shared from
bluetooth and ieee802.15.4 code that use the IPHC decompression.
Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
net/6lowpan/iphc.c | 47 +++++++++++++++++++++--------------------------
net/bluetooth/6lowpan.c | 4 ++++
2 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..747b3cc 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
struct net_device *dev, skb_delivery_cb deliver_skb)
{
- struct sk_buff *new;
int stat;
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
+ skb_push(skb, sizeof(struct ipv6hdr));
+ skb_reset_network_header(skb);
+ skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
- new->protocol = htons(ETH_P_IPV6);
- new->pkt_type = PACKET_HOST;
- new->dev = dev;
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = dev;
raw_dump_table(__func__, "raw skb data dump before receiving",
- new->data, new->len);
+ skb->data, skb->len);
- stat = deliver_skb(new, dev);
+ stat = deliver_skb(skb, dev);
- kfree_skb(new);
+ consume_skb(skb);
return stat;
}
@@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* UDP data uncompression */
if (iphc0 & LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
+ const int needed = sizeof(struct udphdr) + sizeof(hdr);
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +460,11 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
- new = skb_copy_expand(skb, sizeof(struct udphdr),
- skb_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb = new;
+ err = skb_cow(skb, needed);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +474,12 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
(u8 *)&uh, sizeof(uh));
hdr.nexthdr = UIP_PROTO_UDP;
+ } else {
+ err = skb_cow(skb, sizeof(hdr));
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
}
hdr.payload_len = htons(skb->len);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index c2e0d14..4ebc806 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -316,6 +316,10 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
if (dev->type != ARPHRD_6LOWPAN)
goto drop;
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb)
+ goto drop;
+
/* check that it's our buffer */
if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
/* Copy the packet so that the IPv6 header is
--
1.9.1
^ permalink raw reply related
* [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 10:00 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
This patch aims to reduce the amount of copying in the receive path when
decompressing by checking for headroom and calling pskb_expand_head if required.
Another benefit of this is that the skb pointer passed to the decompression
routine will be the same skb after decompression. This will be a step towards
freeing the skb within the receive function and not within all the error paths
of the decompression routine.
Bluetooth and 802.15.4 have been changed to ensure that the skb isn't shared
before calling the decompression routine as this is a requirement of
pskb_expand_head.
v1 -> v2
Use const int for new headroom size.
Remove blank line.
Use skb_unshare instead of skb_copy_expand.
Use consume_skb.
v2 -> v3
Remove cases where we are trying to free a NULL skb.
v3 -> v4
Now uses skb_share_check to ensure the skb isn't shared before decompressing
v4 -> v5
Remove skb_share_check from 802.15.4 lowpan_rcv as it's already done at the
top of the function.
v5 -> v6
Use skb_cow instead of pskb_expand_head, adjusted commit message to reflect
this.
Moved skb_share_check to start of recv_pkt function for bluetooth.
Martin Townsend (1):
6lowpan: Use skb_cow in IPHC decompression.
net/6lowpan/iphc.c | 47 +++++++++++++++++++++--------------------------
net/bluetooth/6lowpan.c | 4 ++++
2 files changed, 25 insertions(+), 26 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH BlueZ 4/4] unit/test-avrcp: Add /TP/MCN/NP/BV-07-C test
From: Luiz Augusto von Dentz @ 2014-10-13 9:55 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413194150-21751-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Verify the NowPlayingContentChanged Notification issued by the TG.
---
android/avrcp-lib.h | 1 +
unit/test-avrcp.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
index 2299c83..8e6ee40 100644
--- a/android/avrcp-lib.h
+++ b/android/avrcp-lib.h
@@ -54,6 +54,7 @@
#define AVRCP_EVENT_TRACK_REACHED_START 0x04
#define AVRCP_EVENT_PLAYBACK_POS_CHANGED 0x05
#define AVRCP_EVENT_SETTINGS_CHANGED 0x08
+#define AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED 0x09
#define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a
#define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b
#define AVRCP_EVENT_UIDS_CHANGED 0x0c
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 4f6526c..b26c553 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -631,6 +631,23 @@ static int uids_changed(struct avrcp *session, uint8_t transaction,
return 0;
}
+static int now_playing_content_changed(struct avrcp *session,
+ uint8_t transaction, uint32_t interval,
+ void *user_data)
+{
+ DBG("");
+
+ avrcp_register_notification_rsp(session, transaction, AVC_CTYPE_INTERIM,
+ AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED,
+ NULL, 0);
+
+ avrcp_register_notification_rsp(session, transaction, AVC_CTYPE_CHANGED,
+ AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED,
+ NULL, 0);
+
+ return 0;
+}
+
static int register_notification(struct avrcp *session, uint8_t transaction,
uint8_t event, uint32_t interval,
void *user_data)
@@ -651,6 +668,9 @@ static int register_notification(struct avrcp *session, uint8_t transaction,
user_data);
case AVRCP_EVENT_UIDS_CHANGED:
return uids_changed(session, transaction, interval, user_data);
+ case AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED:
+ return now_playing_content_changed(session, transaction,
+ interval, user_data);
default:
return -EINVAL;
}
@@ -1404,6 +1424,19 @@ int main(int argc, char *argv[])
brs_pdu(0x02, 0x11, 0x0e, AVRCP_GET_FOLDER_ITEMS,
0x00, 0x05, 0x04, 0xab, 0xcd, 0x00, 0x00));
+ /* NowPlayingContentChanged Notification – TG */
+ define_test("/TP/MCN/NP/BV-07-C", test_server,
+ raw_pdu(0x00, 0x11, 0x0e, 0x03, 0x48, 0x00,
+ 0x00, 0x19, 0x58, AVRCP_REGISTER_NOTIFICATION,
+ 0x00, 0x00, 0x05, 0x09,
+ 0x00, 0x00, 0x00, 0x00),
+ frg_pdu(0x02, 0x11, 0x0e, AVC_CTYPE_INTERIM, 0x48, 0x00,
+ 0x00, 0x19, 0x58, AVRCP_REGISTER_NOTIFICATION,
+ 0x00, 0x00, 0x01, 0x09),
+ raw_pdu(0x02, 0x11, 0x0e, AVC_CTYPE_CHANGED, 0x48, 0x00,
+ 0x00, 0x19, 0x58, AVRCP_REGISTER_NOTIFICATION,
+ 0x00, 0x00, 0x01, 0x09));
+
/* GetItemAttributes - CT */
define_test("/TP/MCN/NP/BV-08-C", test_client,
brs_pdu(0x00, 0x11, 0x0e, AVRCP_GET_ITEM_ATTRIBUTES,
--
1.9.3
^ permalink raw reply related
* [PATCH BlueZ 3/4] android/avrcp-lib: Add checks for unknown events
From: Luiz Augusto von Dentz @ 2014-10-13 9:55 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413194150-21751-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Unknown events should be treated as protocol error since there is no
way to parse them properly and there is no support for vendor specific
either.
---
android/avrcp-lib.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index 60d6e97..a490d76 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -1779,6 +1779,11 @@ static gboolean register_notification_rsp(struct avctp *conn,
rsp = (void *) pdu->params;
event = rsp->event;
+ if (event > AVRCP_EVENT_LAST) {
+ err = -EPROTO;
+ goto done;
+ }
+
switch (event) {
case AVRCP_EVENT_STATUS_CHANGED:
case AVRCP_EVENT_VOLUME_CHANGED:
@@ -1843,6 +1848,9 @@ int avrcp_register_notification(struct avrcp *session, uint8_t event,
struct iovec iov;
struct register_notification_req req;
+ if (event > AVRCP_EVENT_LAST)
+ return -EINVAL;
+
req.event = event;
put_be32(interval, &req.interval);
--
1.9.3
^ permalink raw reply related
* [PATCH BlueZ 2/4] android/avrcp-lib: Rework callback return
From: Luiz Augusto von Dentz @ 2014-10-13 9:55 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413194150-21751-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This allow callbacks to return 0 instead of -EAGAIN when responding
asynchronously which is more consistent since it is not an error.
---
android/avrcp-lib.c | 12 +++++++++++-
android/avrcp.c | 8 ++++----
unit/test-avrcp.c | 42 +++++++++++++++++++++---------------------
3 files changed, 36 insertions(+), 26 deletions(-)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index f3cdab9..60d6e97 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -496,6 +496,9 @@ static ssize_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
ret = handler->func(session, transaction, pdu->params_len, pdu->params,
session->control_data);
+ if (ret == 0)
+ return -EAGAIN;
+
if (ret < 0) {
if (ret == -EAGAIN)
return ret;
@@ -622,6 +625,9 @@ static ssize_t handle_browsing_pdu(struct avctp *conn,
ret = handler->func(session, transaction, pdu->params_len, pdu->params,
session->control_data);
+ if (ret == 0)
+ return -EAGAIN;
+
if (ret < 0) {
if (ret == -EAGAIN)
return ret;
@@ -1171,7 +1177,7 @@ static ssize_t request_continuing(struct avrcp *session, uint8_t transaction,
if (err < 0)
return -EINVAL;
- return -EAGAIN;
+ return 0;
}
static ssize_t abort_continuing(struct avrcp *session, uint8_t transaction,
@@ -1186,6 +1192,10 @@ static ssize_t abort_continuing(struct avrcp *session, uint8_t transaction,
continuing_free(session->continuing);
session->continuing = NULL;
+ avrcp_send_internal(session, transaction, AVC_CTYPE_ACCEPTED,
+ AVC_SUBUNIT_PANEL, AVRCP_ABORT_CONTINUING,
+ AVRCP_PACKET_TYPE_SINGLE, NULL, 0);
+
return 0;
}
diff --git a/android/avrcp.c b/android/avrcp.c
index 940de0b..a0d412d 100644
--- a/android/avrcp.c
+++ b/android/avrcp.c
@@ -642,7 +642,7 @@ static int handle_get_capabilities_cmd(struct avrcp *session,
avrcp_get_capabilities_rsp(session, transaction, sizeof(events),
events);
- return -EAGAIN;
+ return 0;
}
static void push_request(struct avrcp_device *dev, uint8_t pdu_id,
@@ -671,7 +671,7 @@ static int handle_get_play_status_cmd(struct avrcp *session,
push_request(dev, AVRCP_GET_PLAY_STATUS, 0, transaction);
- return -EAGAIN;
+ return 0;
}
static int handle_get_element_attrs_cmd(struct avrcp *session,
@@ -707,7 +707,7 @@ done:
push_request(dev, AVRCP_GET_ELEMENT_ATTRIBUTES, 0, transaction);
- return -EAGAIN;
+ return 0;
}
@@ -741,7 +741,7 @@ static int handle_register_notification_cmd(struct avrcp *session,
push_request(dev, AVRCP_REGISTER_NOTIFICATION, event, transaction);
- return -EAGAIN;
+ return 0;
}
static const struct avrcp_control_ind control_ind = {
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index a6a1872..4f6526c 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -409,7 +409,7 @@ static int list_attributes(struct avrcp *session, uint8_t transaction,
avrcp_list_player_attributes_rsp(session, transaction, 0, NULL);
- return -EAGAIN;
+ return 0;
}
static int get_attribute_text(struct avrcp *session, uint8_t transaction,
@@ -428,7 +428,7 @@ static int get_attribute_text(struct avrcp *session, uint8_t transaction,
avrcp_get_player_attribute_text_rsp(session, transaction, number, attrs,
text);
- return -EAGAIN;
+ return 0;
}
static int list_values(struct avrcp *session, uint8_t transaction,
@@ -472,7 +472,7 @@ static int get_value(struct avrcp *session, uint8_t transaction,
avrcp_get_current_player_value_rsp(session, transaction, number, attrs,
values);
- return -EAGAIN;
+ return 0;
}
static int set_value(struct avrcp *session, uint8_t transaction,
@@ -483,7 +483,7 @@ static int set_value(struct avrcp *session, uint8_t transaction,
avrcp_set_player_value_rsp(session, transaction);
- return -EAGAIN;
+ return 0;
}
static int get_play_status(struct avrcp *session, uint8_t transaction,
@@ -494,7 +494,7 @@ static int get_play_status(struct avrcp *session, uint8_t transaction,
avrcp_get_play_status_rsp(session, transaction, 0xaaaaaaaa, 0xbbbbbbbb,
0x00);
- return -EAGAIN;
+ return 0;
}
static int get_element_attributes(struct avrcp *session, uint8_t transaction,
@@ -516,7 +516,7 @@ static int get_element_attributes(struct avrcp *session, uint8_t transaction,
} else
avrcp_get_element_attrs_rsp(session, transaction, NULL, 0);
- return -EAGAIN;
+ return 0;
}
static int track_changed(struct avrcp *session, uint8_t transaction,
@@ -541,7 +541,7 @@ static int track_changed(struct avrcp *session, uint8_t transaction,
AVRCP_EVENT_TRACK_CHANGED,
&track, sizeof(track));
- return -EAGAIN;
+ return 0;
}
static int settings_changed(struct avrcp *session, uint8_t transaction,
@@ -563,7 +563,7 @@ static int settings_changed(struct avrcp *session, uint8_t transaction,
AVRCP_EVENT_SETTINGS_CHANGED,
settings, sizeof(settings));
- return -EAGAIN;
+ return 0;
}
static int available_players_changed(struct avrcp *session, uint8_t transaction,
@@ -579,7 +579,7 @@ static int available_players_changed(struct avrcp *session, uint8_t transaction,
AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED,
NULL, 0);
- return -EAGAIN;
+ return 0;
}
static int addressed_player_changed(struct avrcp *session, uint8_t transaction,
@@ -600,7 +600,7 @@ static int addressed_player_changed(struct avrcp *session, uint8_t transaction,
AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED,
player, sizeof(player));
- return -EAGAIN;
+ return 0;
}
static int uids_changed(struct avrcp *session, uint8_t transaction,
@@ -622,13 +622,13 @@ static int uids_changed(struct avrcp *session, uint8_t transaction,
if (!g_str_equal(context->data->test_name, "/TP/MCN/CB/BV-11-C") &&
!g_str_equal(context->data->test_name, "/TP/MCN/CB/BI-05-C"))
- return -EAGAIN;
+ return 0;
avrcp_register_notification_rsp(session, transaction, AVC_CTYPE_CHANGED,
AVRCP_EVENT_UIDS_CHANGED,
&counter, sizeof(counter));
- return -EAGAIN;
+ return 0;
}
static int register_notification(struct avrcp *session, uint8_t transaction,
@@ -663,7 +663,7 @@ static int set_volume(struct avrcp *session, uint8_t transaction,
avrcp_set_volume_rsp(session, transaction, volume);
- return -EAGAIN;
+ return 0;
}
static int set_addressed(struct avrcp *session, uint8_t transaction,
@@ -681,7 +681,7 @@ static int set_addressed(struct avrcp *session, uint8_t transaction,
avrcp_set_addressed_player_rsp(session, transaction, status);
- return -EAGAIN;
+ return 0;
}
static int set_browsed(struct avrcp *session, uint8_t transaction,
@@ -701,7 +701,7 @@ static int set_browsed(struct avrcp *session, uint8_t transaction,
AVRCP_STATUS_SUCCESS,
0xabcd, 0, 1, folders);
- return -EAGAIN;
+ return 0;
}
static int get_folder_items(struct avrcp *session, uint8_t transaction,
@@ -722,7 +722,7 @@ static int get_folder_items(struct avrcp *session, uint8_t transaction,
avrcp_get_folder_items_rsp(session, transaction, AVRCP_STATUS_SUCCESS,
0xabcd, 0, NULL, NULL, NULL);
- return -EAGAIN;
+ return 0;
}
static int change_path(struct avrcp *session, uint8_t transaction,
@@ -736,7 +736,7 @@ static int change_path(struct avrcp *session, uint8_t transaction,
avrcp_change_path_rsp(session, transaction, AVRCP_STATUS_SUCCESS, 0);
- return -EAGAIN;
+ return 0;
}
static int get_item_attributes(struct avrcp *session, uint8_t transaction,
@@ -757,7 +757,7 @@ static int get_item_attributes(struct avrcp *session, uint8_t transaction,
avrcp_get_item_attributes_rsp(session, transaction, status, 0, NULL,
NULL);
- return -EAGAIN;
+ return 0;
}
static int play_item(struct avrcp *session, uint8_t transaction, uint8_t scope,
@@ -770,7 +770,7 @@ static int play_item(struct avrcp *session, uint8_t transaction, uint8_t scope,
avrcp_play_item_rsp(session, transaction, AVRCP_STATUS_SUCCESS);
- return -EAGAIN;
+ return 0;
}
static int search(struct avrcp *session, uint8_t transaction,
@@ -780,7 +780,7 @@ static int search(struct avrcp *session, uint8_t transaction,
avrcp_search_rsp(session, transaction, AVRCP_STATUS_SUCCESS, 0xaabb, 0);
- return -EAGAIN;
+ return 0;
}
static int add_to_now_playing(struct avrcp *session, uint8_t transaction,
@@ -795,7 +795,7 @@ static int add_to_now_playing(struct avrcp *session, uint8_t transaction,
avrcp_add_to_now_playing_rsp(session, transaction,
AVRCP_STATUS_SUCCESS);
- return -EAGAIN;
+ return 0;
}
static const struct avrcp_control_ind control_ind = {
--
1.9.3
^ permalink raw reply related
* [PATCH BlueZ 1/4] android/avrcp-lib: Add status parameter
From: Luiz Augusto von Dentz @ 2014-10-13 9:55 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds status parameter for functions generating browsing reponses
as it may not be possible to respond synchronously using the return of
the callback while processing the request.
---
android/avrcp-lib.c | 91 ++++++++++++++++++++++++++++-------------------------
android/avrcp-lib.h | 16 +++++-----
unit/test-avrcp.c | 13 ++++----
3 files changed, 64 insertions(+), 56 deletions(-)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index b7d0f5e..f3cdab9 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -3337,6 +3337,20 @@ int avrcp_set_addressed_player_rsp(struct avrcp *session, uint8_t transaction,
&iov, 1);
}
+static int avrcp_status_rsp(struct avrcp *session, uint8_t transaction,
+ uint8_t pdu_id, uint8_t status)
+{
+ struct iovec iov;
+
+ if (status > AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED)
+ return -EINVAL;
+
+ iov.iov_base = &status;
+ iov.iov_len = sizeof(status);
+
+ return avrcp_send_browsing(session, transaction, pdu_id, &iov, 1);
+}
+
int avrcp_set_browsed_player_rsp(struct avrcp *session, uint8_t transaction,
uint8_t status, uint16_t counter,
uint32_t items, uint8_t depth,
@@ -3347,13 +3361,9 @@ int avrcp_set_browsed_player_rsp(struct avrcp *session, uint8_t transaction,
uint16_t len[UINT8_MAX];
int i;
- if (status != AVRCP_STATUS_SUCCESS) {
- iov[0].iov_base = &status;
- iov[0].iov_len = sizeof(status);
- return avrcp_send_browsing(session, transaction,
- AVRCP_SET_BROWSED_PLAYER,
- iov, 1);
- }
+ if (status != AVRCP_STATUS_SUCCESS)
+ return avrcp_status_rsp(session, transaction,
+ AVRCP_SET_BROWSED_PLAYER, status);
rsp.status = status;
put_be16(counter, &rsp.counter);
@@ -3390,16 +3400,20 @@ int avrcp_set_browsed_player_rsp(struct avrcp *session, uint8_t transaction,
}
int avrcp_get_folder_items_rsp(struct avrcp *session, uint8_t transaction,
- uint16_t counter, uint8_t number,
- uint8_t *type, uint16_t *len,
- uint8_t **params)
+ uint8_t status, uint16_t counter,
+ uint8_t number, uint8_t *type,
+ uint16_t *len, uint8_t **params)
{
struct iovec iov[UINT8_MAX * 2 + 1];
struct get_folder_items_rsp rsp;
uint8_t item[UINT8_MAX][3];
int i;
- rsp.status = AVRCP_STATUS_SUCCESS;
+ if (status != AVRCP_STATUS_SUCCESS)
+ return avrcp_status_rsp(session, transaction,
+ AVRCP_GET_FOLDER_ITEMS, status);
+
+ rsp.status = status;
put_be16(counter, &rsp.counter);
put_be16(number, &rsp.number);
@@ -3422,12 +3436,16 @@ int avrcp_get_folder_items_rsp(struct avrcp *session, uint8_t transaction,
}
int avrcp_change_path_rsp(struct avrcp *session, uint8_t transaction,
- uint32_t items)
+ uint8_t status, uint32_t items)
{
struct iovec iov;
struct change_path_rsp rsp;
- rsp.status = AVRCP_STATUS_SUCCESS;
+ if (status != AVRCP_STATUS_SUCCESS)
+ return avrcp_status_rsp(session, transaction, AVRCP_CHANGE_PATH,
+ status);
+
+ rsp.status = status;
put_be32(items, &rsp.items);
iov.iov_base = &rsp;
@@ -3477,12 +3495,9 @@ int avrcp_get_item_attributes_rsp(struct avrcp *session, uint8_t transaction,
if (number > AVRCP_MEDIA_ATTRIBUTE_LAST)
return -EINVAL;
- if (status != AVRCP_STATUS_SUCCESS) {
- iov[0].iov_base = &status;
- iov[0].iov_len = sizeof(status);
- return avrcp_send_browsing(session, transaction,
- AVRCP_GET_ITEM_ATTRIBUTES, iov, 1);
- }
+ if (status != AVRCP_STATUS_SUCCESS)
+ return avrcp_status_rsp(session, transaction,
+ AVRCP_GET_ITEM_ATTRIBUTES, status);
rsp.status = status;
rsp.number = number;
@@ -3498,27 +3513,24 @@ int avrcp_get_item_attributes_rsp(struct avrcp *session, uint8_t transaction,
number * 2 + 1);
}
-int avrcp_play_item_rsp(struct avrcp *session, uint8_t transaction)
+int avrcp_play_item_rsp(struct avrcp *session, uint8_t transaction,
+ uint8_t status)
{
- struct iovec iov;
- struct play_item_rsp rsp;
-
- rsp.status = AVRCP_STATUS_SUCCESS;
-
- iov.iov_base = &rsp;
- iov.iov_len = sizeof(rsp);
-
- return avrcp_send_browsing(session, transaction, AVRCP_PLAY_ITEM,
- &iov, 1);
+ return avrcp_status_rsp(session, transaction, AVRCP_PLAY_ITEM,
+ status);
}
-int avrcp_search_rsp(struct avrcp *session, uint8_t transaction,
+int avrcp_search_rsp(struct avrcp *session, uint8_t transaction, uint8_t status,
uint16_t counter, uint32_t items)
{
struct iovec iov;
struct search_rsp rsp;
- rsp.status = AVRCP_STATUS_SUCCESS;
+ if (status != AVRCP_STATUS_SUCCESS)
+ return avrcp_status_rsp(session, transaction, AVRCP_SEARCH,
+ status);
+
+ rsp.status = status;
put_be16(counter, &rsp.counter);
put_be32(items, &rsp.items);
@@ -3529,18 +3541,11 @@ int avrcp_search_rsp(struct avrcp *session, uint8_t transaction,
&iov, 1);
}
-int avrcp_add_to_now_playing_rsp(struct avrcp *session, uint8_t transaction)
+int avrcp_add_to_now_playing_rsp(struct avrcp *session, uint8_t transaction,
+ uint8_t status)
{
- struct iovec iov;
- struct add_to_now_playing_rsp rsp;
-
- rsp.status = AVRCP_STATUS_SUCCESS;
-
- iov.iov_base = &rsp;
- iov.iov_len = sizeof(rsp);
-
- return avrcp_send_browsing(session, transaction,
- AVRCP_ADD_TO_NOW_PLAYING, &iov, 1);
+ return avrcp_status_rsp(session, transaction, AVRCP_ADD_TO_NOW_PLAYING,
+ status);
}
int avrcp_send_passthrough(struct avrcp *session, uint32_t vendor, uint8_t op)
diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
index 9985c4a..2299c83 100644
--- a/android/avrcp-lib.h
+++ b/android/avrcp-lib.h
@@ -327,17 +327,19 @@ int avrcp_set_browsed_player_rsp(struct avrcp *session, uint8_t transaction,
uint32_t items, uint8_t depth,
const char **folders);
int avrcp_get_folder_items_rsp(struct avrcp *session, uint8_t transaction,
- uint16_t counter, uint8_t number,
- uint8_t *type, uint16_t *len,
- uint8_t **params);
+ uint8_t status, uint16_t counter,
+ uint8_t number, uint8_t *type,
+ uint16_t *len, uint8_t **params);
int avrcp_change_path_rsp(struct avrcp *session, uint8_t transaction,
- uint32_t items);
+ uint8_t status, uint32_t items);
int avrcp_get_item_attributes_rsp(struct avrcp *session, uint8_t transaction,
uint8_t status, uint8_t number,
uint32_t *attrs, const char **text);
-int avrcp_play_item_rsp(struct avrcp *session, uint8_t transaction);
-int avrcp_search_rsp(struct avrcp *session, uint8_t transaction,
+int avrcp_play_item_rsp(struct avrcp *session, uint8_t transaction,
+ uint8_t status);
+int avrcp_search_rsp(struct avrcp *session, uint8_t transaction, uint8_t status,
uint16_t counter, uint32_t items);
-int avrcp_add_to_now_playing_rsp(struct avrcp *session, uint8_t transaction);
+int avrcp_add_to_now_playing_rsp(struct avrcp *session, uint8_t transaction,
+ uint8_t status);
int avrcp_send_passthrough(struct avrcp *session, uint32_t vendor, uint8_t op);
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 154175e..a6a1872 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -719,8 +719,8 @@ static int get_folder_items(struct avrcp *session, uint8_t transaction,
if (start > 1)
return -ERANGE;
- avrcp_get_folder_items_rsp(session, transaction, 0xabcd, 0, NULL, NULL,
- NULL);
+ avrcp_get_folder_items_rsp(session, transaction, AVRCP_STATUS_SUCCESS,
+ 0xabcd, 0, NULL, NULL, NULL);
return -EAGAIN;
}
@@ -734,7 +734,7 @@ static int change_path(struct avrcp *session, uint8_t transaction,
if (!uid)
return -ENOTDIR;
- avrcp_change_path_rsp(session, transaction, 0);
+ avrcp_change_path_rsp(session, transaction, AVRCP_STATUS_SUCCESS, 0);
return -EAGAIN;
}
@@ -768,7 +768,7 @@ static int play_item(struct avrcp *session, uint8_t transaction, uint8_t scope,
if (!uid)
return -ENOENT;
- avrcp_play_item_rsp(session, transaction);
+ avrcp_play_item_rsp(session, transaction, AVRCP_STATUS_SUCCESS);
return -EAGAIN;
}
@@ -778,7 +778,7 @@ static int search(struct avrcp *session, uint8_t transaction,
{
DBG("");
- avrcp_search_rsp(session, transaction, 0xaabb, 0);
+ avrcp_search_rsp(session, transaction, AVRCP_STATUS_SUCCESS, 0xaabb, 0);
return -EAGAIN;
}
@@ -792,7 +792,8 @@ static int add_to_now_playing(struct avrcp *session, uint8_t transaction,
if (!uid)
return -ENOENT;
- avrcp_add_to_now_playing_rsp(session, transaction);
+ avrcp_add_to_now_playing_rsp(session, transaction,
+ AVRCP_STATUS_SUCCESS);
return -EAGAIN;
}
--
1.9.3
^ permalink raw reply related
* [PATCH v3 2/2] Bluetooth: Improve RFCOMM __test_pf macro robustness
From: Szymon Janc @ 2014-10-13 9:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1413193434-16779-1-git-send-email-szymon.janc@tieto.com>
Value returned by this macro might be used as bit value so it should
return either 0 or 1 to avoid possible bugs (similar to NSC bug)
when shifting it.
Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
net/bluetooth/rfcomm/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index d0bbc73..bce9c3d 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -79,7 +79,7 @@ static struct rfcomm_session *rfcomm_session_del(struct rfcomm_session *s);
#define __test_ea(b) ((b & 0x01))
#define __test_cr(b) (!!(b & 0x02))
-#define __test_pf(b) ((b & 0x10))
+#define __test_pf(b) (!!(b & 0x10))
#define __addr(cr, dlci) (((dlci & 0x3f) << 2) | (cr << 1) | 0x01)
#define __ctrl(type, pf) (((type & 0xef) | (pf << 4)))
--
1.9.1
^ permalink raw reply related
* [PATCH v3 1/2] Bluetooth: Fix RFCOMM NSC response
From: Szymon Janc @ 2014-10-13 9:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
rfcomm_send_nsc expects CR to be either 0 or 1 since it is later
passed to __mcc_type macro and shitfed. Unfortunatelly CR extracted
from received frame type was not sanitized and shifted value was passed
resulting in bogus response.
Note: shifted value was also passed to other functions but was used
only in if satements so this bug appears only for NSC case.
The CR bit in the value octet shall be set to the same value
as the CR bit in the type field octet of the not supported command
frame but the CR bit for NCS response should be set to 0 since it is
always a response.
This was affecting TC_RFC_BV_25_C PTS qualification test.
Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
V3: fixed invalid CR
V2: moved sanitization to macro ifself
added second patch that also fix __test_pf
net/bluetooth/rfcomm/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index af73bc3..d0bbc73 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -78,7 +78,7 @@ static struct rfcomm_session *rfcomm_session_del(struct rfcomm_session *s);
#define __get_type(b) ((b & 0xef))
#define __test_ea(b) ((b & 0x01))
-#define __test_cr(b) ((b & 0x02))
+#define __test_cr(b) (!!(b & 0x02))
#define __test_pf(b) ((b & 0x10))
#define __addr(cr, dlci) (((dlci & 0x3f) << 2) | (cr << 1) | 0x01)
@@ -904,7 +904,7 @@ static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type)
hdr->len = __len8(sizeof(*mcc) + 1);
mcc = (void *) ptr; ptr += sizeof(*mcc);
- mcc->type = __mcc_type(cr, RFCOMM_NSC);
+ mcc->type = __mcc_type(0, RFCOMM_NSC);
mcc->len = __len8(1);
/* Type that we didn't like */
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-13 9:41 UTC (permalink / raw)
To: Martin Townsend
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel,
jukka.rissanen
In-Reply-To: <543B9B4C.1010303@xsilon.com>
On Mon, Oct 13, 2014 at 10:28:44AM +0100, Martin Townsend wrote:
...
> >> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
> > For me it's confusing, you do that on skb, but we never used skb
> > afterwards (maybe on freeing) but we use local_skb, the local_skb should
> > always be cloned because we running skb_clone before.
> skb_clone does ensure that the skb isn't shared anymore so I think it will be safe to remove the skb_share_check. Or I can move it to the start of the function like we do for 802.15.4?
We need to clarify the following points:
skb_is_shared:
It's used twice in some packet handler functions or elsewhere.
now a skb_clone:
A skb clone is only a copy of struct sk_buff, so we can safely run
skb_pull and skb_push, skb->dev = foo. Simple for modify skb attributes.
now a skb_copy:
It's a complete private copy with private data buffer which allow us to
make skb->data[#] = foobar;
Now skb_share_check do:
if skb_is_shared true, then make a clone. This doesn't allow to
manipulate the data, but skb_cow make a copy of the private data so this
is okay for IPHC call. Means after skb_cow, we have something like
skb_copy, but skb_cow should be faster. ;-)
Also skb_share_check checks if skb_is_shared is false, then we don't
need a skb_clone, because it's not shared, then we are allow to
manipulate skb attributes, because nobody else use it.
I would add skb_share_check at beginning of this function like 802.15.4
but the IPV6 DISPATCH of bluetooth do a complete copy of the data buffer
which seems not necessary, but this is another issue and maybe I think
that the IPV6 DISPATCH value isn't needed by bluetooth 6lowpan. <-- But I
am not sure about this. I also can't see that bluetooth 6lowpan runs
skb_pull for the one byte dispatch value. Jukka need to check that, if
he has time and feel like to doing it.
- Alex
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Martin Townsend @ 2014-10-13 9:28 UTC (permalink / raw)
To: Alexander Aring
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel,
jukka.rissanen
In-Reply-To: <20141013085538.GA20544@omega>
Hi Alex,
On 13/10/14 09:55, Alexander Aring wrote:
> Hi Martin,
>
> On Mon, Oct 13, 2014 at 09:44:42AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/10/14 08:36, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> This is only a summarize what we should now to try to do here for next
>>> version.
>>>
>>> I know... this patch ends in a global debate on principles how we deal
>>> with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
>>> more a hack.
>>>
>>> On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
>>>> Currently there are potentially 2 skb_copy_expand calls in IPHC
>>>> decompression. This patch replaces this with one call to
>>>> pskb_expand_head. It also checks to see if there is enough headroom
>>>> first to ensure it's only done if necessary.
>>>> As pskb_expand_head must only have one reference the calling code
>>>> now ensures this.
>>>>
>>>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>>>> ---
>>>> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
>>>> net/bluetooth/6lowpan.c | 7 +++++++
>>>> 2 files changed, 32 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>> index 142eef5..853b4b8 100644
>>>> --- a/net/6lowpan/iphc.c
>>>> +++ b/net/6lowpan/iphc.c
>>>> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
>>>> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>>>> struct net_device *dev, skb_delivery_cb deliver_skb)
>>>> {
>>>> - struct sk_buff *new;
>>>> int stat;
>>>>
>>>> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
>>>> - GFP_ATOMIC);
>>>> - kfree_skb(skb);
>>>> -
>>>> - if (!new)
>>>> - return -ENOMEM;
>>>> -
>>>> - skb_push(new, sizeof(struct ipv6hdr));
>>>> - skb_reset_network_header(new);
>>>> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
>>>> + skb_push(skb, sizeof(struct ipv6hdr));
>>>> + skb_reset_network_header(skb);
>>>> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>>>>
>>>> - new->protocol = htons(ETH_P_IPV6);
>>>> - new->pkt_type = PACKET_HOST;
>>>> - new->dev = dev;
>>>> + skb->protocol = htons(ETH_P_IPV6);
>>>> + skb->pkt_type = PACKET_HOST;
>>>> + skb->dev = dev;
>>>>
>>>> raw_dump_table(__func__, "raw skb data dump before receiving",
>>>> - new->data, new->len);
>>>> + skb->data, skb->len);
>>>>
>>>> - stat = deliver_skb(new, dev);
>>>> + stat = deliver_skb(skb, dev);
>>>>
>>>> - kfree_skb(new);
>>>> + consume_skb(skb);
>>>>
>>>> return stat;
>>>> }
>>>> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> /* UDP data uncompression */
>>>> if (iphc0 & LOWPAN_IPHC_NH_C) {
>>>> struct udphdr uh;
>>>> - struct sk_buff *new;
>>>> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>>>>
>>>> if (uncompress_udp_header(skb, &uh))
>>>> goto drop;
>>>> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> /* replace the compressed UDP head by the uncompressed UDP
>>>> * header
>>>> */
>>>> - new = skb_copy_expand(skb, sizeof(struct udphdr),
>>>> - skb_tailroom(skb), GFP_ATOMIC);
>>>> - kfree_skb(skb);
>>>> -
>>>> - if (!new)
>>>> - return -ENOMEM;
>>>> -
>>>> - skb = new;
>>>> + if (skb_headroom(skb) < needed) {
>>>> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
>>>> + if (unlikely(err)) {
>>>> + kfree_skb(skb);
>>>> + return err;
>>>> + }
>>>> + }
>>> use skb_cow_head here.
>> I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
>>
>> skb_cow seems a good fit to me though.
> okay, then use skb_cow here. I will remember that we could possible
> maybe use "skb_cow_head" here. But yea I mean we running lot of pulls
> and move the head pointer, then push now data on it, this would
> overwrite the pulled data and other skb's may not pull here. If the data
> is shared there, this would crash.
>
>>>>
>>>> skb_push(skb, sizeof(struct udphdr));
>>>> skb_reset_transport_header(skb);
>>>> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> (u8 *)&uh, sizeof(uh));
>>>>
>>>> hdr.nexthdr = UIP_PROTO_UDP;
>>>> + } else {
>>>> + if (skb_headroom(skb) < sizeof(hdr)) {
>>>> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>>>> + if (unlikely(err)) {
>>>> + kfree_skb(skb);
>>>> + return err;
>>>> + }
>>>> + }
>>> same here.
>>>
>>>> }
>>>>
>>>> hdr.payload_len = htons(skb->len);
>>>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>>>> index c2e0d14..6643a7c 100644
>>>> --- a/net/bluetooth/6lowpan.c
>>>> +++ b/net/bluetooth/6lowpan.c
>>>> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>>> kfree_skb(local_skb);
>>>> kfree_skb(skb);
>>>> } else {
>>>> + /* Decompression may use pskb_expand_head so no shared skb's */
>>>> + skb = skb_share_check(skb, GFP_ATOMIC);
>>>> + if (!skb) {
>>>> + dev->stats.rx_dropped++;
>>>> + return NET_RX_DROP;
>>>> + }
>>>> +
>>> I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
>>> skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
>>> is the share_check really needed? In my opinion there are several things
>>> wrong.
>>>
>>> What bluetooth should do here is:
>>>
>>> Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
>>>
>>> In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
>>>
>>> - only run a skb_pull (doesn't change the data buffer, only skb attribute)
>>> for the one byte dispatch.
>>> - run other things like skb_reset_network_header, etc...
>>>
>>> -> Currently this works because we run a complete skb_copy_expand here.
>>> But this is not needed, we need a clone because we don't modify the
>>> data buffer.
>>>
>>> -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
>>> What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
>>> 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
>>> for this here.
>>>
>>>
>>>
>>> In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
>>> this is already handled by "skb_share_check" then.
>> Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
>>
>> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
> For me it's confusing, you do that on skb, but we never used skb
> afterwards (maybe on freeing) but we use local_skb, the local_skb should
> always be cloned because we running skb_clone before.
skb_clone does ensure that the skb isn't shared anymore so I think it will be safe to remove the skb_share_check. Or I can move it to the start of the function like we do for 802.15.4?
>
> skb_share_check is a function which checks is the skb shared, then clone
> it. But we don't do anything with skb afterwards.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-Martin.
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-13 8:55 UTC (permalink / raw)
To: Martin Townsend
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel,
jukka.rissanen
In-Reply-To: <543B90FA.2060603@xsilon.com>
Hi Martin,
On Mon, Oct 13, 2014 at 09:44:42AM +0100, Martin Townsend wrote:
> Hi Alex,
>
> On 11/10/14 08:36, Alexander Aring wrote:
> > Hi Martin,
> >
> > This is only a summarize what we should now to try to do here for next
> > version.
> >
> > I know... this patch ends in a global debate on principles how we deal
> > with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> > more a hack.
> >
> > On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
> >> Currently there are potentially 2 skb_copy_expand calls in IPHC
> >> decompression. This patch replaces this with one call to
> >> pskb_expand_head. It also checks to see if there is enough headroom
> >> first to ensure it's only done if necessary.
> >> As pskb_expand_head must only have one reference the calling code
> >> now ensures this.
> >>
> >> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >> ---
> >> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> >> net/bluetooth/6lowpan.c | 7 +++++++
> >> 2 files changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> >> index 142eef5..853b4b8 100644
> >> --- a/net/6lowpan/iphc.c
> >> +++ b/net/6lowpan/iphc.c
> >> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
> >> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> >> struct net_device *dev, skb_delivery_cb deliver_skb)
> >> {
> >> - struct sk_buff *new;
> >> int stat;
> >>
> >> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
> >> - GFP_ATOMIC);
> >> - kfree_skb(skb);
> >> -
> >> - if (!new)
> >> - return -ENOMEM;
> >> -
> >> - skb_push(new, sizeof(struct ipv6hdr));
> >> - skb_reset_network_header(new);
> >> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> >> + skb_push(skb, sizeof(struct ipv6hdr));
> >> + skb_reset_network_header(skb);
> >> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
> >>
> >> - new->protocol = htons(ETH_P_IPV6);
> >> - new->pkt_type = PACKET_HOST;
> >> - new->dev = dev;
> >> + skb->protocol = htons(ETH_P_IPV6);
> >> + skb->pkt_type = PACKET_HOST;
> >> + skb->dev = dev;
> >>
> >> raw_dump_table(__func__, "raw skb data dump before receiving",
> >> - new->data, new->len);
> >> + skb->data, skb->len);
> >>
> >> - stat = deliver_skb(new, dev);
> >> + stat = deliver_skb(skb, dev);
> >>
> >> - kfree_skb(new);
> >> + consume_skb(skb);
> >>
> >> return stat;
> >> }
> >> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> /* UDP data uncompression */
> >> if (iphc0 & LOWPAN_IPHC_NH_C) {
> >> struct udphdr uh;
> >> - struct sk_buff *new;
> >> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
> >>
> >> if (uncompress_udp_header(skb, &uh))
> >> goto drop;
> >> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> /* replace the compressed UDP head by the uncompressed UDP
> >> * header
> >> */
> >> - new = skb_copy_expand(skb, sizeof(struct udphdr),
> >> - skb_tailroom(skb), GFP_ATOMIC);
> >> - kfree_skb(skb);
> >> -
> >> - if (!new)
> >> - return -ENOMEM;
> >> -
> >> - skb = new;
> >> + if (skb_headroom(skb) < needed) {
> >> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
> >> + if (unlikely(err)) {
> >> + kfree_skb(skb);
> >> + return err;
> >> + }
> >> + }
> > use skb_cow_head here.
> I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
>
> skb_cow seems a good fit to me though.
okay, then use skb_cow here. I will remember that we could possible
maybe use "skb_cow_head" here. But yea I mean we running lot of pulls
and move the head pointer, then push now data on it, this would
overwrite the pulled data and other skb's may not pull here. If the data
is shared there, this would crash.
> >
> >>
> >> skb_push(skb, sizeof(struct udphdr));
> >> skb_reset_transport_header(skb);
> >> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> (u8 *)&uh, sizeof(uh));
> >>
> >> hdr.nexthdr = UIP_PROTO_UDP;
> >> + } else {
> >> + if (skb_headroom(skb) < sizeof(hdr)) {
> >> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> >> + if (unlikely(err)) {
> >> + kfree_skb(skb);
> >> + return err;
> >> + }
> >> + }
> > same here.
> >
> >> }
> >>
> >> hdr.payload_len = htons(skb->len);
> >> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> >> index c2e0d14..6643a7c 100644
> >> --- a/net/bluetooth/6lowpan.c
> >> +++ b/net/bluetooth/6lowpan.c
> >> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >> kfree_skb(local_skb);
> >> kfree_skb(skb);
> >> } else {
> >> + /* Decompression may use pskb_expand_head so no shared skb's */
> >> + skb = skb_share_check(skb, GFP_ATOMIC);
> >> + if (!skb) {
> >> + dev->stats.rx_dropped++;
> >> + return NET_RX_DROP;
> >> + }
> >> +
> > I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> > skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> > is the share_check really needed? In my opinion there are several things
> > wrong.
> >
> > What bluetooth should do here is:
> >
> > Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
> >
> > In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
> >
> > - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> > for the one byte dispatch.
> > - run other things like skb_reset_network_header, etc...
> >
> > -> Currently this works because we run a complete skb_copy_expand here.
> > But this is not needed, we need a clone because we don't modify the
> > data buffer.
> >
> > -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> > What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> > 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> > for this here.
> >
> >
> >
> > In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> > this is already handled by "skb_share_check" then.
> Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
>
> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
For me it's confusing, you do that on skb, but we never used skb
afterwards (maybe on freeing) but we use local_skb, the local_skb should
always be cloned because we running skb_clone before.
skb_share_check is a function which checks is the skb shared, then clone
it. But we don't do anything with skb afterwards.
- Alex
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Martin Townsend @ 2014-10-13 8:44 UTC (permalink / raw)
To: Alexander Aring, Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen
In-Reply-To: <20141011073620.GA28500@omega>
Hi Alex,
On 11/10/14 08:36, Alexander Aring wrote:
> Hi Martin,
>
> This is only a summarize what we should now to try to do here for next
> version.
>
> I know... this patch ends in a global debate on principles how we deal
> with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> more a hack.
>
> On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
>> Currently there are potentially 2 skb_copy_expand calls in IPHC
>> decompression. This patch replaces this with one call to
>> pskb_expand_head. It also checks to see if there is enough headroom
>> first to ensure it's only done if necessary.
>> As pskb_expand_head must only have one reference the calling code
>> now ensures this.
>>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
>> net/bluetooth/6lowpan.c | 7 +++++++
>> 2 files changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index 142eef5..853b4b8 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
>> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>> struct net_device *dev, skb_delivery_cb deliver_skb)
>> {
>> - struct sk_buff *new;
>> int stat;
>>
>> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
>> - GFP_ATOMIC);
>> - kfree_skb(skb);
>> -
>> - if (!new)
>> - return -ENOMEM;
>> -
>> - skb_push(new, sizeof(struct ipv6hdr));
>> - skb_reset_network_header(new);
>> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
>> + skb_push(skb, sizeof(struct ipv6hdr));
>> + skb_reset_network_header(skb);
>> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>>
>> - new->protocol = htons(ETH_P_IPV6);
>> - new->pkt_type = PACKET_HOST;
>> - new->dev = dev;
>> + skb->protocol = htons(ETH_P_IPV6);
>> + skb->pkt_type = PACKET_HOST;
>> + skb->dev = dev;
>>
>> raw_dump_table(__func__, "raw skb data dump before receiving",
>> - new->data, new->len);
>> + skb->data, skb->len);
>>
>> - stat = deliver_skb(new, dev);
>> + stat = deliver_skb(skb, dev);
>>
>> - kfree_skb(new);
>> + consume_skb(skb);
>>
>> return stat;
>> }
>> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> /* UDP data uncompression */
>> if (iphc0 & LOWPAN_IPHC_NH_C) {
>> struct udphdr uh;
>> - struct sk_buff *new;
>> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>>
>> if (uncompress_udp_header(skb, &uh))
>> goto drop;
>> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> /* replace the compressed UDP head by the uncompressed UDP
>> * header
>> */
>> - new = skb_copy_expand(skb, sizeof(struct udphdr),
>> - skb_tailroom(skb), GFP_ATOMIC);
>> - kfree_skb(skb);
>> -
>> - if (!new)
>> - return -ENOMEM;
>> -
>> - skb = new;
>> + if (skb_headroom(skb) < needed) {
>> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
>> + if (unlikely(err)) {
>> + kfree_skb(skb);
>> + return err;
>> + }
>> + }
> use skb_cow_head here.
I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
skb_cow seems a good fit to me though.
>
>>
>> skb_push(skb, sizeof(struct udphdr));
>> skb_reset_transport_header(skb);
>> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> (u8 *)&uh, sizeof(uh));
>>
>> hdr.nexthdr = UIP_PROTO_UDP;
>> + } else {
>> + if (skb_headroom(skb) < sizeof(hdr)) {
>> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>> + if (unlikely(err)) {
>> + kfree_skb(skb);
>> + return err;
>> + }
>> + }
> same here.
>
>> }
>>
>> hdr.payload_len = htons(skb->len);
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index c2e0d14..6643a7c 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>> kfree_skb(local_skb);
>> kfree_skb(skb);
>> } else {
>> + /* Decompression may use pskb_expand_head so no shared skb's */
>> + skb = skb_share_check(skb, GFP_ATOMIC);
>> + if (!skb) {
>> + dev->stats.rx_dropped++;
>> + return NET_RX_DROP;
>> + }
>> +
> I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> is the share_check really needed? In my opinion there are several things
> wrong.
>
> What bluetooth should do here is:
>
> Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
>
> In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
>
> - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> for the one byte dispatch.
> - run other things like skb_reset_network_header, etc...
>
> -> Currently this works because we run a complete skb_copy_expand here.
> But this is not needed, we need a clone because we don't modify the
> data buffer.
>
> -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> for this here.
>
>
>
> In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> this is already handled by "skb_share_check" then.
Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin
^ permalink raw reply
* Re: [PATCH v2 2/2] core: Add subscription API for Manufacturer Specific Data
From: Johan Hedberg @ 2014-10-13 8:01 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1412954626-30226-3-git-send-email-fons@spotify.com>
Hi Alfonso,
On Fri, Oct 10, 2014, Alfonso Acosta wrote:
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -30,6 +30,8 @@
> #include <glib.h>
> #include <stdbool.h>
>
> +#include "eir.h"
> +
> #define MAX_NAME_LENGTH 248
>
> /* Invalid SSP passkey value used to indicate negative replies */
> @@ -138,6 +140,14 @@ struct btd_adapter_pin_cb_iter *btd_adapter_pin_cb_iter_new(
> void btd_adapter_pin_cb_iter_free(struct btd_adapter_pin_cb_iter *iter);
> bool btd_adapter_pin_cb_iter_end(struct btd_adapter_pin_cb_iter *iter);
>
> +typedef void (*btd_msd_cb_t) (struct btd_adapter *adapter,
> + struct btd_device *dev,
> + const struct eir_msd *msd);
> +void btd_adapter_register_msd_cb(struct btd_adapter *adapter,
> + btd_msd_cb_t cb);
In our user space code we try to follow the following principles for
internal header files:
1) The c-file that includes them should also include the prerequisites
2) We don't use multi-include guards
The general idea is that we don't want hidden and implicit dependencies
but prefer having them explicitly spelled out. This practice also helps
detect circular dependencies. For public header files or those of
library-like modules we don't follow this practice (e.g. gdbus/gdbus.h).
As for your patch, I'd suggest to spell out each of the three variables
in your bt_msd_cb_t instead of using the "struct eir_msd" in adapter.h.
That way you don't have a dependency to eir.h from adapter.h.
Johan
^ permalink raw reply
* Re: [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-13 7:35 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <EDC21CD6-FBF6-43DA-A8E4-40BF710EA50E@holtmann.org>
Hi Marcel
> patch looks good, but I wonder why we do not have a diffstat here. Are you not using git format-patch and git send-email to send patches? If not, then you might want to start with that.
I am indeed using format-patch. I went through my commands and I saw I
inserted a -p by error (probably a residue of "log -p").
Do you want me to send a new patch with the stats?
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-12 22:45 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413063887-25870-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
patch looks good, but I wonder why we do not have a diffstat here. Are you not using git format-patch and git send-email to send patches? If not, then you might want to start with that.
Anyhow, since we are in the middle of the merge window for 3.18, I am not yet applying the patch to bluetooth-next, but just for reference here, ack from my side.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-12 10:45 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4r+ofDDBv5+YpFPcvctqYAAGiwNj0u94+XDsoskbSF4Mw@mail.gmail.com>
> I see that, when receiving a Disconnect Complete event,
> hci_disconn_complete_evt() deallocates the connection. But ...
>
> 1. What if the controller misbehaves and doesn't send a Disconnect
> Complete event? AFAIU, the connection will never be deallocated in
> this case.
>
> Wouldn't it make sense to have a timer for this? e.g. make
> hci_disconnect() queue hci_conn_timeout() and, in the later, delete
> the connection if it's not referenced and it was already marked as
> closed.
>
> 2. What if the controller sends an error in the Command Status event
> and the Disconnect Complete never arrives?
>
> I see that hci_cs_disconnect() notifies userland if it was triggered
> through mgmt, but it doesn't provide a retry mechanism in case the
> disconnection came from the kernel itself. In fact, what happens with
> the connection's deallocation in this case? Is it ensured in any way?
3. What if the controller sends an error in the Disconnect Complete? I
see the same issues as in (2).
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-12 0:16 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <5240CE7E-B366-410A-93C4-5FCC3358A68F@holtmann.org>
Hi Marcel,
>>>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *w=
ork)
>>>> conn->state =3D BT_CLOSED;
>>>> break;
>>>> }
>>>> +
>>>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags=
))
>>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_ty=
pe);
>>>
>>> do we really need this one? The hci_conn_timeout should trigger when th=
e connection establishment has a timeout and that is really non of the conc=
erns here. And in case we hit an idle disconnect timeout, we are still endi=
ng up in hci_conn_del eventually, right? If not, I am having the slight fee=
ling that you might have uncovered a bug that we should fix.
>>
>> It's probably just lack of familiarity with the code, but it wasn't
>> all that clear to me that hci_conn_del is called after
>> hci_conn_timeout kicks in. I removed it.
>
> there is always a possibility that you found an actual bug that we trigge=
r, but has not done any harm. At least not harm that anybody has noticed so=
far. So if this happens, then I like to fix the actual bug.
I have gone through the code again to revisit what made me think that
clearing the flag was necessary in hci_conn_timeout() and I may have
possibly found a couple of problems in the deallocation of connections
and handling of disconnection failures.
What made me think that the change in hci_conn_timeout() was necessary
were some call patterns like the following in hci_event.c:
hci_disconnect(conn, reason);
hci_conn_drop(conn);
I wrongly inferred from this that the connection clearing was
sometimes done in hci_conn_timeout() (hci_conn_drop() queues
hci_conn_timeout() when the reference count drops to zero) and not in
hci_conn_del(), to make sure that the connection was cleared
regardless of the status/completion events received from the
controller .
I see that, when receiving a Disconnect Complete event,
hci_disconn_complete_evt() deallocates the connection. But ...
1. What if the controller misbehaves and doesn't send a Disconnect
Complete event? AFAIU, the connection will never be deallocated in
this case.
Wouldn't it make sense to have a timer for this? e.g. make
hci_disconnect() queue hci_conn_timeout() and, in the later, delete
the connection if it's not referenced and it was already marked as
closed.
2. What if the controller sends an error in the Command Status event
and the Disconnect Complete never arrives?
I see that hci_cs_disconnect() notifies userland if it was triggered
through mgmt, but it doesn't provide a retry mechanism in case the
disconnection came from the kernel itself. In fact, what happens with
the connection's deallocation in this case? Is it ensured in any way?
I apologize in advance if my analysis doesn't make much sense or makes
wrong assumptions, as you know, I am still a newbie :)
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 21:44 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..9c4daf7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ /* If disconnection is requested, then look up the
+ * connection. If the remote device is connected, it
+ * will be later used to terminate the link.
+ *
+ * Setting it to NULL explicitly will cause no
+ * termination of the link.
+ */
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL;
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ /* If disconnection is not requested, then
+ * clear the connection variable so that the
+ * link is not terminated.
+ */
+ if (!cp->disconnect)
+ conn = NULL;
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2775,9 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
+ /* If the connection variable is set, then termination of the
+ * link is requested.
+ */
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3082,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 21:44 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <2F926C4D-DBA0-4B6C-A368-FD8E95509A5A@holtmann.org>
Hi Marcel,
> I would have put an extra comment here as well to remind us that conn is for indicating to terminate the connection or not. See my review of v4 of your patch.
>
>> if (!conn) {
>> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
>> &rp, sizeof(rp));
Ops, I missed that one. v6 includes the comment.
Thanks,
Fons
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 16:13 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413037644-15858-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..11aac06 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..063b0a7 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->addr.type == BDADDR_BREDR) {
> + /* If disconnection is requested, then look up the
> + * connection. If the remote device is connected, it
> + * will be later used to terminate the link.
> + *
> + * Setting it to NULL explicitly will cause no
> + * termination of the link.
> + */
> + if (cp->disconnect)
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> + &cp->addr.bdaddr);
> + else
> + conn = NULL;
> +
> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> u8 addr_type;
>
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> + if (conn) {
> + /* Defer clearing up the connection parameters
> + * until closing to give a chance of keeping
> + * them if a repairing happens.
> + */
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> +
> + /* If disconnection is not requested, then
> + * clear the connection variable so that the
> + * link is not terminated.
> + */
> + if (!cp->disconnect)
> + conn = NULL;
> + }
> +
> if (cp->addr.type == BDADDR_LE_PUBLIC)
> addr_type = ADDR_LE_DEV_PUBLIC;
> else
> @@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> -
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
>
> @@ -2747,17 +2775,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - if (cp->disconnect) {
> - if (cp->addr.type == BDADDR_BREDR)
> - conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> - &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> - } else {
> - conn = NULL;
> - }
> -
I would have put an extra comment here as well to remind us that conn is for indicating to terminate the connection or not. See my review of v4 of your patch.
> if (!conn) {
> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
> &rp, sizeof(rp));
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 16:10 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4oqcDKpPySM-qgscD8-=oTrm01Y_KnezTZt4irJ8C1FPg@mail.gmail.com>
Hi Alfonso,
>>> if (cp->addr.type == BDADDR_BREDR) {
>>> + if (cp->disconnect)
>>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>>> + &cp->addr.bdaddr);
>>> + else
>>> + conn = NULL; /* Avoid disconnecting later on*/
>>> +
>>
>> I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
>
> Excuse my ignorance, but Where is the network subsystem coding style
> documented? I haven't been able to find it.
that is actually a good question and I am not sure I have a really good answer for that.
It is essentially Documentation/CodingStyle plus a few extra special cases. Some of them I think are only documented in scripts/checkpatch.pl --strict. The --strict switch is also called --subjective so some of the rules really only apply to the net/ and drivers/net/. I think the term subjective gives it away pretty clearly ;)
We just end up following the rules of the network subsystem here. So there is a difference between the coding style that we use in BlueZ userspace and the one that we use in the kernel. Not a major difference, but it comes to comment styles and multi-line indentations you see the difference.
Regards
Marcel
^ 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