* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
From: Marcel Holtmann @ 2012-09-06 20:10 UTC (permalink / raw)
To: Mat Martineau; +Cc: Andrei Emeltchenko, linux-bluetooth, gustavo
In-Reply-To: <alpine.DEB.2.02.1209060946190.22884@mathewm-linux>
Hi Mat,
> > If we have unacked frames when closing bluetooth socket we deadlock
> > since conn->chan_lock, chan->lock and socket lock are taken. Remove
> > __l2cap_wait_ack completely.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> I don't think you want to remove this code completely, at least not
> without giving some thought to the problem it is solving.
>
> The problem is that programs may have an open socket which they send
> some data on, then immediately close. There is no feedback when data
> is actually sent over the air, so the socket may end up getting torn
> down while there is still data in the HCI tx buffer or some data was
> lost and needs to be retransmitted. Waiting for an acknowledgement
> confirms that the application's sent data made it to the remote
> device.
>
> Without this code, it's difficult to use l2test on a number of
> qualification tests. Profiles or applications using ERTM may depend
> on the "wait for ack before closing" behavior in order to have a clean
> disconnect.
isn't that what we have SO_LINGER for?
Regards
Marcel
^ permalink raw reply
* [PATCH BlueZ v0] gatt: Exchange MTU needs to be executed first
From: Claudio Takahasi @ 2012-09-06 20:37 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
According to Bluetooth SPEC the ATT Exchange MTU sub-procedure needs to
be execute before any ATT command. This patch moves the Exchange MTU
sub-procedure from GATT plugin to device ATTIO core.
---
*** Apply after "LE General Connection Establishment procedure" patches
profiles/gatt/gas.c | 34 ------------------------------
src/device.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 52 insertions(+), 42 deletions(-)
diff --git a/profiles/gatt/gas.c b/profiles/gatt/gas.c
index ddd4e70..2a3a1b3 100644
--- a/profiles/gatt/gas.c
+++ b/profiles/gatt/gas.c
@@ -254,46 +254,12 @@ static void gatt_characteristic_cb(GSList *characteristics, guint8 status,
gatt_find_info(gas->attrib, start, end, gatt_descriptors_cb, gas);
}
-static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
- gpointer user_data)
-{
- struct gas *gas = user_data;
- uint16_t rmtu;
-
- if (status) {
- error("MTU exchange: %s", att_ecode2str(status));
- return;
- }
-
- if (!dec_mtu_resp(pdu, plen, &rmtu)) {
- error("MTU exchange: protocol error");
- return;
- }
-
- gas->mtu = MIN(rmtu, gas->mtu);
- if (g_attrib_set_mtu(gas->attrib, gas->mtu))
- DBG("MTU exchange succeeded: %d", gas->mtu);
- else
- DBG("MTU exchange failed");
-}
-
static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
{
struct gas *gas = user_data;
- GIOChannel *io;
- GError *gerr = NULL;
- uint16_t cid, imtu;
uint16_t app;
gas->attrib = g_attrib_ref(attrib);
- io = g_attrib_get_channel(attrib);
-
- if (bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu,
- BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
- gatt_exchange_mtu(gas->attrib, imtu, exchange_mtu_cb, gas);
- gas->mtu = imtu;
- DBG("MTU Exchange: Requesting %d", imtu);
- }
gas->changed_ind = g_attrib_register(gas->attrib, ATT_OP_HANDLE_IND,
indication_cb, gas, NULL);
diff --git a/src/device.c b/src/device.c
index 81cd3b1..26ab891 100644
--- a/src/device.c
+++ b/src/device.c
@@ -167,6 +167,7 @@ struct btd_device {
GIOChannel *att_io;
guint cleanup_id;
+ uint16_t att_mtu;
};
static GSList *profiles = NULL;
@@ -1962,7 +1963,8 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
if (attcb->error)
attcb->error(gerr, user_data);
- goto done;
+ g_free(attcb);
+ return;
}
attrib = g_attrib_new(io);
@@ -1990,9 +1992,6 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
bonding_request_free(device->bonding);
}
}
-
-done:
- g_free(attcb);
}
static void att_error_cb(const GError *gerr, gpointer user_data)
@@ -2010,15 +2009,60 @@ static void att_error_cb(const GError *gerr, gpointer user_data)
DBG("Enabling automatic connections");
}
-static void att_success_cb(gpointer user_data)
+static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
+ gpointer user_data)
{
struct att_callbacks *attcb = user_data;
struct btd_device *device = attcb->user_data;
+ uint16_t rmtu;
- if (device->attios == NULL)
- return;
+ if (status) {
+ error("MTU exchange: %s", att_ecode2str(status));
+ goto done;
+ }
+
+ if (!dec_mtu_resp(pdu, plen, &rmtu)) {
+ error("MTU exchange: protocol error");
+ goto done;
+ }
+
+ device->att_mtu = MIN(rmtu, device->att_mtu);
+ if (g_attrib_set_mtu(device->attrib, device->att_mtu))
+ DBG("MTU exchange succeeded: %d", device->att_mtu);
+ else
+ DBG("MTU exchange failed");
g_slist_foreach(device->attios, attio_connected, device->attrib);
+
+done:
+ g_free(attcb);
+}
+
+static void exchange_mtu(gpointer user_data)
+{
+ struct att_callbacks *attcb = user_data;
+ struct btd_device *device = attcb->user_data;
+ GIOChannel *io;
+ uint16_t cid, imtu;
+
+ if (device->attios == NULL) {
+ g_free(attcb);
+ return;
+ }
+
+ /* Starting ATT MTU Exchange */
+ io = g_attrib_get_channel(device->attrib);
+ if (bt_io_get(io, NULL, BT_IO_OPT_IMTU, &imtu,
+ BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
+ gatt_exchange_mtu(device->attrib, imtu, exchange_mtu_cb,
+ attcb);
+ device->att_mtu = imtu;
+ DBG("MTU Exchange: Requesting %d", imtu);
+ } else {
+ g_slist_foreach(device->attios, attio_connected,
+ device->attrib);
+ g_free(attcb);
+ }
}
GIOChannel *device_att_connect(gpointer user_data)
@@ -2038,7 +2082,7 @@ GIOChannel *device_att_connect(gpointer user_data)
attcb = g_new0(struct att_callbacks, 1);
attcb->error = att_error_cb;
- attcb->success = att_success_cb;
+ attcb->success = exchange_mtu;
attcb->user_data = device;
if (device_is_bredr(device)) {
--
1.7.12
^ permalink raw reply related
* Re: [PATCH BlueZ v3 06/15] gdbus: Implement DBus.Properties.Set method
From: Luiz Augusto von Dentz @ 2012-09-06 21:31 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Lucas De Marchi, linux-bluetooth
In-Reply-To: <1346961700.21200.116.camel@aeonflux>
Hi Marcel,
On Thu, Sep 6, 2012 at 11:01 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Lucas,
>
>> Contrary to Get() and GetAll(), Set is asynchronous so we pass on the
>> DBusMessage so user is able to create the response. It's the only use of
>> this parameter.
>> ---
>> gdbus/gdbus.h | 7 +++++++
>> gdbus/object.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>> index b2e78c4..3e4aa16 100644
>> --- a/gdbus/gdbus.h
>> +++ b/gdbus/gdbus.h
>> @@ -31,6 +31,8 @@ extern "C" {
>> #include <dbus/dbus.h>
>> #include <glib.h>
>>
>> +typedef enum GDBusPropertySetReturn GDBusPropertySetReturn;
>> +
>> typedef enum GDBusMethodFlags GDBusMethodFlags;
>> typedef enum GDBusSignalFlags GDBusSignalFlags;
>> typedef enum GDBusPropertyFlags GDBusPropertyFlags;
>> @@ -69,6 +71,10 @@ typedef DBusMessage * (* GDBusMethodFunction) (DBusConnection *connection,
>> typedef gboolean (*GDBusPropertyGetter)(const GDBusPropertyTable *property,
>> DBusMessageIter *iter, void *data);
>>
>> +typedef DBusMessage *(*GDBusPropertySetter)(const GDBusPropertyTable *property,
>> + DBusMessageIter *value,
>> + DBusMessage *msg, void *data);
>> +
>
> I am not really happy with this. We just need a unique handle here since
> the return value is either success or an error. I rather don't send
> messages around for no other reason to create the error.
>
> Inside the authorization code I am using GDBusPendingReply as unique
> token. What about using the same approach?
It would be perfect, but Im afraid sometimes the properties can have
restriction on which sender can set them, so perhaps we need to add
the name of the sender as parameter in addiction to just the handle?
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ v3 06/15] gdbus: Implement DBus.Properties.Set method
From: Marcel Holtmann @ 2012-09-07 5:49 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Lucas De Marchi, linux-bluetooth
In-Reply-To: <CABBYNZLZJTcw1Swp-Le25+F8fAweTDy=733PTVffc4sRJbFikQ@mail.gmail.com>
Hi Luiz,
> >> Contrary to Get() and GetAll(), Set is asynchronous so we pass on the
> >> DBusMessage so user is able to create the response. It's the only use of
> >> this parameter.
> >> ---
> >> gdbus/gdbus.h | 7 +++++++
> >> gdbus/object.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >> 2 files changed, 64 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
> >> index b2e78c4..3e4aa16 100644
> >> --- a/gdbus/gdbus.h
> >> +++ b/gdbus/gdbus.h
> >> @@ -31,6 +31,8 @@ extern "C" {
> >> #include <dbus/dbus.h>
> >> #include <glib.h>
> >>
> >> +typedef enum GDBusPropertySetReturn GDBusPropertySetReturn;
> >> +
> >> typedef enum GDBusMethodFlags GDBusMethodFlags;
> >> typedef enum GDBusSignalFlags GDBusSignalFlags;
> >> typedef enum GDBusPropertyFlags GDBusPropertyFlags;
> >> @@ -69,6 +71,10 @@ typedef DBusMessage * (* GDBusMethodFunction) (DBusConnection *connection,
> >> typedef gboolean (*GDBusPropertyGetter)(const GDBusPropertyTable *property,
> >> DBusMessageIter *iter, void *data);
> >>
> >> +typedef DBusMessage *(*GDBusPropertySetter)(const GDBusPropertyTable *property,
> >> + DBusMessageIter *value,
> >> + DBusMessage *msg, void *data);
> >> +
> >
> > I am not really happy with this. We just need a unique handle here since
> > the return value is either success or an error. I rather don't send
> > messages around for no other reason to create the error.
> >
> > Inside the authorization code I am using GDBusPendingReply as unique
> > token. What about using the same approach?
>
> It would be perfect, but Im afraid sometimes the properties can have
> restriction on which sender can set them, so perhaps we need to add
> the name of the sender as parameter in addiction to just the handle?
that should be done through the same security handling we have for the
method calls. Don't try to invent something new.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] gatt: Remove reading Service Changed characteristic after connected
From: Andrzej Kaczmarek @ 2012-09-07 7:49 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CAKT1EBdHL8q-sm=ibqZFoTFYtzeNTVHjB2wvoFBS=zHrb7xyxQ@mail.gmail.com>
Hi Claudio,
On 09/06/2012 03:56 PM, Claudio Takahasi wrote:
> Hi Andrzej:
>
> On Thu, Sep 6, 2012 at 7:53 AM, Andrzej Kaczmarek
> <andrzej.kaczmarek@tieto.com> wrote:
<snip>
>> static void gatt_descriptors_cb(guint8 status, const guint8 *pdu, guint16 len,
>> gpointer user_data)
>> {
>> @@ -311,8 +280,6 @@ static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
>> /* TODO: Read other GAP characteristics - See Core spec page 1739 */
>>
>> /*
>> - * Always read the characteristic value in the first connection
>> - * since attribute handles caching is not supported at the moment.
>> * When re-connecting <<Service Changed>> handle and characteristic
>> * value doesn't need to read again: known information from the
>> * previous interaction.
>> @@ -322,10 +289,6 @@ static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
>>
>> bt_uuid16_create(&uuid, GATT_CHARAC_SERVICE_CHANGED);
>>
>> - gatt_read_char_by_uuid(gas->attrib, gas->gatt.start,
>> - gas->gatt.end, &uuid,
>> - gatt_service_changed_cb, gas);
>> -
>> gatt_discover_char(gas->attrib, gas->gatt.start, gas->gatt.end,
>> &uuid, gatt_characteristic_cb, gas);
>> }
>> --
>> 1.7.11.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> As discussed in the IRC, please add in the commit message the BT SPEC
> errata information/section.
>
> Since we can't read the characteristic value, it will be necessary to
> store the handle, otherwise BlueZ will loose the Service Changed
> Indication when re-connecting.
Handle is already stored during characteristics discovery
(gatt_characteristic_cb) so this should not be a problem.
BR,
Andrzej
^ permalink raw reply
* Re: [PATCH 1/2] Implement broadcom patchram firmware loader
From: Jesse Sung @ 2012-09-07 8:07 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1346429464.23377.19.camel@aeonflux>
Hi Marcel,
2012/9/1 Marcel Holtmann <marcel@holtmann.org>:
> Hi Jesse,
>
>> From: Wen-chien Jesse Sung <jesse.sung@canonical.com>
>>
>>
>> Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
>
> please learn on how to write commit messages. I want a full blown commit
> messages with /sys/kernel/debug/usb/devices output (before and after)
> and something that explains what is actually done.
Sorry, I'll try to do that better next time.
>> ---
>> drivers/bluetooth/btusb.c | 103 +++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 99 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 654e248..7189fed 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -23,6 +23,8 @@
>>
>> #include <linux/module.h>
>> #include <linux/usb.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -47,6 +49,7 @@ static struct usb_driver btusb_driver;
>> #define BTUSB_BROKEN_ISOC 0x20
>> #define BTUSB_WRONG_SCO_MTU 0x40
>> #define BTUSB_ATH3012 0x80
>> +#define BTUSB_BCM_PATCHRAM 0x100
>>
>> static struct usb_device_id btusb_table[] = {
>> /* Generic Bluetooth USB device */
>> @@ -96,14 +99,15 @@ static struct usb_device_id btusb_table[] = {
>> { USB_DEVICE(0x0c10, 0x0000) },
>>
>> /* Broadcom BCM20702A0 */
>> + { USB_DEVICE(0x0489, 0xe031), .driver_info = BTUSB_BCM_PATCHRAM },
>> { USB_DEVICE(0x0489, 0xe042) },
>> - { USB_DEVICE(0x413c, 0x8197) },
>> + { USB_DEVICE(0x413c, 0x8197), .driver_info = BTUSB_BCM_PATCHRAM },
>
> These drivers did work without firmware before. So why is this change
> required? Or is that Broadcom wide?
Currently patchram applies to BCM43142 and BCM20702 modules. Dunno if
there will be
more or not.
Some of these modules have PROM or something like that holding the
needed images.
For them, adding IDs to this table is enough for them to work. But as
what we have found,
these images may have bugs and need an updated image to be loaded to
fix the bugs.
Since failed to load firmware may not trigger an error while doing
probe, these kind
of modules will still work even if there's no firmware provided.
For other modules that do not have images with them, patchram is
needed to bring them
into a working state.
>> /* Foxconn - Hon Hai */
>> { USB_DEVICE(0x0489, 0xe033) },
>>
>> /*Broadcom devices with vendor specific id */
>> - { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) },
>> + { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01), .driver_info = BTUSB_BCM_PATCHRAM },
>>
>> { } /* Terminating entry */
>> };
>> @@ -197,6 +201,37 @@ static struct usb_device_id blacklist_table[] = {
>> { } /* Terminating entry */
>> };
>>
>> +#define PATCHRAM_TIMEOUT 1000
>> +#define FW_0489_E031 "fw-0489_e031.hcd"
>> +#define FW_0A5C_21D3 "fw-0a5c_21d3.hcd"
>> +#define FW_0A5C_21D7 "fw-0a5c_21d7.hcd"
>> +#define FW_0A5C_21E6 "fw-0a5c_21e6.hcd"
>> +#define FW_0A5C_21F3 "fw-0a5c_21f3.hcd"
>> +#define FW_0A5C_21F4 "fw-0a5c_21f4.hcd"
>> +#define FW_413C_8197 "fw-413c_8197.hcd"
>> +
>> +MODULE_FIRMWARE(FW_0489_E031);
>> +MODULE_FIRMWARE(FW_0A5C_21D3);
>> +MODULE_FIRMWARE(FW_0A5C_21D7);
>> +MODULE_FIRMWARE(FW_0A5C_21E6);
>> +MODULE_FIRMWARE(FW_0A5C_21F3);
>> +MODULE_FIRMWARE(FW_0A5C_21F4);
>> +MODULE_FIRMWARE(FW_413C_8197);
>> +
>> +static struct usb_device_id patchram_table[] = {
>> + /* Dell DW1704 */
>> + { USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) FW_0A5C_21D3 },
>> + { USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) FW_0A5C_21D7 },
>> + /* Dell DW380 */
>> + { USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) FW_413C_8197 },
>> + /* FoxConn Hon Hai */
>> + { USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) FW_0489_E031 },
>> + /* Lenovo */
>> + { USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) FW_0A5C_21E6 },
>> + { USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) FW_0A5C_21F3 },
>> + { USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) FW_0A5C_21F4 },
>> +};
>> +
>
> And this looks like totally wasted details to me. Either build the
> firmware name from USB VID:PID or only include the ones that we are
> actually supporting.
I'll get the firmware name from VID and PID instead.
>> #define BTUSB_MAX_ISOC_FRAMES 10
>>
>> #define BTUSB_INTR_RUNNING 0
>> @@ -914,6 +949,55 @@ static void btusb_waker(struct work_struct *work)
>> usb_autopm_put_interface(data->intf);
>> }
>>
>> +static inline void load_patchram_fw(struct usb_device *udev, const struct usb_device_id *id)
>> +{
>> + size_t pos = 0;
>> + int err = 0;
>> + const struct firmware *fw;
>> +
>> + unsigned char reset_cmd[] = { 0x03, 0x0c, 0x00 };
>> + unsigned char download_cmd[] = { 0x2e, 0xfc, 0x00 };
>> +
>> + if (request_firmware(&fw, (const char *) id->driver_info, &udev->dev) < 0) {
>> + BT_INFO("can't load firmware, may not work correctly");
>> + return;
>> + }
>> +
>> + if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> + reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0) {
>> + err = -1;
>> + goto out;
>> + }
>> + msleep(300);
>> +
>> + if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> + download_cmd, sizeof(download_cmd), PATCHRAM_TIMEOUT) < 0) {
>> + err = -1;
>> + goto out;
>> + }
>> + msleep(300);
>> +
>> + while (pos < fw->size) {
>> + size_t len;
>> + len = fw->data[pos + 2] + 3;
>> + if ((pos + len > fw->size) ||
>> + (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0,
>> + USB_TYPE_CLASS, 0, 0, (void *)fw->data + pos, len,
>> + PATCHRAM_TIMEOUT) < 0)) {
>> + err = -1;
>> + goto out;
>> + }
>> + pos += len;
>> + }
>> +
>> + err = (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> + reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0);
>> +out:
>> + if (err)
>> + BT_INFO("fail to load firmware, may not work correctly");
>> + release_firmware(fw);
>> +}
>> +
>> static int btusb_probe(struct usb_interface *intf,
>> const struct usb_device_id *id)
>> {
>> @@ -1078,15 +1162,26 @@ static int btusb_probe(struct usb_interface *intf,
>> }
>> }
>>
>> + usb_set_intfdata(intf, data);
>> +
>> + if (id->driver_info & BTUSB_BCM_PATCHRAM) {
>> + const struct usb_device_id *match;
>> + match = usb_match_id(intf, patchram_table);
>> + if (match) {
>> + btusb_open(hdev);
>> + load_patchram_fw(interface_to_usbdev(intf), match);
>> + btusb_close(hdev);
>> + }
>> + }
>> +
>
> So we are now blocking every other USB devices on that bus here? I
> actually do not like this idea very much.
Do you mean the usleep() in load_patchram_fw()? These only affects people
who have this kind of device, so the impact should be limited.
> Also the call of btusb_open() before hdev is actually registered is
> kinda fishy to me. I am not even sure that works how you think it would.
Humm.. If there are concerns about calling btusb_open() and btusb_close(),
I'll try to find another way to hook usb callbacks.
> And why can't Broadcom just change the PID once the patchram has been
> loaded to something else. That way we can nicely iterate through this.
Unfortunately that's how they do it now, and these modules are already in
users' machines.
> This also does not really belong in a standard driver. Quirks fine, but
> a complete ROM patches procedure that is vendor specific.
>
> As I said above, I want to see the /sys/kernel/debug/usb/devices from
> before and after first. Until then NAK.
before:
T: Bus=01 Lev=02 Prnt=02 Port=04 Cnt=02 Dev#= 4 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=0a5c ProdID=21d3 Rev= 1.12
S: Manufacturer=Broadcom Corp
S: Product=BCM43142A0
S: SerialNumber=3859F9D6199A
C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr= 0mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
I: If#= 1 Alt= 1 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
I: If#= 1 Alt= 2 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
I: If#= 1 Alt= 3 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms
I: If#= 1 Alt= 4 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms
I: If#= 1 Alt= 5 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms
I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=84(I) Atr=02(Bulk) MxPS= 32 Ivl=0ms
E: Ad=04(O) Atr=02(Bulk) MxPS= 32 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)
after:
T: Bus=01 Lev=02 Prnt=02 Port=04 Cnt=02 Dev#= 4 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=0a5c ProdID=21d3 Rev= 1.12
S: Manufacturer=Broadcom Corp
S: Product=BCM43142A0
S: SerialNumber=3859F9D6199A
C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr= 0mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
I: If#= 1 Alt= 1 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
I: If#= 1 Alt= 2 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
I: If#= 1 Alt= 3 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms
I: If#= 1 Alt= 4 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms
I: If#= 1 Alt= 5 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms
I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=84(I) Atr=02(Bulk) MxPS= 32 Ivl=0ms
E: Ad=04(O) Atr=02(Bulk) MxPS= 32 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)
Thanks,
Jesse
^ permalink raw reply
* Re: [PATCH BlueZ v3 06/15] gdbus: Implement DBus.Properties.Set method
From: Luiz Augusto von Dentz @ 2012-09-07 8:35 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Lucas De Marchi, linux-bluetooth
In-Reply-To: <1346996989.21200.120.camel@aeonflux>
Hi Marcel,
On Fri, Sep 7, 2012 at 8:49 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> >> Contrary to Get() and GetAll(), Set is asynchronous so we pass on the
>> >> DBusMessage so user is able to create the response. It's the only use of
>> >> this parameter.
>> >> ---
>> >> gdbus/gdbus.h | 7 +++++++
>> >> gdbus/object.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> >> 2 files changed, 64 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>> >> index b2e78c4..3e4aa16 100644
>> >> --- a/gdbus/gdbus.h
>> >> +++ b/gdbus/gdbus.h
>> >> @@ -31,6 +31,8 @@ extern "C" {
>> >> #include <dbus/dbus.h>
>> >> #include <glib.h>
>> >>
>> >> +typedef enum GDBusPropertySetReturn GDBusPropertySetReturn;
>> >> +
>> >> typedef enum GDBusMethodFlags GDBusMethodFlags;
>> >> typedef enum GDBusSignalFlags GDBusSignalFlags;
>> >> typedef enum GDBusPropertyFlags GDBusPropertyFlags;
>> >> @@ -69,6 +71,10 @@ typedef DBusMessage * (* GDBusMethodFunction) (DBusConnection *connection,
>> >> typedef gboolean (*GDBusPropertyGetter)(const GDBusPropertyTable *property,
>> >> DBusMessageIter *iter, void *data);
>> >>
>> >> +typedef DBusMessage *(*GDBusPropertySetter)(const GDBusPropertyTable *property,
>> >> + DBusMessageIter *value,
>> >> + DBusMessage *msg, void *data);
>> >> +
>> >
>> > I am not really happy with this. We just need a unique handle here since
>> > the return value is either success or an error. I rather don't send
>> > messages around for no other reason to create the error.
>> >
>> > Inside the authorization code I am using GDBusPendingReply as unique
>> > token. What about using the same approach?
>>
>> It would be perfect, but Im afraid sometimes the properties can have
>> restriction on which sender can set them, so perhaps we need to add
>> the name of the sender as parameter in addiction to just the handle?
>
> that should be done through the same security handling we have for the
> method calls. Don't try to invent something new.
Im afraid you will have to go in detail what you want here, the
security table seems to be meant for checking privileges in a method
level while we can probably extend it for properties and have
privileges also in the properties table, but the security table seems
to be global not per interface.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ v5 01/14] core: Control connections based on adapter state
From: Johan Hedberg @ 2012-09-07 9:59 UTC (permalink / raw)
To: João Paulo Rechi Vita; +Cc: linux-bluetooth, Claudio Takahasi
In-Reply-To: <1346785482-13359-2-git-send-email-jprvita@openbossa.org>
Hi,
On Tue, Sep 04, 2012, João Paulo Rechi Vita wrote:
> +static void set_auto_connect(gpointer data, gpointer user_data)
> +{
> + struct btd_device *device = data;
> + gboolean enable = GPOINTER_TO_INT(user_data);
> +
> + device_set_auto_connect(device, enable);
> +}
> +
> static void call_adapter_powered_callbacks(struct btd_adapter *adapter,
> gboolean powered)
> {
> @@ -2125,7 +2133,10 @@ static void call_adapter_powered_callbacks(struct btd_adapter *adapter,
> btd_adapter_powered_cb cb = l->data;
>
> cb(adapter, powered);
> - }
> + }
> +
> + g_slist_foreach(adapter->devices, set_auto_connect,
> + GINT_TO_POINTER(powered));
Instead of this GINT_TO_POINTER magic on a variable that isn't even a
gint couldn't you just pass &powered to g_slist_foreach and then in the
callback do:
gboolean enable = *(gboolean *) user_data;
or
gboolean *enable = user_data;
device_set_auto_connect(device, *enable);
Johan
^ permalink raw reply
* Re: [PATCH BlueZ v5 04/14] core: Add a list of LE devices to connect
From: Johan Hedberg @ 2012-09-07 10:23 UTC (permalink / raw)
To: João Paulo Rechi Vita; +Cc: linux-bluetooth
In-Reply-To: <1346785482-13359-5-git-send-email-jprvita@openbossa.org>
Hi,
On Tue, Sep 04, 2012, João Paulo Rechi Vita wrote:
> This commit creates a per-adapter list of LE devices to connect when a
> advertising from them is seen during a scan.
> ---
> src/adapter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> src/adapter.h | 5 +++++
> 2 files changed, 63 insertions(+), 1 deletion(-)
I applied patches 2-4 as they didn't seem to directly depend on the
first one. So no need to resend those when resending this patch set.
Johan
^ permalink raw reply
* [PATCH 1/3] input: Add helper function to request disconnect
From: Bastien Nocera @ 2012-09-07 11:01 UTC (permalink / raw)
To: linux-bluetooth
---
profiles/input/device.c | 7 +++++++
profiles/input/device.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/profiles/input/device.c b/profiles/input/device.c
index f1e1af0..04726f8 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1308,3 +1308,10 @@ int input_device_close_channels(const bdaddr_t *src, const bdaddr_t *dst)
return 0;
}
+
+void input_device_request_disconnect(struct fake_input *fake)
+{
+ if (fake == NULL || fake->idev == NULL)
+ return;
+ device_request_disconnect(fake->idev->device, NULL);
+}
diff --git a/profiles/input/device.h b/profiles/input/device.h
index 509a353..ff52967 100644
--- a/profiles/input/device.h
+++ b/profiles/input/device.h
@@ -49,3 +49,4 @@ int input_device_unregister(const char *path, const char *uuid);
int input_device_set_channel(const bdaddr_t *src, const bdaddr_t *dst, int psm,
GIOChannel *io);
int input_device_close_channels(const bdaddr_t *src, const bdaddr_t *dst);
+void input_device_request_disconnect(struct fake_input *fake);
--
1.7.12
^ permalink raw reply related
* [PATCH 2/3] fakehid: Disconnect from PS3 remote after 10 mins
From: Bastien Nocera @ 2012-09-07 11:01 UTC (permalink / raw)
To: linux-bluetooth
After 10 minutes, disconnect the PS3 BD Remote to avoid draining its
battery. This is consistent with its behaviour on the PS3.
Original patch by Ruslan N. Marchenko <rufferson@gmail.com>
---
profiles/input/device.h | 1 +
profiles/input/fakehid.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/profiles/input/device.h b/profiles/input/device.h
index ff52967..d8baa2c 100644
--- a/profiles/input/device.h
+++ b/profiles/input/device.h
@@ -33,6 +33,7 @@ struct fake_input {
int uinput; /* uinput socket */
int rfcomm; /* RFCOMM socket */
uint8_t ch; /* RFCOMM channel number */
+ guint timeout_id; /* Disconnect timeout ID */
gboolean (*connect) (struct input_conn *iconn, GError **err);
int (*disconnect) (struct input_conn *iconn);
void *priv;
diff --git a/profiles/input/fakehid.c b/profiles/input/fakehid.c
index d9af2dd..b12c526 100644
--- a/profiles/input/fakehid.c
+++ b/profiles/input/fakehid.c
@@ -41,6 +41,9 @@
#include "fakehid.h"
#include "uinput.h"
+/* Timeout to get the PS3 remote disconnected, in seconds */
+#define PS3_REMOTE_TIMEOUT 10 * 60
+
enum ps3remote_special_keys {
PS3R_BIT_PS = 0,
PS3R_BIT_ENTER = 3,
@@ -138,6 +141,13 @@ static unsigned int ps3remote_keymap[] = {
[0xff] = KEY_MAX,
};
+static gboolean ps3_remote_timeout_cb(gpointer user_data);
+
+static void ps3remote_set_timeout(struct fake_input *fake)
+{
+ fake->timeout_id = g_timeout_add_seconds(PS3_REMOTE_TIMEOUT, ps3_remote_timeout_cb, fake);
+}
+
static int ps3remote_decode(char *buff, int size, unsigned int *value)
{
static unsigned int lastkey = 0;
@@ -200,6 +210,16 @@ error:
return -1;
}
+static gboolean
+ps3_remote_timeout_cb(gpointer user_data)
+{
+ struct fake_input *fake = (struct fake_input *) user_data;
+ input_device_request_disconnect(fake);
+ DBG("Disconnected PS3 BD Remote after timeout");
+ fake->timeout_id = 0;
+ return FALSE;
+}
+
static gboolean ps3remote_event(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
@@ -253,9 +273,17 @@ static gboolean ps3remote_event(GIOChannel *chan, GIOCondition cond,
goto failed;
}
+ if (fake->timeout_id > 0)
+ g_source_remove(fake->timeout_id);
+ ps3remote_set_timeout(fake);
+
return TRUE;
failed:
+ if (fake->timeout_id > 0) {
+ g_source_remove(fake->timeout_id);
+ fake->timeout_id = 0;
+ }
ioctl(fake->uinput, UI_DEV_DESTROY);
close(fake->uinput);
fake->uinput = -1;
@@ -315,6 +343,8 @@ static int ps3remote_setup_uinput(struct fake_input *fake,
goto err;
}
+ ps3remote_set_timeout(fake);
+
return 0;
err:
@@ -375,6 +405,8 @@ struct fake_input *fake_hid_connadd(struct fake_input *fake,
for (l = fake_hid->devices; l != NULL; l = l->next) {
old = l->data;
if (old->idev == fake->idev) {
+ if (fake->timeout_id > 0)
+ g_source_remove(fake->timeout_id);
g_free(fake);
fake = old;
fake_hid->connect(fake, NULL);
--
1.7.12
^ permalink raw reply related
* [PATCH 3/3] fakehid: Use the same constant as declared
From: Bastien Nocera @ 2012-09-07 11:01 UTC (permalink / raw)
To: linux-bluetooth
ps3remote_keymap[] uses 0xff as the max value, so should we.
---
profiles/input/fakehid.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/input/fakehid.c b/profiles/input/fakehid.c
index b12c526..e665869 100644
--- a/profiles/input/fakehid.c
+++ b/profiles/input/fakehid.c
@@ -328,7 +328,7 @@ static int ps3remote_setup_uinput(struct fake_input *fake,
}
/* enabling keys */
- for (i = 0; i < 256; i++)
+ for (i = 0; i < 0xff; i++)
if (ps3remote_keymap[i] != KEY_RESERVED)
if (ioctl(fake->uinput, UI_SET_KEYBIT,
ps3remote_keymap[i]) < 0) {
--
1.7.12
^ permalink raw reply related
* [PATCH 1/2] input: Add helper function to request disconnect
From: Bastien Nocera @ 2012-09-07 12:01 UTC (permalink / raw)
To: linux-bluetooth
---
profiles/input/device.c | 8 ++++++++
profiles/input/device.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/profiles/input/device.c b/profiles/input/device.c
index f1e1af0..c42f137 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1308,3 +1308,11 @@ int input_device_close_channels(const bdaddr_t *src, const bdaddr_t *dst)
return 0;
}
+
+void input_device_request_disconnect(struct fake_input *fake)
+{
+ if (fake == NULL || fake->idev == NULL)
+ return;
+
+ device_request_disconnect(fake->idev->device, NULL);
+}
diff --git a/profiles/input/device.h b/profiles/input/device.h
index 509a353..ff52967 100644
--- a/profiles/input/device.h
+++ b/profiles/input/device.h
@@ -49,3 +49,4 @@ int input_device_unregister(const char *path, const char *uuid);
int input_device_set_channel(const bdaddr_t *src, const bdaddr_t *dst, int psm,
GIOChannel *io);
int input_device_close_channels(const bdaddr_t *src, const bdaddr_t *dst);
+void input_device_request_disconnect(struct fake_input *fake);
--
1.7.12
^ permalink raw reply related
* [PATCH 2/2] fakehid: Disconnect from PS3 remote after 10 mins
From: Bastien Nocera @ 2012-09-07 12:02 UTC (permalink / raw)
To: linux-bluetooth
After 10 minutes, disconnect the PS3 BD Remote to avoid draining its
battery. This is consistent with its behaviour on the PS3.
Original patch by Ruslan N. Marchenko <rufferson@gmail.com>
---
profiles/input/device.h | 1 +
profiles/input/fakehid.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/profiles/input/device.h b/profiles/input/device.h
index ff52967..d8baa2c 100644
--- a/profiles/input/device.h
+++ b/profiles/input/device.h
@@ -33,6 +33,7 @@ struct fake_input {
int uinput; /* uinput socket */
int rfcomm; /* RFCOMM socket */
uint8_t ch; /* RFCOMM channel number */
+ guint timeout_id; /* Disconnect timeout ID */
gboolean (*connect) (struct input_conn *iconn, GError **err);
int (*disconnect) (struct input_conn *iconn);
void *priv;
diff --git a/profiles/input/fakehid.c b/profiles/input/fakehid.c
index 305a204..4c7c9ab 100644
--- a/profiles/input/fakehid.c
+++ b/profiles/input/fakehid.c
@@ -41,6 +41,9 @@
#include "fakehid.h"
#include "uinput.h"
+/* Timeout to get the PS3 remote disconnected, in seconds */
+#define PS3_REMOTE_TIMEOUT 10 * 60
+
enum ps3remote_special_keys {
PS3R_BIT_PS = 0,
PS3R_BIT_ENTER = 3,
@@ -200,6 +203,24 @@ error:
return -1;
}
+static gboolean ps3_remote_timeout_cb(gpointer user_data)
+{
+ struct fake_input *fake = user_data;
+
+ input_device_request_disconnect(fake);
+ DBG("Disconnected PS3 BD Remote after timeout");
+
+ fake->timeout_id = 0;
+
+ return FALSE;
+}
+
+static void ps3remote_set_timeout(struct fake_input *fake)
+{
+ fake->timeout_id = g_timeout_add_seconds(PS3_REMOTE_TIMEOUT,
+ ps3_remote_timeout_cb, fake);
+}
+
static gboolean ps3remote_event(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
@@ -253,9 +274,17 @@ static gboolean ps3remote_event(GIOChannel *chan, GIOCondition cond,
goto failed;
}
+ if (fake->timeout_id > 0)
+ g_source_remove(fake->timeout_id);
+ ps3remote_set_timeout(fake);
+
return TRUE;
failed:
+ if (fake->timeout_id > 0) {
+ g_source_remove(fake->timeout_id);
+ fake->timeout_id = 0;
+ }
ioctl(fake->uinput, UI_DEV_DESTROY);
close(fake->uinput);
fake->uinput = -1;
@@ -315,6 +344,8 @@ static int ps3remote_setup_uinput(struct fake_input *fake,
goto err;
}
+ ps3remote_set_timeout(fake);
+
return 0;
err:
@@ -375,6 +406,8 @@ struct fake_input *fake_hid_connadd(struct fake_input *fake,
for (l = fake_hid->devices; l != NULL; l = l->next) {
old = l->data;
if (old->idev == fake->idev) {
+ if (fake->timeout_id > 0)
+ g_source_remove(fake->timeout_id);
g_free(fake);
fake = old;
fake_hid->connect(fake, NULL);
--
1.7.12
^ permalink raw reply related
* Re: [PATCH 1/2] input: Add helper function to request disconnect
From: Johan Hedberg @ 2012-09-07 12:24 UTC (permalink / raw)
To: Bastien Nocera; +Cc: linux-bluetooth
In-Reply-To: <1347019318.26781.2.camel@sirocco.hadess.net>
Hi Bastien,
On Fri, Sep 07, 2012, Bastien Nocera wrote:
>
> ---
> profiles/input/device.c | 8 ++++++++
> profiles/input/device.h | 1 +
> 2 files changed, 9 insertions(+)
Both patches have been applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Add support for Logitech Harmony Adapter for PS3
From: Johan Hedberg @ 2012-09-07 12:45 UTC (permalink / raw)
To: David Dillow; +Cc: linux-bluetooth
In-Reply-To: <1346378760.7976.2.camel@obelisk.thedillows.org>
Hi David,
On Thu, Aug 30, 2012, David Dillow wrote:
> This emulates a Sony BD Remote for the Logitech Harmony series of
> universal remotes.
> --
> profiles/input/fakehid.c | 10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
Applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
From: Andrei Emeltchenko @ 2012-09-07 14:07 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo
In-Reply-To: <alpine.DEB.2.02.1209060946190.22884@mathewm-linux>
Hi Mat,
On Thu, Sep 06, 2012 at 10:01:52AM -0700, Mat Martineau wrote:
>
> Hi Andrei -
>
> On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
>
> >From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> >If we have unacked frames when closing bluetooth socket we deadlock
> >since conn->chan_lock, chan->lock and socket lock are taken. Remove
> >__l2cap_wait_ack completely.
> >
> >Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> I don't think you want to remove this code completely, at least not
> without giving some thought to the problem it is solving.
>
> The problem is that programs may have an open socket which they send
> some data on, then immediately close. There is no feedback when
> data is actually sent over the air, so the socket may end up getting
> torn down while there is still data in the HCI tx buffer or some
> data was lost and needs to be retransmitted. Waiting for an
> acknowledgement confirms that the application's sent data made it to
> the remote device.
>
> Without this code, it's difficult to use l2test on a number of
> qualification tests. Profiles or applications using ERTM may depend
> on the "wait for ack before closing" behavior in order to have a
> clean disconnect.
>
> It is not reasonable to deadlock while waiting for unacked packets
> to go to 0, so maybe more needs to be done in __l2cap_wait_ack to
> limit the wait time.
Have you seen my previous RFC concerning this? It has 3 tries and then
exits from the loop.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
From: Andrei Emeltchenko @ 2012-09-07 14:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Mat Martineau, linux-bluetooth, gustavo
In-Reply-To: <1346962251.21200.119.camel@aeonflux>
Hi Marcel,
On Thu, Sep 06, 2012 at 01:10:51PM -0700, Marcel Holtmann wrote:
> Hi Mat,
>
> > > If we have unacked frames when closing bluetooth socket we deadlock
> > > since conn->chan_lock, chan->lock and socket lock are taken. Remove
> > > __l2cap_wait_ack completely.
> > >
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > I don't think you want to remove this code completely, at least not
> > without giving some thought to the problem it is solving.
> >
> > The problem is that programs may have an open socket which they send
> > some data on, then immediately close. There is no feedback when data
> > is actually sent over the air, so the socket may end up getting torn
> > down while there is still data in the HCI tx buffer or some data was
> > lost and needs to be retransmitted. Waiting for an acknowledgement
> > confirms that the application's sent data made it to the remote
> > device.
> >
> > Without this code, it's difficult to use l2test on a number of
> > qualification tests. Profiles or applications using ERTM may depend
> > on the "wait for ack before closing" behavior in order to have a clean
> > disconnect.
>
> isn't that what we have SO_LINGER for?
Looking at the code I suspect that SO_LINGER is not working. Maybe we need
to merge linger code and wait_ack stuff.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* [RFC v3 0/9] Optional acquire in Media API and related
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
This patchset addresses the same problems as before (optional acquire race condition, see patch v3 9/9) along with the symmetric "optional release" problem as described in patch v3 8/9. The solution in this second case consists of exposing the transport state in D-Bus, as discussed in the IRC.
>From original patch:
This patch reopens the discussion started by the thread "when is acquire
ok to call". The race condition seems to be real (even thought difficult
to reproduce), and I couldn't think of any approach to solve this
without altering the Media API.
Mikel Astiz (9):
media: Fix accesstype comparison
media: Add a2dp_sep_is_playing() to internal API
media: Add gateway_get_state() to internal API
media: Replace transport->in_use flag with state
media: Watch interface state changes in transport
media: Split transport state based on playing flag
media: Automatically release transport when HUP
media: Expose transport state in D-Bus
media: Extend media API with optional acquire
audio/a2dp.c | 8 +
audio/a2dp.h | 1 +
audio/gateway.c | 7 +
audio/gateway.h | 1 +
audio/transport.c | 390 +++++++++++++++++++++++++++++++++++++++++++----------
doc/media-api.txt | 19 +++
6 files changed, 356 insertions(+), 70 deletions(-)
--
1.7.7.6
^ permalink raw reply
* [RFC v3 1/9] media: Fix accesstype comparison
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Replace the string representation of the accesstype with a conventional
binary representation. This makes the code simpler and more efficient.
This also fixes a minor bug in the Release() D-Bus method, where the
string comparison was used to see whether the owner should be removed. A
client acquiring with "rw" and releasing with "wr" would lead to the
inconsistent state of having a released transport with an owner with no
accesstype. Partial releases can also get affected by this bug since the
released character (partial accesstype) got replaced by a whitespace.
Additionally, this approach is more robust in case new flags are added
in the future.
---
audio/transport.c | 119 ++++++++++++++++++++++++++++++-----------------------
1 files changed, 68 insertions(+), 51 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index d40c92d..d418878 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -49,6 +49,11 @@
#define MEDIA_TRANSPORT_INTERFACE "org.bluez.MediaTransport"
+typedef enum {
+ TRANSPORT_LOCK_READ = 1,
+ TRANSPORT_LOCK_WRITE = 1 << 1,
+} transport_lock_t;
+
struct media_request {
DBusMessage *msg;
guint id;
@@ -58,7 +63,7 @@ struct media_owner {
struct media_transport *transport;
struct media_request *pending;
char *name;
- char *accesstype;
+ transport_lock_t lock;
guint watch;
};
@@ -84,8 +89,7 @@ struct media_transport {
int fd; /* Transport file descriptor */
uint16_t imtu; /* Transport input mtu */
uint16_t omtu; /* Transport output mtu */
- gboolean read_lock;
- gboolean write_lock;
+ transport_lock_t lock;
gboolean in_use;
guint (*resume) (struct media_transport *transport,
struct media_owner *owner);
@@ -104,6 +108,31 @@ struct media_transport {
void *data;
};
+static const char *lock2str(transport_lock_t lock)
+{
+ if (lock == 0)
+ return "";
+ else if (lock == TRANSPORT_LOCK_READ)
+ return "r";
+ else if (lock == TRANSPORT_LOCK_WRITE)
+ return "w";
+ else
+ return "rw";
+}
+
+static transport_lock_t str2lock(const char *str)
+{
+ transport_lock_t lock = 0;
+
+ if (g_strstr_len(str, -1, "r") != NULL)
+ lock |= TRANSPORT_LOCK_READ;
+
+ if (g_strstr_len(str, -1, "w") != NULL)
+ lock |= TRANSPORT_LOCK_WRITE;
+
+ return lock;
+}
+
void media_transport_destroy(struct media_transport *transport)
{
char *path;
@@ -148,17 +177,15 @@ static void media_request_reply(struct media_request *req,
}
static gboolean media_transport_release(struct media_transport *transport,
- const char *accesstype)
+ transport_lock_t lock)
{
- if (g_strstr_len(accesstype, -1, "r") != NULL) {
- transport->read_lock = FALSE;
+ transport->lock &= ~lock;
+
+ if (lock & TRANSPORT_LOCK_READ)
DBG("Transport %s: read lock released", transport->path);
- }
- if (g_strstr_len(accesstype, -1, "w") != NULL) {
- transport->write_lock = FALSE;
+ if (lock & TRANSPORT_LOCK_WRITE)
DBG("Transport %s: write lock released", transport->path);
- }
return TRUE;
}
@@ -191,7 +218,6 @@ static void media_owner_free(struct media_owner *owner)
media_owner_remove(owner);
g_free(owner->name);
- g_free(owner->accesstype);
g_free(owner);
}
@@ -200,7 +226,7 @@ static void media_transport_remove(struct media_transport *transport,
{
DBG("Transport %s Owner %s", transport->path, owner->name);
- media_transport_release(transport, owner->accesstype);
+ media_transport_release(transport, owner->lock);
/* Reply if owner has a pending request */
if (owner->pending)
@@ -260,10 +286,10 @@ static void a2dp_resume_complete(struct avdtp *session,
media_transport_set_fd(transport, fd, imtu, omtu);
- if (g_strstr_len(owner->accesstype, -1, "r") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_READ) == 0)
imtu = 0;
- if (g_strstr_len(owner->accesstype, -1, "w") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_WRITE) == 0)
omtu = 0;
ret = g_dbus_send_reply(transport->conn, req->msg,
@@ -371,10 +397,10 @@ static void headset_resume_complete(struct audio_device *dev, void *user_data)
media_transport_set_fd(transport, fd, imtu, omtu);
- if (g_strstr_len(owner->accesstype, -1, "r") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_READ) == 0)
imtu = 0;
- if (g_strstr_len(owner->accesstype, -1, "w") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_WRITE) == 0)
omtu = 0;
ret = g_dbus_send_reply(transport->conn, req->msg,
@@ -476,10 +502,10 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err,
media_transport_set_fd(transport, fd, imtu, omtu);
- if (g_strstr_len(owner->accesstype, -1, "r") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_READ) == 0)
imtu = 0;
- if (g_strstr_len(owner->accesstype, -1, "w") == NULL)
+ if ((owner->lock & TRANSPORT_LOCK_WRITE) == 0)
omtu = 0;
ret = g_dbus_send_reply(transport->conn, req->msg,
@@ -569,35 +595,18 @@ static void media_owner_exit(DBusConnection *connection, void *user_data)
}
static gboolean media_transport_acquire(struct media_transport *transport,
- const char *accesstype)
+ transport_lock_t lock)
{
- gboolean read_lock = FALSE, write_lock = FALSE;
-
- if (g_strstr_len(accesstype, -1, "r") != NULL) {
- if (transport->read_lock == TRUE)
- return FALSE;
- read_lock = TRUE;
- }
-
- if (g_strstr_len(accesstype, -1, "w") != NULL) {
- if (transport->write_lock == TRUE)
- return FALSE;
- write_lock = TRUE;
- }
-
- /* Check invalid accesstype */
- if (read_lock == FALSE && write_lock == FALSE)
+ if (transport->lock & lock)
return FALSE;
- if (read_lock) {
- transport->read_lock = read_lock;
+ transport->lock |= lock;
+
+ if (lock & TRANSPORT_LOCK_READ)
DBG("Transport %s: read lock acquired", transport->path);
- }
- if (write_lock) {
- transport->write_lock = write_lock;
+ if (lock & TRANSPORT_LOCK_WRITE)
DBG("Transport %s: write lock acquired", transport->path);
- }
return TRUE;
@@ -616,16 +625,16 @@ static void media_transport_add(struct media_transport *transport,
static struct media_owner *media_owner_create(DBusConnection *conn,
DBusMessage *msg,
- const char *accesstype)
+ transport_lock_t lock)
{
struct media_owner *owner;
owner = g_new0(struct media_owner, 1);
owner->name = g_strdup(dbus_message_get_sender(msg));
- owner->accesstype = g_strdup(accesstype);
+ owner->lock = lock;
DBG("Owner created: sender=%s accesstype=%s", owner->name,
- accesstype);
+ lock2str(lock));
return owner;
}
@@ -662,6 +671,7 @@ static DBusMessage *acquire(DBusConnection *conn, DBusMessage *msg,
struct media_owner *owner;
struct media_request *req;
const char *accesstype, *sender;
+ transport_lock_t lock;
guint id;
if (!dbus_message_get_args(msg, NULL,
@@ -675,13 +685,17 @@ static DBusMessage *acquire(DBusConnection *conn, DBusMessage *msg,
if (owner != NULL)
return btd_error_not_authorized(msg);
- if (media_transport_acquire(transport, accesstype) == FALSE)
+ lock = str2lock(accesstype);
+ if (lock == 0)
+ return btd_error_invalid_args(msg);
+
+ if (media_transport_acquire(transport, lock) == FALSE)
return btd_error_not_authorized(msg);
- owner = media_owner_create(conn, msg, accesstype);
+ owner = media_owner_create(conn, msg, lock);
id = transport->resume(transport, owner);
if (id == 0) {
- media_transport_release(transport, accesstype);
+ media_transport_release(transport, lock);
media_owner_free(owner);
return btd_error_not_authorized(msg);
}
@@ -699,6 +713,7 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
struct media_transport *transport = data;
struct media_owner *owner;
const char *accesstype, *sender;
+ transport_lock_t lock;
struct media_request *req;
if (!dbus_message_get_args(msg, NULL,
@@ -712,7 +727,9 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
if (owner == NULL)
return btd_error_not_authorized(msg);
- if (g_strcmp0(owner->accesstype, accesstype) == 0) {
+ lock = str2lock(accesstype);
+
+ if (owner->lock == lock) {
guint id;
/* Not the last owner, no need to suspend */
@@ -742,9 +759,9 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
media_owner_add(owner, req);
return NULL;
- } else if (g_strstr_len(owner->accesstype, -1, accesstype) != NULL) {
- media_transport_release(transport, accesstype);
- g_strdelimit(owner->accesstype, accesstype, ' ');
+ } else if ((owner->lock & lock) == lock) {
+ media_transport_release(transport, lock);
+ owner->lock &= ~lock;
} else
return btd_error_not_authorized(msg);
--
1.7.7.6
^ permalink raw reply related
* [RFC v3 2/9] media: Add a2dp_sep_is_playing() to internal API
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Add this function to expose whether the local SEP is streaming or not.
---
audio/a2dp.c | 8 ++++++++
audio/a2dp.h | 1 +
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/audio/a2dp.c b/audio/a2dp.c
index a9546b7..64b37e7 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1833,6 +1833,14 @@ gboolean a2dp_sep_unlock(struct a2dp_sep *sep, struct avdtp *session)
return TRUE;
}
+gboolean a2dp_sep_is_playing(struct a2dp_sep *sep)
+{
+ if (avdtp_sep_get_state(sep->lsep) == AVDTP_STATE_STREAMING)
+ return TRUE;
+ else
+ return FALSE;
+}
+
gboolean a2dp_sep_get_lock(struct a2dp_sep *sep)
{
return sep->locked;
diff --git a/audio/a2dp.h b/audio/a2dp.h
index 27b4a57..deab3b8 100644
--- a/audio/a2dp.h
+++ b/audio/a2dp.h
@@ -90,6 +90,7 @@ gboolean a2dp_cancel(struct audio_device *dev, unsigned int id);
gboolean a2dp_sep_lock(struct a2dp_sep *sep, struct avdtp *session);
gboolean a2dp_sep_unlock(struct a2dp_sep *sep, struct avdtp *session);
gboolean a2dp_sep_get_lock(struct a2dp_sep *sep);
+gboolean a2dp_sep_is_playing(struct a2dp_sep *sep);
struct avdtp_stream *a2dp_sep_get_stream(struct a2dp_sep *sep);
struct a2dp_sep *a2dp_get_sep(struct avdtp *session,
struct avdtp_stream *stream);
--
1.7.7.6
^ permalink raw reply related
* [RFC v3 3/9] media: Add gateway_get_state() to internal API
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Expose the state of the gateway interface in the internal API.
---
audio/gateway.c | 7 +++++++
audio/gateway.h | 1 +
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/audio/gateway.c b/audio/gateway.c
index 0603f12..53094af 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -787,6 +787,13 @@ gboolean gateway_is_active(struct audio_device *dev)
return FALSE;
}
+gateway_state_t gateway_get_state(struct audio_device *dev)
+{
+ struct gateway *gw = dev->gateway;
+
+ return gw->state;
+}
+
int gateway_connect_rfcomm(struct audio_device *dev, GIOChannel *io)
{
if (!io)
diff --git a/audio/gateway.h b/audio/gateway.h
index 0893962..d87d76a 100644
--- a/audio/gateway.h
+++ b/audio/gateway.h
@@ -59,6 +59,7 @@ void gateway_unregister(struct audio_device *dev);
struct gateway *gateway_init(struct audio_device *dev);
gboolean gateway_is_active(struct audio_device *dev);
gboolean gateway_is_connected(struct audio_device *dev);
+gateway_state_t gateway_get_state(struct audio_device *dev);
int gateway_connect_rfcomm(struct audio_device *dev, GIOChannel *io);
int gateway_connect_sco(struct audio_device *dev, GIOChannel *chan);
void gateway_start_service(struct audio_device *device);
--
1.7.7.6
^ permalink raw reply related
* [RFC v3 4/9] media: Replace transport->in_use flag with state
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Refactor the code to use a enum type to represent the transport state.
This should scale better when additional states need to be represented.
A helper function has been added to help track the mapping between the
enum type and the old in_use flag.
---
audio/transport.c | 77 ++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 58 insertions(+), 19 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index d418878..0ae9a8c 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -54,6 +54,16 @@ typedef enum {
TRANSPORT_LOCK_WRITE = 1 << 1,
} transport_lock_t;
+typedef enum {
+ TRANSPORT_STATE_IDLE, /* Not acquired */
+ TRANSPORT_STATE_ACQUIRED, /* Acquired (not necessarily playing) */
+} transport_state_t;
+
+static char *str_state[] = {
+ "TRANSPORT_STATE_IDLE",
+ "TRANSPORT_STATE_ACQUIRED",
+};
+
struct media_request {
DBusMessage *msg;
guint id;
@@ -90,7 +100,7 @@ struct media_transport {
uint16_t imtu; /* Transport input mtu */
uint16_t omtu; /* Transport output mtu */
transport_lock_t lock;
- gboolean in_use;
+ transport_state_t state;
guint (*resume) (struct media_transport *transport,
struct media_owner *owner);
guint (*suspend) (struct media_transport *transport,
@@ -133,6 +143,32 @@ static transport_lock_t str2lock(const char *str)
return lock;
}
+static gboolean state_in_use(transport_state_t state)
+{
+ switch (state) {
+ case TRANSPORT_STATE_IDLE:
+ return FALSE;
+ case TRANSPORT_STATE_ACQUIRED:
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
+static void transport_set_state(struct media_transport *transport,
+ transport_state_t state)
+{
+ transport_state_t old_state = transport->state;
+
+ if (old_state == state)
+ return;
+
+ transport->state = state;
+
+ DBG("State changed %s: %s -> %s", transport->path, str_state[old_state],
+ str_state[state]);
+}
+
void media_transport_destroy(struct media_transport *transport)
{
char *path;
@@ -240,7 +276,7 @@ static void media_transport_remove(struct media_transport *transport,
media_owner_free(owner);
/* Suspend if there is no longer any owner */
- if (transport->owners == NULL && transport->in_use)
+ if (transport->owners == NULL && state_in_use(transport->state))
transport->suspend(transport, NULL);
}
@@ -322,13 +358,14 @@ static guint resume_a2dp(struct media_transport *transport,
return 0;
}
- if (transport->in_use == TRUE)
+ if (state_in_use(transport->state))
goto done;
- transport->in_use = a2dp_sep_lock(sep, a2dp->session);
- if (transport->in_use == FALSE)
+ if (a2dp_sep_lock(sep, a2dp->session) == FALSE)
return 0;
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
done:
return a2dp_resume(a2dp->session, sep, a2dp_resume_complete, owner);
}
@@ -349,7 +386,7 @@ static void a2dp_suspend_complete(struct avdtp *session,
}
a2dp_sep_unlock(sep, a2dp->session);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
media_transport_remove(transport, owner);
}
@@ -362,7 +399,7 @@ static guint suspend_a2dp(struct media_transport *transport,
if (!owner) {
a2dp_sep_unlock(sep, a2dp->session);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
return 0;
}
@@ -424,14 +461,15 @@ static guint resume_headset(struct media_transport *transport,
{
struct audio_device *device = transport->device;
- if (transport->in_use == TRUE)
+ if (state_in_use(transport->state))
goto done;
- transport->in_use = headset_lock(device, HEADSET_LOCK_READ |
- HEADSET_LOCK_WRITE);
- if (transport->in_use == FALSE)
+ if (headset_lock(device, HEADSET_LOCK_READ |
+ HEADSET_LOCK_WRITE) == FALSE)
return 0;
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
done:
return headset_request_stream(device, headset_resume_complete,
owner);
@@ -450,7 +488,7 @@ static void headset_suspend_complete(struct audio_device *dev, void *user_data)
}
headset_unlock(dev, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
media_transport_remove(transport, owner);
}
@@ -461,7 +499,7 @@ static guint suspend_headset(struct media_transport *transport,
if (!owner) {
headset_unlock(device, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
return 0;
}
@@ -529,14 +567,15 @@ static guint resume_gateway(struct media_transport *transport,
{
struct audio_device *device = transport->device;
- if (transport->in_use == TRUE)
+ if (state_in_use(transport->state))
goto done;
- transport->in_use = gateway_lock(device, GATEWAY_LOCK_READ |
- GATEWAY_LOCK_WRITE);
- if (transport->in_use == FALSE)
+ if (gateway_lock(device, GATEWAY_LOCK_READ |
+ GATEWAY_LOCK_WRITE) == FALSE)
return 0;
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
done:
return gateway_request_stream(device, gateway_resume_complete,
owner);
@@ -556,7 +595,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
}
gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
media_transport_remove(transport, owner);
return FALSE;
}
@@ -569,7 +608,7 @@ static guint suspend_gateway(struct media_transport *transport,
if (!owner) {
gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
- transport->in_use = FALSE;
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
return 0;
}
--
1.7.7.6
^ permalink raw reply related
* [RFC v3 5/9] media: Watch interface state changes in transport
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Install watches to keep track whether the audio is streaming or not.
This should be relevant if the transport needs to reflect this state.
---
audio/transport.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 103 insertions(+), 0 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 0ae9a8c..3026022 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -45,6 +45,8 @@
#include "a2dp.h"
#include "headset.h"
#include "gateway.h"
+#include "sink.h"
+#include "source.h"
#include "avrcp.h"
#define MEDIA_TRANSPORT_INTERFACE "org.bluez.MediaTransport"
@@ -101,6 +103,10 @@ struct media_transport {
uint16_t omtu; /* Transport output mtu */
transport_lock_t lock;
transport_state_t state;
+ guint hs_watch;
+ guint ag_watch;
+ guint source_watch;
+ guint sink_watch;
guint (*resume) (struct media_transport *transport,
struct media_owner *owner);
guint (*suspend) (struct media_transport *transport,
@@ -173,6 +179,18 @@ void media_transport_destroy(struct media_transport *transport)
{
char *path;
+ if (transport->hs_watch)
+ headset_remove_state_cb(transport->hs_watch);
+
+ if (transport->ag_watch)
+ gateway_remove_state_cb(transport->ag_watch);
+
+ if (transport->sink_watch)
+ sink_remove_state_cb(transport->sink_watch);
+
+ if (transport->source_watch)
+ source_remove_state_cb(transport->source_watch);
+
path = g_strdup(transport->path);
g_dbus_unregister_interface(transport->conn, path,
MEDIA_TRANSPORT_INTERFACE);
@@ -1083,6 +1101,76 @@ static void headset_nrec_changed(struct audio_device *dev, gboolean nrec,
DBUS_TYPE_BOOLEAN, &nrec);
}
+static void transport_update_playing(struct media_transport *transport,
+ gboolean playing)
+{
+ DBG("%s Playing=%d", transport->path, playing);
+}
+
+static void headset_state_changed(struct audio_device *dev,
+ headset_state_t old_state,
+ headset_state_t new_state,
+ void *user_data)
+{
+ struct media_transport *transport = user_data;
+
+ if (dev != transport->device)
+ return;
+
+ if (new_state == HEADSET_STATE_PLAYING)
+ transport_update_playing(transport, TRUE);
+ else
+ transport_update_playing(transport, FALSE);
+}
+
+static void gateway_state_changed(struct audio_device *dev,
+ gateway_state_t old_state,
+ gateway_state_t new_state,
+ void *user_data)
+{
+ struct media_transport *transport = user_data;
+
+ if (dev != transport->device)
+ return;
+
+ if (new_state == GATEWAY_STATE_PLAYING)
+ transport_update_playing(transport, TRUE);
+ else
+ transport_update_playing(transport, FALSE);
+}
+
+static void sink_state_changed(struct audio_device *dev,
+ sink_state_t old_state,
+ sink_state_t new_state,
+ void *user_data)
+{
+ struct media_transport *transport = user_data;
+
+ if (dev != transport->device)
+ return;
+
+ if (new_state == SINK_STATE_PLAYING)
+ transport_update_playing(transport, TRUE);
+ else
+ transport_update_playing(transport, FALSE);
+}
+
+static void source_state_changed(struct audio_device *dev,
+ source_state_t old_state,
+ source_state_t new_state,
+ void *user_data)
+{
+ struct media_transport *transport = user_data;
+
+ if (dev != transport->device)
+ return;
+
+ if (new_state == SOURCE_STATE_PLAYING)
+ transport_update_playing(transport, TRUE);
+ else
+ transport_update_playing(transport, FALSE);
+}
+
struct media_transport *media_transport_create(DBusConnection *conn,
struct media_endpoint *endpoint,
struct audio_device *device,
@@ -1118,6 +1206,15 @@ struct media_transport *media_transport_create(DBusConnection *conn,
transport->set_property = set_property_a2dp;
transport->data = a2dp;
transport->destroy = destroy_a2dp;
+
+ if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0)
+ transport->sink_watch = sink_add_state_cb(
+ sink_state_changed,
+ transport);
+ else
+ transport->source_watch = source_add_state_cb(
+ source_state_changed,
+ transport);
} else if (strcasecmp(uuid, HFP_AG_UUID) == 0 ||
strcasecmp(uuid, HSP_AG_UUID) == 0) {
struct headset_transport *headset;
@@ -1135,6 +1232,9 @@ struct media_transport *media_transport_create(DBusConnection *conn,
transport->set_property = set_property_headset;
transport->data = headset;
transport->destroy = destroy_headset;
+ transport->hs_watch = headset_add_state_cb(
+ headset_state_changed,
+ transport);
} else if (strcasecmp(uuid, HFP_HS_UUID) == 0 ||
strcasecmp(uuid, HSP_HS_UUID) == 0) {
transport->resume = resume_gateway;
@@ -1142,6 +1242,9 @@ struct media_transport *media_transport_create(DBusConnection *conn,
transport->cancel = cancel_gateway;
transport->get_properties = get_properties_gateway;
transport->set_property = set_property_gateway;
+ transport->ag_watch = gateway_add_state_cb(
+ gateway_state_changed,
+ transport);
} else
goto fail;
--
1.7.7.6
^ permalink raw reply related
* [RFC v3 6/9] media: Split transport state based on playing flag
From: Mikel Astiz @ 2012-09-07 15:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
In-Reply-To: <1347031233-21621-1-git-send-email-mikel.astiz.oss@gmail.com>
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Split the transport states (formerly in_use) into more specific states
where the stream state (playing or suspended) is explicitly represented,
along with the transitional states (locally initiated suspend and
resume).
---
audio/transport.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 61 insertions(+), 9 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 3026022..8055791 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -57,13 +57,21 @@ typedef enum {
} transport_lock_t;
typedef enum {
- TRANSPORT_STATE_IDLE, /* Not acquired */
- TRANSPORT_STATE_ACQUIRED, /* Acquired (not necessarily playing) */
+ TRANSPORT_STATE_IDLE, /* Not acquired and suspended */
+ TRANSPORT_STATE_PENDING, /* Playing but not acquired */
+ TRANSPORT_STATE_REQUESTING, /* Acquire in progress */
+ TRANSPORT_STATE_ACQUIRED, /* Acquired and playing */
+ TRANSPORT_STATE_POST_ACQUIRE, /* Acquired but later suspended */
+ TRANSPORT_STATE_SUSPENDING, /* Release in progress */
} transport_state_t;
static char *str_state[] = {
"TRANSPORT_STATE_IDLE",
+ "TRANSPORT_STATE_PENDING",
+ "TRANSPORT_STATE_REQUESTING",
"TRANSPORT_STATE_ACQUIRED",
+ "TRANSPORT_STATE_POST_ACQUIRE",
+ "TRANSPORT_STATE_SUSPENDING",
};
struct media_request {
@@ -153,8 +161,12 @@ static gboolean state_in_use(transport_state_t state)
{
switch (state) {
case TRANSPORT_STATE_IDLE:
+ case TRANSPORT_STATE_PENDING:
return FALSE;
+ case TRANSPORT_STATE_REQUESTING:
case TRANSPORT_STATE_ACQUIRED:
+ case TRANSPORT_STATE_POST_ACQUIRE:
+ case TRANSPORT_STATE_SUSPENDING:
return TRUE;
}
@@ -356,6 +368,8 @@ static void a2dp_resume_complete(struct avdtp *session,
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
return;
fail:
@@ -382,7 +396,8 @@ static guint resume_a2dp(struct media_transport *transport,
if (a2dp_sep_lock(sep, a2dp->session) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return a2dp_resume(a2dp->session, sep, a2dp_resume_complete, owner);
@@ -417,7 +432,12 @@ static guint suspend_a2dp(struct media_transport *transport,
if (!owner) {
a2dp_sep_unlock(sep, a2dp->session);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (a2dp_sep_is_playing(sep))
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -468,6 +488,8 @@ static void headset_resume_complete(struct audio_device *dev, void *user_data)
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
return;
fail:
@@ -486,7 +508,8 @@ static guint resume_headset(struct media_transport *transport,
HEADSET_LOCK_WRITE) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return headset_request_stream(device, headset_resume_complete,
@@ -516,8 +539,15 @@ static guint suspend_headset(struct media_transport *transport,
struct audio_device *device = transport->device;
if (!owner) {
+ headset_state_t state = headset_get_state(device);
+
headset_unlock(device, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (state == HEADSET_STATE_PLAYING)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -574,6 +604,8 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err,
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+
return;
fail:
@@ -592,7 +624,8 @@ static guint resume_gateway(struct media_transport *transport,
GATEWAY_LOCK_WRITE) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return gateway_request_stream(device, gateway_resume_complete,
@@ -625,8 +658,15 @@ static guint suspend_gateway(struct media_transport *transport,
static int id = 1;
if (!owner) {
+ gateway_state_t state = gateway_get_state(device);
+
gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (state == GATEWAY_STATE_PLAYING)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -806,6 +846,8 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
return btd_error_in_progress(msg);
}
+ transport_set_state(transport, TRANSPORT_STATE_SUSPENDING);
+
id = transport->suspend(transport, owner);
if (id == 0) {
media_transport_remove(transport, owner);
@@ -1104,7 +1146,17 @@ static void headset_nrec_changed(struct audio_device *dev, gboolean nrec,
static void transport_update_playing(struct media_transport *transport,
gboolean playing)
{
- DBG("%s Playing=%d", transport->path, playing);
+ DBG("%s State=%s Playing=%d", transport->path,
+ str_state[transport->state], playing);
+
+ if (playing == FALSE) {
+ if (transport->state == TRANSPORT_STATE_ACQUIRED)
+ transport_set_state(transport,
+ TRANSPORT_STATE_POST_ACQUIRE);
+ else if (transport->state == TRANSPORT_STATE_PENDING)
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+ } else if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
}
static void headset_state_changed(struct audio_device *dev,
--
1.7.7.6
^ 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