* Re: [PATCH 4/5] l2cap-tester: Remove unneeded variable
From: Andrei Emeltchenko @ 2013-12-16 10:50 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: BlueZ development
In-Reply-To: <CAJdJm_NJCUAeMkvGMYLah3L__DECCd_ocPyd-F46KrcdmCuGMg@mail.gmail.com>
Hi Anderson,
On Mon, Dec 16, 2013 at 06:39:28AM -0400, Anderson Lizardo wrote:
> Hi Andrei,
>
> On Mon, Dec 16, 2013 at 4:57 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > --- a/tools/l2cap-tester.c
> > +++ b/tools/l2cap-tester.c
> > @@ -489,15 +489,14 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> > {
> > const uint8_t *master_bdaddr;
> > struct sockaddr_l2 addr;
> > - int sk, err;
> > + int sk;
> >
> > sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | SOCK_NONBLOCK,
> > BTPROTO_L2CAP);
> > if (sk < 0) {
> > - err = -errno;
> > tester_warn("Can't create socket: %s (%d)", strerror(errno),
> > errno);
> > - return err;
> > + return -errno;
> > }
> >
> > master_bdaddr = hciemu_get_master_bdaddr(data->hciemu);
> > @@ -517,11 +516,10 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> > addr.l2_bdaddr_type = BDADDR_BREDR;
> >
> > if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> > - err = -errno;
> > tester_warn("Can't bind socket: %s (%d)", strerror(errno),
> > errno);
> > close(sk);
> > - return err;
> > + return -errno;
>
> This is not a good idea because close() will overwrite the original
> error if it fails as well. The previous situation is also valid
> because tester_warn() may call library functions that set errno.
Yes, though the return value is not used at all, we may just return -1.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Luiz Augusto von Dentz @ 2013-12-16 13:08 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This IPC is used to communicate Android BlueZ daemon and AudioFlinger
plugin.
---
v2: Rework IPC commands to match Android Audio HAL
android/Makefile.am | 3 +-
android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 1 deletion(-)
create mode 100644 android/audio-ipc-api.txt
diff --git a/android/Makefile.am b/android/Makefile.am
index 79f30d7..ac00bb2 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
android/pixit-gap.txt android/pixit-hid.txt \
android/pixit-opp.txt android/pixit-pan.txt \
android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
- android/pts-opp.txt android/pts-pbap.txt
+ android/pts-opp.txt android/pts-pbap.txt \
+ android/audio-ipc-api.txt
diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
new file mode 100644
index 0000000..e9a2136
--- /dev/null
+++ b/android/audio-ipc-api.txt
@@ -0,0 +1,85 @@
+Bluetooth Audio Plugin
+======================
+
+The audio plugin happen to be in a different socket but all the rules for
+HAL socket apply here as well, the abstract socket name is
+"\0bluez_audio_socket" (tentative):
+
+ .--Android--. .---Audio---.
+ | daemon | | Plugin |
+ | | Command | |
+ | | <-------------------------- | |
+ | | | |
+ | | --------------------------> | |
+ | | Response | |
+ | | | |
+ | | | |
+ | | | |
+ '-----------' '-----------'
+
+
+ Audio HAL Daemon
+ ----------------------------------------------------
+
+ call dev->open() --> command 0x01
+ return dev->open() <-- response 0x01
+
+ call dev->open_output_stream() --> command 0x03
+ return dev->open_output_stream() <-- response 0x03
+
+ call stream_in->read() --> command 0x05
+ return stream_in->read() <-- response 0x05
+
+ call stream_in->common.standby() --> command 0x06
+ return stream_in->common.standby() <-- response 0x06
+
+ call dev->close_output_stream() --> command 0x04
+ return dev->close_output_stream() <-- response 0x04
+
+ call dev->close() --> command 0x02
+ return dev->close() <-- response 0x02
+
+Identifier: "audio" (BT_AUDIO_ID)
+
+ Opcode 0x00 - Error response
+
+ Response parameters: Status (1 octet)
+
+ Opcode 0x01 - Open Audio Endpoint commmand
+
+ Command parameters: Service UUID (16 octets)
+ Codec ID (1 octets)
+ Codec capabilities length (1 octets)
+ Codec capabilities (variable)
+ Number of codec presets (1 octets)
+ Codec preset # length (1 octets)
+ Codec preset # configuration (variable)
+ ...
+ Response parameters: Endpoint ID (1 octets)
+
+ Opcode 0x02 - Close Audio Endpoint command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x03 - Open Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: Codec configuration length (1 octets)
+ Codec configuration (1 octets)
+ File descriptor (inline)
+
+ Opcode 0x04 - Close Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x05 - Resume Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x06 - Suspend Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] bluetooth/hidhost: Fix using feature event as output event
From: Szymon Janc @ 2013-12-16 14:37 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
Rename handle_uhid_event to handle_uhid_output and make it handle only
output events.
---
android/hidhost.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/android/hidhost.c b/android/hidhost.c
index 8bfdfed..0e0391a 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -153,7 +153,8 @@ static void hid_device_free(struct hid_device *dev)
g_free(dev);
}
-static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
+static void handle_uhid_output(struct hid_device *dev,
+ struct uhid_output_req *output)
{
int fd, i;
uint8_t *req = NULL;
@@ -162,15 +163,14 @@ static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
if (!(dev->ctrl_io))
return;
- req_size = 1 + (ev->u.output.size / 2);
+ req_size = 1 + (output->size / 2);
req = g_try_malloc0(req_size);
if (!req)
return;
- req[0] = HID_MSG_SET_REPORT | ev->u.output.rtype;
+ req[0] = HID_MSG_SET_REPORT | output->rtype;
for (i = 0; i < (req_size - 1); i++)
- sscanf((char *) &(ev->u.output.data)[i * 2],
- "%hhx", &req[1 + i]);
+ sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
fd = g_io_channel_unix_get_fd(dev->ctrl_io);
@@ -224,8 +224,10 @@ static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
* asleep This is optional, though. */
break;
case UHID_OUTPUT:
+ handle_uhid_output(dev, &ev.u.output);
+ break;
case UHID_FEATURE:
- handle_uhid_event(dev, &ev);
+ /* TODO */
break;
case UHID_OUTPUT_EV:
/* This is only sent by kernels prior to linux-3.11. It
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/2] android/hidhost: Simplify handle_uhid_output
From: Szymon Janc @ 2013-12-16 14:37 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1387204652-14850-1-git-send-email-szymon.janc@tieto.com>
Make it use VLA for req buffer. This makes function simpler and also
fix cutting req to 255 bytes (req_len was uint8_t)
---
android/hidhost.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/android/hidhost.c b/android/hidhost.c
index 0e0391a..76322af 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
static void handle_uhid_output(struct hid_device *dev,
struct uhid_output_req *output)
{
- int fd, i;
- uint8_t *req = NULL;
- uint8_t req_size = 0;
+ int fd;
+ unsigned int i;
+ uint8_t req[1 + (output->size / 2)];
if (!(dev->ctrl_io))
return;
- req_size = 1 + (output->size / 2);
- req = g_try_malloc0(req_size);
- if (!req)
- return;
-
req[0] = HID_MSG_SET_REPORT | output->rtype;
- for (i = 0; i < (req_size - 1); i++)
+ for (i = 0; i < (sizeof(req) - 1); i++)
sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
fd = g_io_channel_unix_get_fd(dev->ctrl_io);
- if (write(fd, req, req_size) < 0)
+ if (write(fd, req, sizeof(req)) < 0)
error("error writing set_report: %s (%d)",
strerror(errno), errno);
-
- g_free(req);
}
static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/2] android/tester: Add missing break
From: Andrei Emeltchenko @ 2013-12-16 14:52 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
android/android-tester.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index eb938d0..2a8c8d0 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -487,6 +487,7 @@ static void adapter_state_changed_cb(bt_state_t state)
tester_setup_complete();
else
tester_setup_failed();
+ break;
default:
break;
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/2] android/tester: Enable bthost after device is enabled
From: Andrei Emeltchenko @ 2013-12-16 14:52 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387205568-14461-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
This puts bthost to connectible mode allowing us to use powered setup
for connect to emulated device.
---
android/android-tester.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/android/android-tester.c b/android/android-tester.c
index 2a8c8d0..d0d3690 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -33,10 +33,15 @@
#include "src/shared/mgmt.h"
#include "src/shared/hciemu.h"
+#include "emulator/bthost.h"
+#include "monitor/bt.h"
+
#include <hardware/hardware.h>
#include <hardware/bluetooth.h>
#include <hardware/bt_sock.h>
+#include "utils.h"
+
#define adapter_props adapter_prop_bdaddr, adapter_prop_bdname, \
adapter_prop_uuids, adapter_prop_cod, \
adapter_prop_type, adapter_prop_scan_mode, \
@@ -463,6 +468,42 @@ failed:
close(fd);
}
+static void client_connectable_complete(uint16_t opcode, uint8_t status,
+ const void *param, uint8_t len,
+ void *user_data)
+{
+ switch (opcode) {
+ case BT_HCI_CMD_WRITE_SCAN_ENABLE:
+ case BT_HCI_CMD_LE_SET_ADV_ENABLE:
+ break;
+ default:
+ return;
+ }
+
+ tester_print("Client set connectable status 0x%02x", status);
+
+ if (status)
+ tester_setup_failed();
+ else
+ tester_setup_complete();
+}
+
+static void setup_powered_client(void)
+{
+ struct test_data *data = tester_get_data();
+ struct bthost *bthost;
+
+ tester_print("Controller powered on");
+
+ bthost = hciemu_client_get_host(data->hciemu);
+ bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
+
+ if (data->hciemu_type == HCIEMU_TYPE_LE)
+ bthost_set_adv_enable(bthost, 0x01);
+ else
+ bthost_write_scan_enable(bthost, 0x03);
+}
+
static void adapter_state_changed_cb(bt_state_t state)
{
enum hal_bluetooth_callbacks_id hal_cb;
@@ -484,7 +525,7 @@ static void adapter_state_changed_cb(bt_state_t state)
break;
case adapter_test_setup_mode:
if (state == BT_STATE_ON)
- tester_setup_complete();
+ setup_powered_client();
else
tester_setup_failed();
break;
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 2/2] android/hidhost: Simplify handle_uhid_output
From: Luiz Augusto von Dentz @ 2013-12-16 15:19 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1387204652-14850-2-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
On Mon, Dec 16, 2013 at 4:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Make it use VLA for req buffer. This makes function simpler and also
> fix cutting req to 255 bytes (req_len was uint8_t)
> ---
> android/hidhost.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 0e0391a..76322af 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
> static void handle_uhid_output(struct hid_device *dev,
> struct uhid_output_req *output)
> {
> - int fd, i;
> - uint8_t *req = NULL;
> - uint8_t req_size = 0;
> + int fd;
> + unsigned int i;
> + uint8_t req[1 + (output->size / 2)];
Im not a fan of VLA and Ive seem even some static analyzer that would
warn about its use without first checking > 0 and have a check of
upper bond limit, so perhaps just having it set to UHID_DATA_MAX or
dynamically allocate a buffer to match the output MTU size of the
control channel would better imo.
--
Luiz Augusto von Dentz
^ permalink raw reply
* RFCOMM connection failing
From: Patrick Valsecchi @ 2013-12-16 15:55 UTC (permalink / raw)
To: linux-bluetooth
Hi,
I'm trying to connect my PC:
Linux ... 3.11.0-14-generic #21-Ubuntu SMP ... x86_64 x86_64 x86_64
GNU/Linux
0a5c:2198 Broadcom Corp. Bluetooth 3.0 Device
To connect with my bluetooth dive computer:
Shearwater Petrel
It fails with a "Transport endpoint is not connected (107)" most of the
time or go further but seem to have corrupted RFCOMM payload.
If I pass the USB device to a W7 VM (virtualbox) and try from them, the
communication works like charm.
So I went ahead and sniffed the USB communication in both cases using
wireshark. The two dumps () can be found here (UsbDumpFrom*.pcapng, can
be open using wireshark):
https://cloud.thus.ch/public.php?service=files&t=de2eabf30c82efa08cf546ff5045e585
Basically the Linux one is just stopping at frame 203 where it sends a
RFCOMM SABM, gets the answer and reports an error to the user. The
windows dump show the same RFCOMM SABM command in frame 191 and gets the
same answer but continues and everything works.
On IRC aholler told me you guys would prefer to get a btmon dump with
bluez v5. So I went ahead, installed bluez 5.12 and generated a dump
(yes, the comm still fails the same way) that you can find in the same
location as the dumps.
I'm stuck there. Can somebody help me go further?
Thanks
^ permalink raw reply
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Lukasz Rymanowski @ 2013-12-16 16:27 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1387199317-27438-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
> plugin.
> ---
> v2: Rework IPC commands to match Android Audio HAL
>
> android/Makefile.am | 3 +-
> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 87 insertions(+), 1 deletion(-)
> create mode 100644 android/audio-ipc-api.txt
>
> diff --git a/android/Makefile.am b/android/Makefile.am
> index 79f30d7..ac00bb2 100644
> --- a/android/Makefile.am
> +++ b/android/Makefile.am
> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
> android/pixit-gap.txt android/pixit-hid.txt \
> android/pixit-opp.txt android/pixit-pan.txt \
> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
> - android/pts-opp.txt android/pts-pbap.txt
> + android/pts-opp.txt android/pts-pbap.txt \
> + android/audio-ipc-api.txt
> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
> new file mode 100644
> index 0000000..e9a2136
> --- /dev/null
> +++ b/android/audio-ipc-api.txt
> @@ -0,0 +1,85 @@
> +Bluetooth Audio Plugin
> +======================
> +
> +The audio plugin happen to be in a different socket but all the rules for
> +HAL socket apply here as well, the abstract socket name is
> +"\0bluez_audio_socket" (tentative):
> +
> + .--Android--. .---Audio---.
> + | daemon | | Plugin |
> + | | Command | |
> + | | <-------------------------- | |
> + | | | |
> + | | --------------------------> | |
> + | | Response | |
> + | | | |
> + | | | |
> + | | | |
> + '-----------' '-----------'
> +
> +
> + Audio HAL Daemon
> + ----------------------------------------------------
> +
> + call dev->open() --> command 0x01
> + return dev->open() <-- response 0x01
> +
> + call dev->open_output_stream() --> command 0x03
> + return dev->open_output_stream() <-- response 0x03
> +
> + call stream_in->read() --> command 0x05
> + return stream_in->read() <-- response 0x05
> +
I think it should be stream_out->write() here.
> + call stream_in->common.standby() --> command 0x06
> + return stream_in->common.standby() <-- response 0x06
> +
Also here: stream_out->common.standby()
> + call dev->close_output_stream() --> command 0x04
> + return dev->close_output_stream() <-- response 0x04
> +
> + call dev->close() --> command 0x02
> + return dev->close() <-- response 0x02
> +
> +Identifier: "audio" (BT_AUDIO_ID)
> +
> + Opcode 0x00 - Error response
> +
> + Response parameters: Status (1 octet)
> +
> + Opcode 0x01 - Open Audio Endpoint commmand
> +
> + Command parameters: Service UUID (16 octets)
> + Codec ID (1 octets)
> + Codec capabilities length (1 octets)
> + Codec capabilities (variable)
> + Number of codec presets (1 octets)
> + Codec preset # length (1 octets)
> + Codec preset # configuration (variable)
> + ...
> + Response parameters: Endpoint ID (1 octets)
> +
> + Opcode 0x02 - Close Audio Endpoint command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x03 - Open Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: Codec configuration length (1 octets)
> + Codec configuration (1 octets)
> + File descriptor (inline)
> +
> + Opcode 0x04 - Close Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x05 - Resume Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x06 - Suspend Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> --
> 1.8.3.1
>
Otherwise looks ok to me.
BR
Lukasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Luiz Augusto von Dentz @ 2013-12-16 18:31 UTC (permalink / raw)
To: Lukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CAN_7+YaOQLno_kZoi7sAk2f8Ky_natHGa1Zi-J9vErfZ5WW8ug@mail.gmail.com>
Hi Lukasz,
On Mon, Dec 16, 2013 at 6:27 PM, Lukasz Rymanowski
<lukasz.rymanowski@gmail.com> wrote:
> Hi Luiz,
>
>
> On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
>> plugin.
>> ---
>> v2: Rework IPC commands to match Android Audio HAL
>>
>> android/Makefile.am | 3 +-
>> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 87 insertions(+), 1 deletion(-)
>> create mode 100644 android/audio-ipc-api.txt
>>
>> diff --git a/android/Makefile.am b/android/Makefile.am
>> index 79f30d7..ac00bb2 100644
>> --- a/android/Makefile.am
>> +++ b/android/Makefile.am
>> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
>> android/pixit-gap.txt android/pixit-hid.txt \
>> android/pixit-opp.txt android/pixit-pan.txt \
>> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
>> - android/pts-opp.txt android/pts-pbap.txt
>> + android/pts-opp.txt android/pts-pbap.txt \
>> + android/audio-ipc-api.txt
>> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
>> new file mode 100644
>> index 0000000..e9a2136
>> --- /dev/null
>> +++ b/android/audio-ipc-api.txt
>> @@ -0,0 +1,85 @@
>> +Bluetooth Audio Plugin
>> +======================
>> +
>> +The audio plugin happen to be in a different socket but all the rules for
>> +HAL socket apply here as well, the abstract socket name is
>> +"\0bluez_audio_socket" (tentative):
>> +
>> + .--Android--. .---Audio---.
>> + | daemon | | Plugin |
>> + | | Command | |
>> + | | <-------------------------- | |
>> + | | | |
>> + | | --------------------------> | |
>> + | | Response | |
>> + | | | |
>> + | | | |
>> + | | | |
>> + '-----------' '-----------'
>> +
>> +
>> + Audio HAL Daemon
>> + ----------------------------------------------------
>> +
>> + call dev->open() --> command 0x01
>> + return dev->open() <-- response 0x01
>> +
>> + call dev->open_output_stream() --> command 0x03
>> + return dev->open_output_stream() <-- response 0x03
>> +
>> + call stream_in->read() --> command 0x05
>> + return stream_in->read() <-- response 0x05
>> +
> I think it should be stream_out->write() here.
>
>> + call stream_in->common.standby() --> command 0x06
>> + return stream_in->common.standby() <-- response 0x06
>> +
> Also here: stream_out->common.standby()
I guess it should work both ways, but perhaps Android is never meant
to sink role nevertheless I would not limit the IPC to just source as
it could be useful and it doesn't need that much to work.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Peter Hurley @ 2013-12-16 19:34 UTC (permalink / raw)
To: Gianluca Anzolin
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131215150847.GA10288@sottospazio.it>
On 12/15/2013 10:08 AM, Gianluca Anzolin wrote:
> On Sun, Dec 15, 2013 at 09:03:35AM -0500, Peter Hurley wrote:
>> On 12/15/2013 06:24 AM, Gianluca Anzolin wrote:
>>> On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
>>>> Am 12.12.2013 21:36, schrieb Peter Hurley:
>>>>
>>>>>> What currently happens is that when one kills rfcomm (and any other
>>>>>> terminal which might use that tty), the entry in /dev doesn't
>>>>>> disappear. That means the same call to refcomm with the same device
>>>>>> (e.g. [/dev/]rfcomm1 doesn't work.
>>>>>
>>>>> Thanks for the report, Alexander.
>>>>>
>>>>> Point 4 above details a different situation; something else is
>>>>> happening.
>>>>>
>>>>> Would you please detail the necessary steps to reproduce this regression?
>>>>> (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
>>>>
>>>> Just call
>>>>
>>>> rfcomm connect rfcomm9 01:23:45:67:89:ab
>>>>
>>>> wait until the connection happened (a message will appear) and then
>>>> press ctrl-c. This still terminates the bluetooth connection, but the
>>>> device in /dev is now left.
>>>
>>> Yes I'm able to reproduce the regression which is indeed caused by that
>>> commit.
>>>
>>> However I'm puzzled. Surely there is a fifth case I didn't cover because
>>> when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
>>> not, and therefore I cannot get a reference to it and send the HUP.
>>
>> There is a fifth case, but it's crazy.
>>
>> The tty has been properly shutdown and destroyed because the tty file handle
>> was closed, not hungup. The rfcomm device reference was properly put
>> when the tty was released.
>>
>> But when the remote hangs up (and sends disc), then rfcomm_dev_state_change()
>> is called -- to kill the port reference (thus the rfcomm device) that was
>> instantiated locally! Ridiculous. Doubly ridiculous because it's the local
>> port shutdown that closes the dlc locally that sends the disconnect (and sets
>> the local dlc state) that triggers the received rfcomm_dev_state_change()!
>>
>> If this behavior is desirable (or necessary because it's been exposed to
>> userspace), then why was the design ever reference-counted to begin with?
>>
>> Regards,
>> Peter Hurley
>
> The attached patch fixes the regression by releasing the tty_port in the
> shutdown method(). This way we can avoid strange games in the dlc callback
> where we are constrained by the dlc lock.
>
> If this kind of approach is acceptable I will submit the patch for inclusion in
> a separate email.
This solution is acceptable to me, but I think the comment should briefly
explain why this fix is necessary, and the changelog should explain why in detail.
Perhaps with a fixme comment that rfcomm_tty_install() should just take over
the port reference (instead of adding one) and rfcomm_tty_cleanup() should
conditionally release on RFCOMM_RELEASE_ONHUP.
Because then:
1) this fix would not be necessary.
2) the release in rfcomm_tty_hangup() would not be necessary
3) the second release in rfcomm_release_dev would not be necessary
4) the RFCOMM_TTY_RELEASED bit could be removed
Regards,
Peter Hurley
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:20 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <52AF55B4.6000303@hurleysoftware.com>
On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
>
> This solution is acceptable to me, but I think the comment should briefly
> explain why this fix is necessary, and the changelog should explain why in detail.
>
> Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> conditionally release on RFCOMM_RELEASE_ONHUP.
>
> Because then:
> 1) this fix would not be necessary.
> 2) the release in rfcomm_tty_hangup() would not be necessary
> 3) the second release in rfcomm_release_dev would not be necessary
> 4) the RFCOMM_TTY_RELEASED bit could be removed
>
>
> Regards,
> Peter Hurley
Taking over the refcount in the install method would certainly look better. I'm
going to test it ASAP :D
But why getting rid of the release in in rfcomm_tty_hangup()?
We could lose the bluetooth connection at any time and the dlc callback
would have to hangup the tty (and release the port if necessary).
Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
created without the RFCOMM_RELEASE_ONHUP flag.
Besides any process could release the port behind us (with the command rfcomm
release rfcomm1 for example).
Gianluca
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:27 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216202044.GA19746@sottospazio.it>
On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> >
> > This solution is acceptable to me, but I think the comment should briefly
> > explain why this fix is necessary, and the changelog should explain why in detail.
> >
> > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > conditionally release on RFCOMM_RELEASE_ONHUP.
> >
> > Because then:
> > 1) this fix would not be necessary.
> > 2) the release in rfcomm_tty_hangup() would not be necessary
> > 3) the second release in rfcomm_release_dev would not be necessary
> > 4) the RFCOMM_TTY_RELEASED bit could be removed
> >
> >
> > Regards,
> > Peter Hurley
>
> Taking over the refcount in the install method would certainly look better. I'm
> going to test it ASAP :D
>
> But why getting rid of the release in in rfcomm_tty_hangup()?
> We could lose the bluetooth connection at any time and the dlc callback
> would have to hangup the tty (and release the port if necessary).
>
> Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> created without the RFCOMM_RELEASE_ONHUP flag.
>
> Besides any process could release the port behind us (with the command rfcomm
> release rfcomm1 for example).
>
> Gianluca
Nevermind I figured it out the reason...
^ permalink raw reply
* [patch] Bluetooth: fix ->alloc_skb() error checking
From: Dan Carpenter @ 2013-12-16 20:28 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo Padovan, Johan Hedberg, David S. Miller, linux-bluetooth,
netdev, kernel-janitors
There are two functions that implement ->alloc_skb().
a2mp_cahan_alloc_skb_cb() returns NULL.
l2cap_sock_alloc_skb_cb() returns an ERR_PTR.
The callers all check for ERR_PTRs and don't check for NULL. On the
other hand bt_skb_alloc() and the core net alloc_skb() return NULL so
returning an error pointer is inconsistent. This confusion between some
alloc_skb() functions returning ERR_PTR and some returning NULL has been
a source of bugs such as 787949039fcd ('Bluetooth: fix return value
check'). This patch makes ->alloc_skb() return NULL and changes the
callers to match.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b0ad2c752d73..84c3ce04cb35 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2341,8 +2341,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
tmp = chan->ops->alloc_skb(chan, count,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(tmp))
- return PTR_ERR(tmp);
+ if (!tmp)
+ return -ENOMEM;
*frag = tmp;
@@ -2379,8 +2379,8 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
skb->priority = priority;
@@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (skb)
+ return ERR_PTR(-ENOMEM);
skb->priority = priority;
@@ -2457,8 +2457,8 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
/* Create L2CAP header */
lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
@@ -2578,8 +2578,8 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
/* Create L2CAP header */
lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
^ permalink raw reply related
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Lukasz Rymanowski @ 2013-12-16 20:33 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZ+d6c1ohwkj4Bqoa_=fN+aCT34LwJR1eE53xix_woPYow@mail.gmail.com>
Hi Luiz,
On Mon, Dec 16, 2013 at 7:31 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lukasz,
>
> On Mon, Dec 16, 2013 at 6:27 PM, Lukasz Rymanowski
> <lukasz.rymanowski@gmail.com> wrote:
>> Hi Luiz,
>>
>>
>> On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
>>> plugin.
>>> ---
>>> v2: Rework IPC commands to match Android Audio HAL
>>>
>>> android/Makefile.am | 3 +-
>>> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 87 insertions(+), 1 deletion(-)
>>> create mode 100644 android/audio-ipc-api.txt
>>>
>>> diff --git a/android/Makefile.am b/android/Makefile.am
>>> index 79f30d7..ac00bb2 100644
>>> --- a/android/Makefile.am
>>> +++ b/android/Makefile.am
>>> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
>>> android/pixit-gap.txt android/pixit-hid.txt \
>>> android/pixit-opp.txt android/pixit-pan.txt \
>>> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
>>> - android/pts-opp.txt android/pts-pbap.txt
>>> + android/pts-opp.txt android/pts-pbap.txt \
>>> + android/audio-ipc-api.txt
>>> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
>>> new file mode 100644
>>> index 0000000..e9a2136
>>> --- /dev/null
>>> +++ b/android/audio-ipc-api.txt
>>> @@ -0,0 +1,85 @@
>>> +Bluetooth Audio Plugin
>>> +======================
>>> +
>>> +The audio plugin happen to be in a different socket but all the rules for
>>> +HAL socket apply here as well, the abstract socket name is
>>> +"\0bluez_audio_socket" (tentative):
>>> +
>>> + .--Android--. .---Audio---.
>>> + | daemon | | Plugin |
>>> + | | Command | |
>>> + | | <-------------------------- | |
>>> + | | | |
>>> + | | --------------------------> | |
>>> + | | Response | |
>>> + | | | |
>>> + | | | |
>>> + | | | |
>>> + '-----------' '-----------'
>>> +
>>> +
>>> + Audio HAL Daemon
>>> + ----------------------------------------------------
>>> +
>>> + call dev->open() --> command 0x01
>>> + return dev->open() <-- response 0x01
>>> +
>>> + call dev->open_output_stream() --> command 0x03
>>> + return dev->open_output_stream() <-- response 0x03
>>> +
>>> + call stream_in->read() --> command 0x05
>>> + return stream_in->read() <-- response 0x05
>>> +
>> I think it should be stream_out->write() here.
>>
>>> + call stream_in->common.standby() --> command 0x06
>>> + return stream_in->common.standby() <-- response 0x06
>>> +
>> Also here: stream_out->common.standby()
>
> I guess it should work both ways, but perhaps Android is never meant
> to sink role nevertheless I would not limit the IPC to just source as
> it could be useful and it doesn't need that much to work.
>
I agree that it should work for stream in as well, and actually I can
imagine product based on Android acting as SINK on A2DP.
Anyway, maybe it would be better to point it out directly and keep HAL
calls example for stream out.
/Łukasz
> --
> Luiz Augusto von Dentz
^ permalink raw reply
* Re: [patch] Bluetooth: fix ->alloc_skb() error checking
From: Anderson Lizardo @ 2013-12-16 20:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, David S. Miller,
BlueZ development, netdev, kernel-janitors
In-Reply-To: <20131216202857.GB19601@elgon.mountain>
Hi Dan,
On Mon, Dec 16, 2013 at 4:28 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> @@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
>
> skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> msg->msg_flags & MSG_DONTWAIT);
> - if (IS_ERR(skb))
> - return skb;
> + if (skb)
> + return ERR_PTR(-ENOMEM);
It should be "!skb" above right?
>
> skb->priority = priority;
>
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [patch] Bluetooth: fix ->alloc_skb() error checking
From: Dan Carpenter @ 2013-12-16 20:57 UTC (permalink / raw)
To: Anderson Lizardo
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, David S. Miller,
BlueZ development, netdev, kernel-janitors
In-Reply-To: <CAJdJm_O9q3YgpjTQexbLvMm8ViViu+MrmRCqPwQDDUzaxVQYUg@mail.gmail.com>
On Mon, Dec 16, 2013 at 04:35:25PM -0400, Anderson Lizardo wrote:
> Hi Dan,
>
> On Mon, Dec 16, 2013 at 4:28 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > @@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> >
> > skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> > msg->msg_flags & MSG_DONTWAIT);
> > - if (IS_ERR(skb))
> > - return skb;
> > + if (skb)
> > + return ERR_PTR(-ENOMEM);
>
> It should be "!skb" above right?
>
Gar.... I'm so sorry about that.
regards,
dan carpenter
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:58 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216202720.GB19746@sottospazio.it>
[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]
On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > >
> > > This solution is acceptable to me, but I think the comment should briefly
> > > explain why this fix is necessary, and the changelog should explain why in detail.
> > >
> > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > >
> > > Because then:
> > > 1) this fix would not be necessary.
> > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > 3) the second release in rfcomm_release_dev would not be necessary
> > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > >
> > >
> > > Regards,
> > > Peter Hurley
> >
> > Taking over the refcount in the install method would certainly look better. I'm
> > going to test it ASAP :D
> >
> > But why getting rid of the release in in rfcomm_tty_hangup()?
> > We could lose the bluetooth connection at any time and the dlc callback
> > would have to hangup the tty (and release the port if necessary).
> >
> > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > created without the RFCOMM_RELEASE_ONHUP flag.
> >
> > Besides any process could release the port behind us (with the command rfcomm
> > release rfcomm1 for example).
> >
> > Gianluca
>
> Nevermind I figured it out the reason...
I'm testing the attached patch ATM, which does what you described. It works
very well.
It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
that bit.
Does it look better?
Thanks,
Gianluca
[-- Attachment #2: rfc2.patch --]
[-- Type: text/x-diff, Size: 1221 bytes --]
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..f455a22 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,9 +437,6 @@ static int rfcomm_release_dev(void __user *arg)
tty_kref_put(tty);
}
- if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
-
tty_port_put(&dev->port);
return 0;
}
@@ -673,6 +670,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
if (err)
rfcomm_tty_cleanup(tty);
+ /* take over the tty_port reference if it was created with the
+ * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+ * when the last process closes the tty. This behaviour is expected by
+ * userspace.
+ */
+ if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+ tty_port_put(&dev->port);
+
return err;
}
@@ -1010,10 +1015,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
BT_DBG("tty %p dev %p", tty, dev);
tty_port_hangup(&dev->port);
-
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
- !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
}
static int rfcomm_tty_tiocmget(struct tty_struct *tty)
^ permalink raw reply related
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 21:15 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216205858.GA20119@sottospazio.it>
[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]
On Mon, Dec 16, 2013 at 09:58:58PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > > >
> > > > This solution is acceptable to me, but I think the comment should briefly
> > > > explain why this fix is necessary, and the changelog should explain why in detail.
> > > >
> > > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > > >
> > > > Because then:
> > > > 1) this fix would not be necessary.
> > > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > > 3) the second release in rfcomm_release_dev would not be necessary
> > > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > > >
> > > >
> > > > Regards,
> > > > Peter Hurley
> > >
> > > Taking over the refcount in the install method would certainly look better. I'm
> > > going to test it ASAP :D
> > >
> > > But why getting rid of the release in in rfcomm_tty_hangup()?
> > > We could lose the bluetooth connection at any time and the dlc callback
> > > would have to hangup the tty (and release the port if necessary).
> > >
> > > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > > created without the RFCOMM_RELEASE_ONHUP flag.
> > >
> > > Besides any process could release the port behind us (with the command rfcomm
> > > release rfcomm1 for example).
> > >
> > > Gianluca
> >
> > Nevermind I figured it out the reason...
>
> I'm testing the attached patch ATM, which does what you described. It works
> very well.
>
> It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
> that bit.
ok, replying to myself again (sorry for that). RFCOMM_TTY_RELEASE cannot go
away. We have still to manage the case where the port is opened without
RFCOMM_RELEASE_ONHUP.
This last patch does release the port in that situation.
Tested with:
# rfcomm bind 1 <addr>
# rfcomm release 1
and with
# rfcomm connect 1 <addr>
Thanks,
Gianluca
[-- Attachment #2: rfc3.patch --]
[-- Type: text/x-diff, Size: 1319 bytes --]
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..0357dcf 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,7 +437,8 @@ static int rfcomm_release_dev(void __user *arg)
tty_kref_put(tty);
}
- if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+ if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+ !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
tty_port_put(&dev->port);
tty_port_put(&dev->port);
@@ -673,6 +674,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
if (err)
rfcomm_tty_cleanup(tty);
+ /* take over the tty_port reference if it was created with the
+ * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+ * when the last process closes the tty. This behaviour is expected by
+ * userspace.
+ */
+ if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+ tty_port_put(&dev->port);
+
return err;
}
@@ -1010,10 +1019,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
BT_DBG("tty %p dev %p", tty, dev);
tty_port_hangup(&dev->port);
-
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
- !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
}
static int rfcomm_tty_tiocmget(struct tty_struct *tty)
^ permalink raw reply related
* [PATCH_v2 1/6] android/pan: Rename pan_device_free to destroy_pan_device
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
Renaming function name because it does more than freeing memory.
Also moving disconnect notification call to destory_pan_device
reduce redundancy.
---
android/pan.c | 51 +++++++++++++++++++++++----------------------------
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/android/pan.c b/android/pan.c
index e410f54..ec589cf 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -65,24 +65,6 @@ static int device_cmp(gconstpointer s, gconstpointer user_data)
return bacmp(&dev->dst, dst);
}
-static void pan_device_free(struct pan_device *dev)
-{
- local_role = HAL_PAN_ROLE_NONE;
-
- if (dev->watch > 0) {
- g_source_remove(dev->watch);
- dev->watch = 0;
- }
-
- if (dev->io) {
- g_io_channel_unref(dev->io);
- dev->io = NULL;
- }
-
- devices = g_slist_remove(devices, dev);
- g_free(dev);
-}
-
static void bt_pan_notify_conn_state(struct pan_device *dev, uint8_t state)
{
struct hal_ev_pan_conn_state ev;
@@ -121,6 +103,25 @@ static void bt_pan_notify_ctrl_state(struct pan_device *dev, uint8_t state)
&ev);
}
+static void destroy_pan_device(struct pan_device *dev)
+{
+ local_role = HAL_PAN_ROLE_NONE;
+
+ if (dev->watch > 0) {
+ g_source_remove(dev->watch);
+ dev->watch = 0;
+ }
+
+ if (dev->io) {
+ g_io_channel_unref(dev->io);
+ dev->io = NULL;
+ }
+
+ bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+ devices = g_slist_remove(devices, dev);
+ g_free(dev);
+}
+
static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
@@ -130,8 +131,7 @@ static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
bnep_if_down(dev->iface);
bnep_conndel(&dev->dst);
- bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
- pan_device_free(dev);
+ destroy_pan_device(dev);
return FALSE;
}
@@ -145,8 +145,7 @@ static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
if (err < 0) {
error("bnep connect req failed: %s", strerror(-err));
bnep_conndel(&dev->dst);
- bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
- pan_device_free(dev);
+ destroy_pan_device(dev);
return;
}
@@ -189,8 +188,7 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer data)
return;
fail:
- bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
- pan_device_free(dev);
+ destroy_pan_device(dev);
}
static void bt_pan_connect(const void *buf, uint16_t len)
@@ -292,10 +290,7 @@ static void bt_pan_disconnect(const void *buf, uint16_t len)
bnep_if_down(dev->iface);
bnep_conndel(&dst);
-
- bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
- pan_device_free(dev);
-
+ destroy_pan_device(dev);
status = HAL_STATUS_SUCCESS;
failed:
--
1.8.3.2
^ permalink raw reply related
* [PATCH_v2 2/6] android/pan: Rename connect_cb to bt_io_connect_cb
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
In-Reply-To: <1387231516-4127-1-git-send-email-ravikumar.veeramally@linux.intel.com>
Renaming for easy readability.
---
android/pan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/android/pan.c b/android/pan.c
index ec589cf..03db350 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -162,7 +162,7 @@ static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
dev->io = NULL;
}
-static void connect_cb(GIOChannel *chan, GError *err, gpointer data)
+static void bt_io_connect_cb(GIOChannel *chan, GError *err, gpointer data)
{
struct pan_device *dev = data;
uint16_t src, dst;
@@ -238,7 +238,7 @@ static void bt_pan_connect(const void *buf, uint16_t len)
ba2str(&dev->dst, addr);
DBG("connecting to %s %s", addr, dev->iface);
- dev->io = bt_io_connect(connect_cb, dev, NULL, &gerr,
+ dev->io = bt_io_connect(bt_io_connect_cb, dev, NULL, &gerr,
BT_IO_OPT_SOURCE_BDADDR, &adapter_addr,
BT_IO_OPT_DEST_BDADDR, &dev->dst,
BT_IO_OPT_PSM, BNEP_PSM,
--
1.8.3.2
^ permalink raw reply related
* [PATCH_v2 3/6] bnep: Add bnep_new and bnep_free api's
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
In-Reply-To: <1387231516-4127-1-git-send-email-ravikumar.veeramally@linux.intel.com>
Refacoring connect and disconnect mechanisms. It would be more
convinient for caller to maintain just bnep connection reference
and delete whenever it is not required.
---
profiles/network/bnep.c | 37 +++++++++++++++++++++++++++++++++++++
profiles/network/bnep.h | 5 +++++
2 files changed, 42 insertions(+)
diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 08037e6..d7d8832 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -71,6 +71,7 @@ struct bnep_conn {
GIOChannel *io;
uint16_t src;
uint16_t dst;
+ bdaddr_t dst_addr;
guint attempts;
guint setup_to;
void *data;
@@ -246,6 +247,42 @@ int bnep_if_down(const char *devname)
return 0;
}
+struct bnep_conn *bnep_new(uint16_t src, uint16_t dst,
+ const bdaddr_t *dst_addr)
+{
+ struct bnep_conn *bc;
+
+ DBG("");
+
+ if (!dst_addr)
+ return NULL;
+
+ bc = g_new0(struct bnep_conn, 1);
+ if (!bc)
+ return NULL;
+
+ bc->src = src;
+ bc->dst = dst;
+ bacpy(&bc->dst_addr, dst_addr);
+
+ return bc;
+}
+
+void bnep_free(struct bnep_conn *bc)
+{
+ DBG("");
+
+ if (!bc)
+ return;
+
+ if (bc->io) {
+ g_io_channel_unref(bc->io);
+ bc->io = NULL;
+ }
+
+ g_free(bc);
+}
+
static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
diff --git a/profiles/network/bnep.h b/profiles/network/bnep.h
index dd22c40..9c28899 100644
--- a/profiles/network/bnep.h
+++ b/profiles/network/bnep.h
@@ -21,6 +21,8 @@
*
*/
+struct bnep_conn;
+
int bnep_init(void);
int bnep_cleanup(void);
@@ -28,6 +30,9 @@ uint16_t bnep_service_id(const char *svc);
const char *bnep_uuid(uint16_t id);
const char *bnep_name(uint16_t id);
+struct bnep_conn *bnep_new(uint16_t src, uint16_t dst,
+ const bdaddr_t *dst_addr);
+void bnep_free(struct bnep_conn *bnep);
int bnep_connadd(int sk, uint16_t role, char *dev);
int bnep_conndel(const bdaddr_t *dst);
int bnep_if_up(const char *devname);
--
1.8.3.2
^ permalink raw reply related
* [PATCH_v2 4/6] bnep: Refactored bnep connect and disconnect calls
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
In-Reply-To: <1387231516-4127-1-git-send-email-ravikumar.veeramally@linux.intel.com>
Refactored bnep connect and disconnect calls to simply and
keeping bnep related functionality behind curtains.
Provided bnep_conn struct globally. bnep_connect calls takes
care of bnep_setup until interface up then connect callback
will be called, disconnect_cb is registered as watchdog and it
will triggered by remote device on disconnect or any other
I/O error. bnep_disconnect should be called only when iface is
up/connected.
---
android/pan.c | 56 +++++++--------------
profiles/network/bnep.c | 112 +++++++++++++++++++++++++-----------------
profiles/network/bnep.h | 10 ++--
profiles/network/connection.c | 50 ++++++++++++-------
4 files changed, 122 insertions(+), 106 deletions(-)
diff --git a/android/pan.c b/android/pan.c
index 03db350..0cae6ac 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -54,7 +54,7 @@ struct pan_device {
uint8_t conn_state;
uint8_t role;
GIOChannel *io;
- guint watch;
+ struct bnep_conn *bnep;
};
static int device_cmp(gconstpointer s, gconstpointer user_data)
@@ -105,38 +105,31 @@ static void bt_pan_notify_ctrl_state(struct pan_device *dev, uint8_t state)
static void destroy_pan_device(struct pan_device *dev)
{
- local_role = HAL_PAN_ROLE_NONE;
-
- if (dev->watch > 0) {
- g_source_remove(dev->watch);
- dev->watch = 0;
- }
-
if (dev->io) {
+ g_io_channel_shutdown(dev->io, TRUE, NULL);
g_io_channel_unref(dev->io);
dev->io = NULL;
}
+ if (dev->conn_state == HAL_PAN_STATE_CONNECTED)
+ bnep_disconnect(dev->bnep);
+
bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+ bnep_free(dev->bnep);
devices = g_slist_remove(devices, dev);
g_free(dev);
}
-static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
- gpointer data)
+static void bnep_disconn_cb(void *data)
{
struct pan_device *dev = data;
DBG("%s disconnected", dev->iface);
- bnep_if_down(dev->iface);
- bnep_conndel(&dev->dst);
destroy_pan_device(dev);
-
- return FALSE;
}
-static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
+static void bnep_conn_cb(char *iface, int err, void *data)
{
struct pan_device *dev = data;
@@ -144,22 +137,15 @@ static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
if (err < 0) {
error("bnep connect req failed: %s", strerror(-err));
- bnep_conndel(&dev->dst);
destroy_pan_device(dev);
return;
}
- memcpy(dev->iface, iface, sizeof(dev->iface));
-
- DBG("%s connected", dev->iface);
+ DBG("%s connected", iface);
+ memcpy(dev->iface, iface, sizeof(dev->iface));
bt_pan_notify_ctrl_state(dev, HAL_PAN_CTRL_ENABLED);
bt_pan_notify_conn_state(dev, HAL_PAN_STATE_CONNECTED);
-
- dev->watch = g_io_add_watch(chan, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
- bnep_watchdog_cb, dev);
- g_io_channel_unref(dev->io);
- dev->io = NULL;
}
static void bt_io_connect_cb(GIOChannel *chan, GError *err, gpointer data)
@@ -179,7 +165,13 @@ static void bt_io_connect_cb(GIOChannel *chan, GError *err, gpointer data)
dst = (dev->role == HAL_PAN_ROLE_NAP) ? BNEP_SVC_NAP : BNEP_SVC_PANU;
sk = g_io_channel_unix_get_fd(dev->io);
- perr = bnep_connect(sk, src, dst, bnep_conn_cb, dev);
+ dev->bnep = bnep_new(src, dst, &dev->dst);
+ if (!dev->bnep) {
+ error("unable to allocate memory for dev->bnep");
+ goto fail;
+ }
+
+ perr = bnep_connect(dev->bnep, sk, bnep_conn_cb, bnep_disconn_cb, dev);
if (perr < 0) {
error("bnep connect req failed: %s", strerror(-perr));
goto fail;
@@ -267,7 +259,7 @@ static void bt_pan_disconnect(const void *buf, uint16_t len)
{
const struct hal_cmd_pan_disconnect *cmd = buf;
struct pan_device *dev;
- uint8_t status;
+ uint8_t status = HAL_STATUS_FAILED;
GSList *l;
bdaddr_t dst;
@@ -276,20 +268,10 @@ static void bt_pan_disconnect(const void *buf, uint16_t len)
android2bdaddr(&cmd->bdaddr, &dst);
l = g_slist_find_custom(devices, &dst, device_cmp);
- if (!l) {
- status = HAL_STATUS_FAILED;
+ if (!l)
goto failed;
- }
dev = l->data;
-
- if (dev->watch) {
- g_source_remove(dev->watch);
- dev->watch = 0;
- }
-
- bnep_if_down(dev->iface);
- bnep_conndel(&dst);
destroy_pan_device(dev);
status = HAL_STATUS_SUCCESS;
diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index d7d8832..752e00e 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -72,25 +72,15 @@ struct bnep_conn {
uint16_t src;
uint16_t dst;
bdaddr_t dst_addr;
+ char iface[16];
guint attempts;
guint setup_to;
void *data;
- bnep_connect_cb conn_cb;
+ bnep_connect_cb conn_cb;
+ bnep_disconnect_cb disconn_cb;
+ int watch;
};
-static void free_bnep_connect(struct bnep_conn *bc)
-{
- if (!bc)
- return;
-
- if (bc->io) {
- g_io_channel_unref(bc->io);
- bc->io = NULL;
- }
-
- g_free(bc);
-}
-
uint16_t bnep_service_id(const char *svc)
{
int i;
@@ -268,19 +258,16 @@ struct bnep_conn *bnep_new(uint16_t src, uint16_t dst,
return bc;
}
-void bnep_free(struct bnep_conn *bc)
+static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
+ gpointer data)
{
- DBG("");
+ struct bnep_conn *bc = data;
- if (!bc)
- return;
+ DBG("");
- if (bc->io) {
- g_io_channel_unref(bc->io);
- bc->io = NULL;
- }
+ bc->disconn_cb(bc->data);
- g_free(bc);
+ return FALSE;
}
static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,
@@ -290,12 +277,11 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,
struct bnep_control_rsp *rsp;
struct timeval timeo;
char pkt[BNEP_MTU];
- char iface[16];
ssize_t r;
int sk;
if (cond & G_IO_NVAL)
- goto failed;
+ return FALSE;
if (bc->setup_to > 0) {
g_source_remove(bc->setup_to);
@@ -347,24 +333,28 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,
setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
sk = g_io_channel_unix_get_fd(bc->io);
- if (bnep_connadd(sk, bc->src, iface)) {
+ if (bnep_connadd(sk, bc->src, bc->iface)) {
error("bnep conn could not be added");
goto failed;
}
- if (bnep_if_up(iface)) {
- error("could not up %s", iface);
+ if (bnep_if_up(bc->iface)) {
+ error("could not up %s", bc->iface);
+ bnep_conndel(&bc->dst_addr);
goto failed;
}
- bc->conn_cb(chan, iface, 0, bc->data);
- free_bnep_connect(bc);
+ bc->watch = g_io_add_watch(bc->io, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+ bnep_watchdog_cb, bc);
+ g_io_channel_unref(bc->io);
+ bc->io = NULL;
+
+ bc->conn_cb(bc->iface, 0, bc->data);
return FALSE;
failed:
- bc->conn_cb(NULL, NULL, -EIO, bc->data);
- free_bnep_connect(bc);
+ bc->conn_cb(NULL, -EIO, bc->data);
return FALSE;
}
@@ -408,40 +398,72 @@ static gboolean bnep_conn_req_to(gpointer user_data)
return TRUE;
}
- bc->conn_cb(NULL, NULL, -ETIMEDOUT, bc->data);
- free_bnep_connect(bc);
+ bc->conn_cb(NULL, -ETIMEDOUT, bc->data);
return FALSE;
}
-int bnep_connect(int sk, uint16_t src, uint16_t dst, bnep_connect_cb conn_cb,
- void *data)
+int bnep_connect(struct bnep_conn *bc, int sk, bnep_connect_cb conn_cb,
+ bnep_disconnect_cb disconn_cb, void *data)
{
- struct bnep_conn *bc;
int err;
- if (!conn_cb)
+ if (!bc || !conn_cb || !disconn_cb)
return -EINVAL;
- bc = g_new0(struct bnep_conn, 1);
bc->io = g_io_channel_unix_new(sk);
- bc->attempts = 0;
- bc->src = src;
- bc->dst = dst;
- bc->conn_cb = conn_cb;
bc->data = data;
+ bc->conn_cb = conn_cb;
+ bc->disconn_cb = disconn_cb;
err = bnep_setup_conn_req(bc);
if (err < 0)
return err;
bc->setup_to = g_timeout_add_seconds(CON_SETUP_TO,
- bnep_conn_req_to, bc);
- g_io_add_watch(bc->io, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+ bnep_conn_req_to, bc);
+ bc->watch = g_io_add_watch(bc->io,
+ G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
bnep_setup_cb, bc);
return 0;
}
+void bnep_disconnect(struct bnep_conn *bc)
+{
+ DBG("");
+
+ if (!bc)
+ return;
+
+ if (bc->io) {
+ g_io_channel_unref(bc->io);
+ bc->io = NULL;
+ }
+
+ if (bc->watch) {
+ g_source_remove(bc->watch);
+ bc->watch = 0;
+ }
+
+ bnep_if_down(bc->iface);
+ bnep_conndel(&bc->dst_addr);
+}
+
+void bnep_free(struct bnep_conn *bc)
+{
+ DBG("");
+
+ if (!bc)
+ return;
+
+ if (bc->io) {
+ g_io_channel_unref(bc->io);
+ bc->io = NULL;
+ }
+
+ g_free(bc);
+}
+
int bnep_add_to_bridge(const char *devname, const char *bridge)
{
int ifindex;
diff --git a/profiles/network/bnep.h b/profiles/network/bnep.h
index 9c28899..51b09f1 100644
--- a/profiles/network/bnep.h
+++ b/profiles/network/bnep.h
@@ -33,6 +33,11 @@ const char *bnep_name(uint16_t id);
struct bnep_conn *bnep_new(uint16_t src, uint16_t dst,
const bdaddr_t *dst_addr);
void bnep_free(struct bnep_conn *bnep);
+typedef void (*bnep_connect_cb) (char *iface, int err, void *data);
+typedef void (*bnep_disconnect_cb) (void *data);
+int bnep_connect(struct bnep_conn *bnep, int sk, bnep_connect_cb conn_cb,
+ bnep_disconnect_cb disconn_cb, void *data);
+void bnep_disconnect(struct bnep_conn *bnep);
int bnep_connadd(int sk, uint16_t role, char *dev);
int bnep_conndel(const bdaddr_t *dst);
int bnep_if_up(const char *devname);
@@ -40,11 +45,6 @@ int bnep_if_down(const char *devname);
int bnep_add_to_bridge(const char *devname, const char *bridge);
int bnep_del_from_bridge(const char *devname, const char *bridge);
-typedef void (*bnep_connect_cb) (GIOChannel *chan, char *iface, int err,
- void *data);
-int bnep_connect(int sk, uint16_t src, uint16_t dst, bnep_connect_cb conn_cb,
- void *data);
-
ssize_t bnep_send_ctrl_rsp(int sk, uint8_t type, uint8_t ctrl, uint16_t resp);
uint16_t bnep_setup_chk(uint16_t dst_role, uint16_t src_role);
uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req, uint16_t *dst,
diff --git a/profiles/network/connection.c b/profiles/network/connection.c
index fb3e1ce..c7cc99f 100644
--- a/profiles/network/connection.c
+++ b/profiles/network/connection.c
@@ -72,6 +72,7 @@ struct network_conn {
guint dc_id;
struct network_peer *peer;
DBusMessage *connect;
+ struct bnep_conn *bnep;
};
static GSList *peers = NULL;
@@ -106,8 +107,7 @@ static struct network_conn *find_connection_by_state(GSList *list,
return NULL;
}
-static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
- gpointer data)
+static void bnep_disconn_cb(void *data)
{
struct network_conn *nc = data;
DBusConnection *conn = btd_get_dbus_connection();
@@ -126,12 +126,18 @@ static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
info("%s disconnected", nc->dev);
- bnep_if_down(nc->dev);
nc->state = DISCONNECTED;
memset(nc->dev, 0, sizeof(nc->dev));
strcpy(nc->dev, "bnep%d");
- return FALSE;
+ bnep_free(nc->bnep);
+ nc->bnep = NULL;
+
+ if (nc->io) {
+ g_io_channel_shutdown(nc->io, TRUE, NULL);
+ g_io_channel_unref(nc->io);
+ nc->io = NULL;
+ }
}
static void local_connect_cb(struct network_conn *nc, int err)
@@ -158,9 +164,17 @@ static void cancel_connection(struct network_conn *nc, int err)
if (nc->connect)
local_connect_cb(nc, err);
- g_io_channel_shutdown(nc->io, TRUE, NULL);
- g_io_channel_unref(nc->io);
- nc->io = NULL;
+ if (nc->io) {
+ g_io_channel_shutdown(nc->io, TRUE, NULL);
+ g_io_channel_unref(nc->io);
+ nc->io = NULL;
+ }
+
+ if (nc->state == CONNECTED)
+ bnep_disconnect(nc->bnep);
+
+ bnep_free(nc->bnep);
+ nc->bnep = NULL;
nc->state = DISCONNECTED;
}
@@ -169,11 +183,7 @@ static void connection_destroy(DBusConnection *conn, void *user_data)
{
struct network_conn *nc = user_data;
- if (nc->state == CONNECTED) {
- bnep_if_down(nc->dev);
- bnep_conndel(device_get_address(nc->peer->device));
- } else if (nc->io)
- cancel_connection(nc, -EIO);
+ cancel_connection(nc, -EIO);
}
static void disconnect_cb(struct btd_device *device, gboolean removal,
@@ -186,7 +196,7 @@ static void disconnect_cb(struct btd_device *device, gboolean removal,
connection_destroy(NULL, user_data);
}
-static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
+static void bnep_conn_cb(char *iface, int err, void *data)
{
struct network_conn *nc = data;
const char *path;
@@ -220,11 +230,6 @@ static void bnep_conn_cb(GIOChannel *chan, char *iface, int err, void *data)
nc->state = CONNECTED;
nc->dc_id = device_add_disconnect_watch(nc->peer->device, disconnect_cb,
nc, NULL);
- g_io_add_watch(chan, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
- bnep_watchdog_cb, nc);
- g_io_channel_unref(nc->io);
- nc->io = NULL;
-
return;
failed:
@@ -242,7 +247,14 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer data)
}
sk = g_io_channel_unix_get_fd(nc->io);
- perr = bnep_connect(sk, BNEP_SVC_PANU, nc->id, bnep_conn_cb, nc);
+ nc->bnep = bnep_new(BNEP_SVC_PANU, nc->id,
+ device_get_address(nc->peer->device));
+ if (!nc->bnep) {
+ error("unable to allocate memory for nc->bnep");
+ goto failed;
+ }
+
+ perr = bnep_connect(nc->bnep, sk, bnep_conn_cb, bnep_disconn_cb, nc);
if (perr < 0) {
error("bnep connect(): %s (%d)", strerror(-perr), -perr);
goto failed;
--
1.8.3.2
^ permalink raw reply related
* [PATCH_v2 5/6] bnep: Refactored bnep server apis for bridge addition and deletion
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
In-Reply-To: <1387231516-4127-1-git-send-email-ravikumar.veeramally@linux.intel.com>
To simplify bnep server realted bridge creation and deletion calls
provided extra apis and moved related apis to static.
---
profiles/network/bnep.c | 50 +++++++++++++++++++++++++++++++++++++++++------
profiles/network/bnep.h | 10 ++++------
profiles/network/server.c | 37 +++--------------------------------
3 files changed, 51 insertions(+), 46 deletions(-)
diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 752e00e..5deac81 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -151,7 +151,7 @@ int bnep_cleanup(void)
return 0;
}
-int bnep_conndel(const bdaddr_t *dst)
+static int bnep_conndel(const bdaddr_t *dst)
{
struct bnep_conndel_req req;
@@ -167,7 +167,7 @@ int bnep_conndel(const bdaddr_t *dst)
return 0;
}
-int bnep_connadd(int sk, uint16_t role, char *dev)
+static int bnep_connadd(int sk, uint16_t role, char *dev)
{
struct bnep_connadd_req req;
@@ -187,7 +187,7 @@ int bnep_connadd(int sk, uint16_t role, char *dev)
return 0;
}
-int bnep_if_up(const char *devname)
+static int bnep_if_up(const char *devname)
{
struct ifreq ifr;
int sk, err;
@@ -212,7 +212,7 @@ int bnep_if_up(const char *devname)
return 0;
}
-int bnep_if_down(const char *devname)
+static int bnep_if_down(const char *devname)
{
struct ifreq ifr;
int sk, err;
@@ -464,7 +464,7 @@ void bnep_free(struct bnep_conn *bc)
g_free(bc);
}
-int bnep_add_to_bridge(const char *devname, const char *bridge)
+static int bnep_add_to_bridge(const char *devname, const char *bridge)
{
int ifindex;
struct ifreq ifr;
@@ -495,7 +495,7 @@ int bnep_add_to_bridge(const char *devname, const char *bridge)
return 0;
}
-int bnep_del_from_bridge(const char *devname, const char *bridge)
+static int bnep_del_from_bridge(const char *devname, const char *bridge)
{
int ifindex = if_nametoindex(devname);
struct ifreq ifr;
@@ -601,3 +601,41 @@ uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req, uint16_t *dst,
return BNEP_SUCCESS;
}
+
+int bnep_server_add(int sk, uint16_t dst, char *bridge, char *iface,
+ const bdaddr_t *addr)
+{
+ if (!bridge || !bridge || !iface || !addr)
+ return -EINVAL;
+
+ if (bnep_connadd(sk, dst, iface) < 0) {
+ error("Can't add connection to the bridge %s: %s(%d)",
+ bridge, strerror(errno), errno);
+ return -errno;
+ }
+
+ if (bnep_add_to_bridge(iface, bridge) < 0) {
+ error("Can't add %s to the bridge %s: %s(%d)",
+ iface, bridge, strerror(errno), errno);
+ bnep_conndel(addr);
+ return -errno;
+ }
+
+ if (bnep_if_up(iface) < 0) {
+ error("Can't up the interface %s: %s(%d)",
+ iface, strerror(errno), errno);
+ return -errno;
+ }
+
+ return 0;
+}
+
+void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr)
+{
+ if (!bridge || !iface || !addr)
+ return;
+
+ bnep_del_from_bridge(iface, bridge);
+ bnep_if_down(iface);
+ bnep_conndel(addr);
+}
diff --git a/profiles/network/bnep.h b/profiles/network/bnep.h
index 51b09f1..734055d 100644
--- a/profiles/network/bnep.h
+++ b/profiles/network/bnep.h
@@ -38,12 +38,10 @@ typedef void (*bnep_disconnect_cb) (void *data);
int bnep_connect(struct bnep_conn *bnep, int sk, bnep_connect_cb conn_cb,
bnep_disconnect_cb disconn_cb, void *data);
void bnep_disconnect(struct bnep_conn *bnep);
-int bnep_connadd(int sk, uint16_t role, char *dev);
-int bnep_conndel(const bdaddr_t *dst);
-int bnep_if_up(const char *devname);
-int bnep_if_down(const char *devname);
-int bnep_add_to_bridge(const char *devname, const char *bridge);
-int bnep_del_from_bridge(const char *devname, const char *bridge);
+
+int bnep_server_add(int sk, uint16_t dst, char *bridge, char *iface,
+ const bdaddr_t *addr);
+void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr);
ssize_t bnep_send_ctrl_rsp(int sk, uint8_t type, uint8_t ctrl, uint16_t resp);
uint16_t bnep_setup_chk(uint16_t dst_role, uint16_t src_role);
diff --git a/profiles/network/server.c b/profiles/network/server.c
index 73741ec..7cb5a1e 100644
--- a/profiles/network/server.c
+++ b/profiles/network/server.c
@@ -251,35 +251,6 @@ static sdp_record_t *server_record_new(const char *name, uint16_t id)
return record;
}
-static int server_connadd(struct network_server *ns,
- struct network_session *session,
- uint16_t dst_role)
-{
- char devname[16];
- int err, nsk;
-
- nsk = g_io_channel_unix_get_fd(session->io);
- err = bnep_connadd(nsk, dst_role, devname);
- if (err < 0)
- return err;
-
- info("Added new connection: %s", devname);
-
- if (bnep_add_to_bridge(devname, ns->bridge) < 0) {
- error("Can't add %s to the bridge %s: %s(%d)",
- devname, ns->bridge, strerror(errno), errno);
- return -EPERM;
- }
-
- bnep_if_up(devname);
-
- strncpy(session->dev, devname, sizeof(devname));
-
- ns->sessions = g_slist_append(ns->sessions, session);
-
- return 0;
-}
-
static void session_free(void *data)
{
struct network_session *session = data;
@@ -377,7 +348,8 @@ static gboolean bnep_setup(GIOChannel *chan,
goto reply;
}
- if (server_connadd(ns, na->setup, dst_role) < 0)
+ if (bnep_server_add(sk, dst_role, ns->bridge, na->setup->dev,
+ &na->setup->dst) < 0)
goto reply;
na->setup = NULL;
@@ -524,10 +496,7 @@ static void server_remove_sessions(struct network_server *ns)
if (*session->dev == '\0')
continue;
- bnep_del_from_bridge(session->dev, ns->bridge);
- bnep_if_down(session->dev);
-
- bnep_conndel(&session->dst);
+ bnep_server_delete(ns->bridge, session->dev, &session->dst);
}
g_slist_free_full(ns->sessions, session_free);
--
1.8.3.2
^ permalink raw reply related
* [PATCH_v2 6/6] bnep: Refactor bnep setup response validation functionality
From: Ravi kumar Veeramally @ 2013-12-16 22:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ravi kumar Veeramally
In-Reply-To: <1387231516-4127-1-git-send-email-ravikumar.veeramally@linux.intel.com>
Providing single api to validate bnep setup resp and hide
other functions.
---
profiles/network/bnep.c | 49 ++++++++++++++++++++++++++++++++++++++++++-----
profiles/network/bnep.h | 4 +---
profiles/network/server.c | 46 +++++++++-----------------------------------
3 files changed, 54 insertions(+), 45 deletions(-)
diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 5deac81..6e1af74 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -535,19 +535,19 @@ ssize_t bnep_send_ctrl_rsp(int sk, uint8_t type, uint8_t ctrl, uint16_t resp)
return send(sk, &rsp, sizeof(rsp), 0);
}
-uint16_t bnep_setup_chk(uint16_t dst, uint16_t src)
+static uint16_t bnep_setup_chk(uint16_t dst, uint16_t src)
{
/* Allowed PAN Profile scenarios */
switch (dst) {
case BNEP_SVC_NAP:
case BNEP_SVC_GN:
if (src == BNEP_SVC_PANU)
- return 0;
+ return BNEP_SUCCESS;
return BNEP_CONN_INVALID_SRC;
case BNEP_SVC_PANU:
if (src == BNEP_SVC_PANU || src == BNEP_SVC_GN ||
src == BNEP_SVC_NAP)
- return 0;
+ return BNEP_SUCCESS;
return BNEP_CONN_INVALID_SRC;
}
@@ -555,8 +555,8 @@ uint16_t bnep_setup_chk(uint16_t dst, uint16_t src)
return BNEP_CONN_INVALID_DST;
}
-uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req, uint16_t *dst,
- uint16_t *src)
+static uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req,
+ uint16_t *dst, uint16_t *src)
{
const uint8_t bt_base[] = { 0x00, 0x00, 0x10, 0x00, 0x80, 0x00,
0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB };
@@ -602,6 +602,45 @@ uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req, uint16_t *dst,
return BNEP_SUCCESS;
}
+int bnep_validate_setup_rsp(int sk, uint16_t *dst)
+{
+ uint8_t packet[BNEP_MTU];
+ struct bnep_setup_conn_req *req = (void *) packet;
+ uint16_t src;
+ uint8_t pkt[3];
+ int n, rsp = BNEP_CONN_NOT_ALLOWED;
+
+ /* Reading BNEP_SETUP_CONNECTION_REQUEST_MSG */
+ n = read(sk, packet, sizeof(packet));
+ if (n < 0) {
+ error("read(): %s(%d)", strerror(errno), errno);
+ return n;
+ }
+
+ /* Highest known Control command ID
+ * is BNEP_FILTER_MULT_ADDR_RSP = 0x06 */
+ if (req->type == BNEP_CONTROL &&
+ req->ctrl > BNEP_FILTER_MULT_ADDR_RSP) {
+ pkt[0] = BNEP_CONTROL;
+ pkt[1] = BNEP_CMD_NOT_UNDERSTOOD;
+ pkt[2] = req->ctrl;
+
+ send(sk, pkt, sizeof(pkt), 0);
+ return -EINVAL;
+ }
+
+ if (req->type != BNEP_CONTROL || req->ctrl != BNEP_SETUP_CONN_REQ)
+ return -EINVAL;
+
+ rsp = bnep_setup_decode(req, dst, &src);
+ if (rsp)
+ return rsp;
+
+ rsp = bnep_setup_chk(*dst, src);
+
+ return rsp;
+}
+
int bnep_server_add(int sk, uint16_t dst, char *bridge, char *iface,
const bdaddr_t *addr)
{
diff --git a/profiles/network/bnep.h b/profiles/network/bnep.h
index 734055d..36cfee0 100644
--- a/profiles/network/bnep.h
+++ b/profiles/network/bnep.h
@@ -43,7 +43,5 @@ int bnep_server_add(int sk, uint16_t dst, char *bridge, char *iface,
const bdaddr_t *addr);
void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr);
+int bnep_validate_setup_rsp(int sk, uint16_t *dst);
ssize_t bnep_send_ctrl_rsp(int sk, uint8_t type, uint8_t ctrl, uint16_t resp);
-uint16_t bnep_setup_chk(uint16_t dst_role, uint16_t src_role);
-uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req, uint16_t *dst,
- uint16_t *src);
diff --git a/profiles/network/server.c b/profiles/network/server.c
index 7cb5a1e..432b6d5 100644
--- a/profiles/network/server.c
+++ b/profiles/network/server.c
@@ -282,10 +282,8 @@ static gboolean bnep_setup(GIOChannel *chan,
{
struct network_adapter *na = user_data;
struct network_server *ns;
- uint8_t packet[BNEP_MTU];
- struct bnep_setup_conn_req *req = (void *) packet;
- uint16_t src_role, dst_role, rsp = BNEP_CONN_NOT_ALLOWED;
- int n, sk;
+ uint16_t dst;
+ int sk, rsp = BNEP_CONN_NOT_ALLOWED;
if (cond & G_IO_NVAL)
return FALSE;
@@ -296,45 +294,18 @@ static gboolean bnep_setup(GIOChannel *chan,
}
sk = g_io_channel_unix_get_fd(chan);
-
- /* Reading BNEP_SETUP_CONNECTION_REQUEST_MSG */
- n = read(sk, packet, sizeof(packet));
- if (n < 0) {
- error("read(): %s(%d)", strerror(errno), errno);
- return FALSE;
- }
-
- /* Highest known Control command ID
- * is BNEP_FILTER_MULT_ADDR_RSP = 0x06 */
- if (req->type == BNEP_CONTROL &&
- req->ctrl > BNEP_FILTER_MULT_ADDR_RSP) {
- uint8_t pkt[3];
-
- pkt[0] = BNEP_CONTROL;
- pkt[1] = BNEP_CMD_NOT_UNDERSTOOD;
- pkt[2] = req->ctrl;
-
- send(sk, pkt, sizeof(pkt), 0);
-
- return FALSE;
- }
-
- if (req->type != BNEP_CONTROL || req->ctrl != BNEP_SETUP_CONN_REQ)
+ rsp = bnep_validate_setup_rsp(sk, &dst);
+ if (rsp < 0)
return FALSE;
- rsp = bnep_setup_decode(req, &dst_role, &src_role);
- if (rsp)
- goto reply;
-
- rsp = bnep_setup_chk(dst_role, src_role);
- if (rsp)
+ if (rsp > 0)
goto reply;
rsp = BNEP_CONN_NOT_ALLOWED;
- ns = find_server(na->servers, dst_role);
+ ns = find_server(na->servers, dst);
if (!ns) {
- error("Server unavailable: (0x%x)", dst_role);
+ error("Server unavailable: (0x%x)", dst);
goto reply;
}
@@ -348,10 +319,11 @@ static gboolean bnep_setup(GIOChannel *chan,
goto reply;
}
- if (bnep_server_add(sk, dst_role, ns->bridge, na->setup->dev,
+ if (bnep_server_add(sk, dst, ns->bridge, na->setup->dev,
&na->setup->dst) < 0)
goto reply;
+ ns->sessions = g_slist_append(ns->sessions, na->setup);
na->setup = NULL;
rsp = BNEP_SUCCESS;
--
1.8.3.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox