* Re: [PATCH 1/4] Add the Attribute interface to the API
From: Claudio Takahasi @ 2010-09-29 14:52 UTC (permalink / raw)
To: Claudio Takahasi, linux-bluetooth, Vinicius Costa Gomes
In-Reply-To: <20100929134908.GB19362@jh-x301>
On Wed, Sep 29, 2010 at 10:49 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Tue, Sep 28, 2010, Claudio Takahasi wrote:
>> +Attribute Protocol hierarchy
>> +============================
>> +
>> +Service org.bluez
>> +Interface org.bluez.Attribute
>> +Object path [prefix]/{hci0}/{device0}
>> +
>> +
>> +Methods dict GetProperties()
>> +
>> + Returns all properties for the interface. See the
>> + properties section for available properties.
>> +
>> +Properties array{object} Services
>> +
>> + List of all the Primary Services that this device
>> + implements.
>> +
>> +
>> Device Service hierarchy
>> ========================
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index 95b5b22..b818299 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -139,10 +139,6 @@ Properties string Address [readonly]
>> List of 128-bit UUIDs that represents the available
>> remote services.
>>
>> - array{object} Services [readonly]
>> -
>> - List of characteristics based services.
>> -
>> boolean Paired [readonly]
>>
>> Indicates if the remote device is paired.
>
> What's the motivation of moving this into its own D-Bus interface? I
> thought the plan was to abstract both traditional SDP and ATT behind the
> same API in which case having this in the Device interface makes more
> sense imho.
>
> Johan
>
Hi Johan,
we forgot the SDP integration plan. Forget this patch.
>From the implementation point of view, implement inside the attrib
client plugin was the easiest way, if we keep this property inside the
Device interface a new function needs to be created in device.h to
register the available primary services object paths.
Claudio.
^ permalink raw reply
* Re: Spurious authentication request?
From: Daniel Örstadius @ 2010-09-29 14:39 UTC (permalink / raw)
To: Daniel Örstadius, linux-bluetooth
In-Reply-To: <20100928081513.GA20800@jh-x301>
Hi Johan,
On Tue, Sep 28, 2010 at 11:15 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Daniel,
>
> On Mon, Sep 27, 2010, Daniel Örstadius wrote:
>> Occasionally when pairing with a headset BlueZ issues what looks
>> to be an unnecessary "Authentication Requested" after
>> receiving "Auth Complete" and "Simple Pairing Complete".
>>
>> 2010-09-22 11:23:45.282348 > HCI Event: Auth Complete (0x06) plen 3
>> status 0x00 handle 2
>> 2010-09-22 11:23:45.282378 < HCI Command: Set Connection Encryption
>> (0x01|0x0013) plen 3
>> handle 2 encrypt 0x01
>> [..]
>> 2010-09-22 11:23:45.363646 < HCI Command: Authentication Requested
>> (0x01|0x0011) plen 2
>> handle 2
>>
>> This request is coming from bonding_connect_cb and occurs if that
>> callback happens to be executed before the function
>> hcid_dbus_bonding_process_complete (triggered by the auth complete
>> event). The order between those functions might have been affected
>> by this patch
>>
>> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=be35d00671519b8d92361c2caad9abeedda151f8
>>
>> The request for authentication seems to be used for legacy pairing and
>> should probably not be made for simple pairing. Also, it might be
>> inconsistent that the kernel issues "Authentication Requested" for
>> simple pairing, but bluetoothd does it for legacy?
>>
>> I guess enclosing the call in a check for legacy pairing would
>> work, if that information is or can be made available to
>> bluetoothd.
>>
>> Have I understood the situation correctly and if so, what would
>> be a good way to fix this issue?
>
> The reason why the extra authentication request is there is to deal with
> old kernels that do not initiate authentication by themselves. So
> checking for SSP vs legacy pairing wouldn't work.
>
Ok, thanks.
In newer kernels the auth req seems to be triggered in
hci_event.c/hci_remote_features_evt() only if both sides support SSP.
For legacy pairing it's up to bluetoothd to request authentication.
Because of that, there is an unnecessary "auth req" from bluetoothd in 2.1
pairing if bonding_connect_cb gets called before device_bonding_complete.
The latter function clears the bonding_req in btd_device, causing the former
to return directly.
I think this is the relation between HCI events and those functions in
bluetoothd: [*]
auth complete evt => device_bonding_complete
encrypt change evt => bonding_connect_cb
"auth complete" occurs before "encrypt change", but its corresponding
function doesn't always get called first (maybe because of the low priority
on the raw HCI socket watch in security.c).
As you mentioned offline it might work to query the kernel to find out if
the device is authenticated. I don't know what should be considered to
be the root cause of the issue.
/Daniel
[*] From looking at the sources, the kernel seems respond to the "auth
complete" event with a request for encryption. Could be that
bonding_connect_cb in bluetoothd gets called when the kernel receives
the "encrypt change" event (since the socket state goes to
BT_CONNECTED at that point and bluetoothd is watching the socket for
G_IO_OUT via glib).
^ permalink raw reply
* [PATCH] Fix not detecting errors on small files
From: Luiz Augusto von Dentz @ 2010-09-29 14:27 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
When flushing the last part of buffer/file we have to wait for response
since that can contain errors.
To fix this gw_obex_xfer_close is called in order to force gwobex to
resume the request and return any error related to it.
---
client/transfer.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/client/transfer.c b/client/transfer.c
index 8ceb36f..e5fcfcc 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -360,7 +360,7 @@ static void put_xfer_progress(GwObexXfer *xfer, gpointer user_data)
{
struct transfer_data *transfer = user_data;
struct transfer_callback *callback = transfer->callback;
- gint written, err = 0;
+ gint written = 0, err = 0;
if (transfer->buffer_len == 0) {
transfer->buffer_len = DEFAULT_BUFFER_SIZE;
@@ -380,7 +380,12 @@ static void put_xfer_progress(GwObexXfer *xfer, gpointer user_data)
transfer->filled += len;
if (transfer->filled == 0) {
- gw_obex_xfer_flush(xfer, &err);
+ if (written > 0) {
+ gw_obex_xfer_flush(xfer, &err);
+ return;
+ }
+
+ gw_obex_xfer_close(xfer, &err);
goto done;
}
--
1.7.1
^ permalink raw reply related
* Re: Pull request: git://git.infradead.org/users/cktakahasi/bluez.git for-upstream
From: Claudio Takahasi @ 2010-09-29 14:05 UTC (permalink / raw)
To: Claudio Takahasi, BlueZ development
In-Reply-To: <20100929134521.GA19362@jh-x301>
On Wed, Sep 29, 2010 at 10:45 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Claudio,
>
> On Thu, Sep 23, 2010, Claudio Takahasi wrote:
>> Rename hciops {start, stop}_discovery to {start, stop}_inquiry
>
> I'm not sure about this patch. Wasn't the idea to hide the device
> discovery details from the higher layers? I.e. the method could be
> called discovery and it'd take care of both BR/EDR and LE discovery
> methods.
>
> Johan
>
Hi Johan,
StartDiscovery() triggers all the process. The adapter type: BR/EDR
only/LE only or dual mode is hidden from the higher layers.
So, your suggestion it try to hide all discovery details inside the
hciops as much as possible? I am not sure at the moment if it is
possible, but we can try. We need to clean the code anyway in order to
have a "smooth" migration to the new Bluetooth Management Interface.
Regards,
Claudio
^ permalink raw reply
* [PATCH 2/2] Fix missing comma in cmd_le_str
From: Ville Tervo @ 2010-09-29 13:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ville Tervo
In-Reply-To: <1285768581-19654-1-git-send-email-ville.tervo@nokia.com>
---
parser/hci.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/parser/hci.c b/parser/hci.c
index 7f23605..863afd9 100644
--- a/parser/hci.c
+++ b/parser/hci.c
@@ -381,7 +381,7 @@ static char *cmd_testing_str[CMD_TESTING_NUM + 1] = {
"Unknown",
};
-#define CMD_LE_NUM 30
+#define CMD_LE_NUM 31
static char *cmd_le_str[CMD_LE_NUM + 1] = {
"Unknown",
"LE Set Event Mask",
@@ -403,7 +403,7 @@ static char *cmd_le_str[CMD_LE_NUM + 1] = {
"LE Add Device To White List",
"LE Remove Device From White List",
"LE Connection Update",
- "LE Set Host Channel Classification"
+ "LE Set Host Channel Classification",
"LE Read Channel Map",
"LE Read Remote Used Features",
"LE Encrypt",
--
1.7.1
^ permalink raw reply related
* [PATCH 1/2] Add master/slave parsing for le_conn_complete event
From: Ville Tervo @ 2010-09-29 13:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ville Tervo
---
parser/hci.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/parser/hci.c b/parser/hci.c
index 250ba58..7f23605 100644
--- a/parser/hci.c
+++ b/parser/hci.c
@@ -3239,7 +3239,8 @@ static inline void evt_le_conn_complete_dump(int level, struct frame *frm)
evt_le_connection_complete *evt = frm->ptr;
p_indent(level, frm);
- printf("status 0x%2.2x handle %d\n", evt->status, btohs(evt->handle));
+ printf("status 0x%2.2x handle %d, role %s\n",
+ evt->status, btohs(evt->handle), evt->role?"slave":"master");
}
static inline void le_meta_ev_dump(int level, struct frame *frm)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/4] Add the Attribute interface to the API
From: Johan Hedberg @ 2010-09-29 13:49 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth, Vinicius Costa Gomes
In-Reply-To: <1285696155-32766-1-git-send-email-claudio.takahasi@openbossa.org>
Hi,
On Tue, Sep 28, 2010, Claudio Takahasi wrote:
> +Attribute Protocol hierarchy
> +============================
> +
> +Service org.bluez
> +Interface org.bluez.Attribute
> +Object path [prefix]/{hci0}/{device0}
> +
> +
> +Methods dict GetProperties()
> +
> + Returns all properties for the interface. See the
> + properties section for available properties.
> +
> +Properties array{object} Services
> +
> + List of all the Primary Services that this device
> + implements.
> +
> +
> Device Service hierarchy
> ========================
>
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 95b5b22..b818299 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -139,10 +139,6 @@ Properties string Address [readonly]
> List of 128-bit UUIDs that represents the available
> remote services.
>
> - array{object} Services [readonly]
> -
> - List of characteristics based services.
> -
> boolean Paired [readonly]
>
> Indicates if the remote device is paired.
What's the motivation of moving this into its own D-Bus interface? I
thought the plan was to abstract both traditional SDP and ATT behind the
same API in which case having this in the Device interface makes more
sense imho.
Johan
^ permalink raw reply
* Re: Pull request: git://git.infradead.org/users/cktakahasi/bluez.git for-upstream
From: Johan Hedberg @ 2010-09-29 13:45 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: BlueZ development
In-Reply-To: <AANLkTindKFz1fx9cJMu9aSpehG5LaUHHO7TkgWqMzqj=@mail.gmail.com>
Hi Claudio,
On Thu, Sep 23, 2010, Claudio Takahasi wrote:
> Rename hciops {start, stop}_discovery to {start, stop}_inquiry
I'm not sure about this patch. Wasn't the idea to hide the device
discovery details from the higher layers? I.e. the method could be
called discovery and it'd take care of both BR/EDR and LE discovery
methods.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: haijun liu @ 2010-09-29 13:33 UTC (permalink / raw)
To: Ville Tervo
Cc: Gustavo F. Padovan, linux-bluetooth@vger.kernel.org,
Mat Martineau, dantian.ip
In-Reply-To: <20100929131229.GK1931@null>
Replace hard code of configuration continuation flag with self-comment macro.
Signed-off-by: Haijun.Liu <haijun.liu@atheros.com>
---
net/bluetooth/l2cap_core.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index efcf510..279f98a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2985,11 +2985,11 @@ static inline int l2cap_config_req(struct
l2cap_conn *conn, struct l2cap_cmd_hdr
memcpy(l2cap_pi(sk)->conf_req + l2cap_pi(sk)->conf_len, req->data, len);
l2cap_pi(sk)->conf_len += len;
- if (flags & 0x0001) {
+ if (flags & L2CAP_CONF_FLAG_CONT) {
/* Incomplete config. Send empty response. */
l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
l2cap_build_conf_rsp(sk, rsp,
- L2CAP_CONF_SUCCESS, 0x0001), rsp);
+ L2CAP_CONF_SUCCESS, L2CAP_CONF_FLAG_CONT), rsp);
goto unlock;
}
@@ -3092,7 +3092,7 @@ static inline int l2cap_config_rsp(struct
l2cap_conn *conn, struct l2cap_cmd_hdr
goto done;
}
- if (flags & 0x01)
+ if (flags & L2CAP_CONF_FLAG_CONT)
goto done;
l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: Ville Tervo @ 2010-09-29 13:12 UTC (permalink / raw)
To: ext haijun liu
Cc: Gustavo F. Padovan, linux-bluetooth@vger.kernel.org,
Mat Martineau, dantian.ip
In-Reply-To: <AANLkTi=tyZ4rsn8QtqV8b04=f=1QA9qwJW03PQzroJ7O@mail.gmail.com>
Hi,
On Wed, Sep 29, 2010 at 02:42:35PM +0200, ext haijun liu wrote:
> Hi Ville,
> Do you mean l2cap_config_rsp()?
No. I meant change like this in addition to your changes.
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index efcf510..7a9c194 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2989,7 +2989,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
/* Incomplete config. Send empty response. */
l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
l2cap_build_conf_rsp(sk, rsp,
- L2CAP_CONF_SUCCESS, 0x0001), rsp);
+ L2CAP_CONF_SUCCESS, L2CAP_CONF_FLAG_CONT), rsp);
goto unlock;
}
> I made it, but I don't why gmail hide that part.
I got the whole message.
--
Ville
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: haijun liu @ 2010-09-29 12:42 UTC (permalink / raw)
To: Ville Tervo
Cc: Gustavo F. Padovan, linux-bluetooth@vger.kernel.org,
Mat Martineau, dantian.ip
In-Reply-To: <20100929113615.GH1931@null>
Hi Ville,
Do you mean l2cap_config_rsp()?
I made it, but I don't why gmail hide that part.
--
Haijun Liu
^ permalink raw reply
* HDP development status and collaboration
From: Elvis Pfützenreuter @ 2010-09-29 12:14 UTC (permalink / raw)
To: linux-bluetooth
Cc: Jarvenpaa Jarmo (Nokia/Oulu), Aldenor Martins, Johan Hedberg,
Jose Antonio Santos Cadenas, Santiago Carot-Nemesio
[-- Attachment #1: Type: text/plain, Size: 963 bytes --]
Attached is a spreadsheet with a simple breakdown of HDP development tasks. Managers like spreadsheets, most of us don't, so the most relevant part -- the pending tasks -- are listed below for convenience:
device.Echo PENDING
device.CreateChannel IN PROGRESS (OpenHealth)
device.DestroyChannel PENDING
device.ChannelConnected PENDING
device.ChannelDeleted PENDING
device properties PENDING
Channel object PENDING
channel.Acquire PENDING
channel.Release PENDING
channel properties PENDING
HDP verification against PTS PENDING (depends on the rest)
I'd say that we are getting near completion. Would be nearer if we could distribute those tasks; currently, only OpenHealth friends are working on HDP code, in their repository.
I know it's sometimes difficult to collaborate while working on the very same source files, but we should try; the breakdown by API calls seem to be the way to divide work among available developers.
[-- Attachment #2: hdp_development.xls --]
[-- Type: application/vnd.ms-excel, Size: 10752 bytes --]
^ permalink raw reply
* Re: a2dp, myth, pulse
From: Sander van Grieken @ 2010-09-29 11:37 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <AANLkTim27JkfgsumEwd3jK+n1DzYvTYWJGCLGreuw7y3@mail.gmail.com>
On Tuesday 28 September 2010 09:05:18 Brad Midgley wrote:
> Hey
>
> > can avrcp be used to drive mythtv? I've got a setup where the myth
> > frontend is rarely the foreground app
>
> So if avrcp were to present events in a /dev/input/eventX device, I
> could use inputlircd to connect those events to lirc clients. It might
> make it easier if a udev rule created something like /dev/input/avrcp0
> so we could find it more easily for the inputlircd config.
>
> And I see now that it is by virtue of the use of liblirc that myth can
> get remote events even if it's not the foreground app.
>
> Control is enabled by default in 4.60, is there anything I can check
> to see why I don't see any log messages about avrcp, no input device
> appear, nothing logged, etc? The main connection is initiated by the
> headset and the audio connection is initiated by the computer. If I
> remember tinkering with this stuff, bluez would need to initiate the
> control connection in this case.
Currently there is support in bluez to act as a AVRCP target, and it will deliver these
events through the input layer. No new input device will appear, it will go through
/dev/uinput (which I think you can open for reading then)
The Control connection is ONLY (implicitly) established when an audio connection
(HS,HF,A2DP) is succesfully started, so you should be able to control myth (through lirc
perhaps) with a headset that has some controls. If this is done, you should be able to see
some debug logging w.r.t. RCP. You probably need to run bluetoothd using -d -n.
--
Sander
^ permalink raw reply
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: Ville Tervo @ 2010-09-29 11:36 UTC (permalink / raw)
To: ext haijun liu
Cc: Gustavo F. Padovan, linux-bluetooth@vger.kernel.org,
Mat Martineau, dantian.ip
In-Reply-To: <AANLkTi=EVEwuKy3QwNJ1sVpe2_EJCkQQ3iGutKQQRneU@mail.gmail.com>
Hi,
Patch is ok but just one comment. See below.
On Wed, Sep 29, 2010 at 01:12:39PM +0200, haijun liu wrote:
>
> Replace hard code of configuration continuation flag with self-comment macro.
>
> - if (flags & 0x0001) {
> + if (flags & L2CAP_CONF_FLAG_CONT) {
> /* Incomplete config. Send empty response. */
> l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
> l2cap_build_conf_rsp(sk, rsp,
Maybe you could use L2CAP_CONF_FLAG_CONT also in l2cap_build_conf_rsp()?
--
Ville
^ permalink raw reply
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: haijun liu @ 2010-09-29 11:12 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth, Mat Martineau, dantian.ip
In-Reply-To: <20100928234820.GB8518@vigoh>
Replace hard code of configuration continuation flag with self-comment macro.
Signed-off-by: Haijun.Liu <haijun.liu@atheros.com>
---
include/net/bluetooth/l2cap.h | 2 ++
net/bluetooth/l2cap_core.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index df599dc..2b114ca 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -193,6 +193,8 @@ struct l2cap_conf_rsp {
#define L2CAP_CONF_REJECT 0x0002
#define L2CAP_CONF_UNKNOWN 0x0003
+#define L2CAP_CONF_FLAG_CONT 0x0001
+
struct l2cap_conf_opt {
__u8 type;
__u8 len;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index efcf510..bef5c9f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2985,7 +2985,7 @@ static inline int l2cap_config_req(struct
l2cap_conn *conn, struct l2cap_cmd_hdr
memcpy(l2cap_pi(sk)->conf_req + l2cap_pi(sk)->conf_len, req->data, len);
l2cap_pi(sk)->conf_len += len;
- if (flags & 0x0001) {
+ if (flags & L2CAP_CONF_FLAG_CONT) {
/* Incomplete config. Send empty response. */
l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
l2cap_build_conf_rsp(sk, rsp,
@@ -3092,7 +3092,7 @@ static inline int l2cap_config_rsp(struct
l2cap_conn *conn, struct l2cap_cmd_hdr
goto done;
}
- if (flags & 0x01)
+ if (flags & L2CAP_CONF_FLAG_CONT)
goto done;
l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 2/2] Bluetooth: Update conf_state before send config_req out.
From: haijun liu @ 2010-09-29 11:11 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth@vger.kernel.org, dantian.ip
In-Reply-To: <20100928234934.GC8518@vigoh>
Update conf_state with L2CAP_CONF_REQ_SENT before send
config_req out in l2cap_config_req().
Signed-off-by: Haijun.Liu <haijun.liu@atheros.com>
---
net/bluetooth/l2cap_core.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index bef5c9f..f461fee 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3026,6 +3026,7 @@ static inline int l2cap_config_req(struct
l2cap_conn *conn, struct l2cap_cmd_hdr
if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_REQ_SENT)) {
u8 buf[64];
+ l2cap_pi(sk)->conf_state |= L2CAP_CONF_REQ_SENT;
l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_CONF_REQ,
l2cap_build_conf_req(sk, buf), buf);
l2cap_pi(sk)->num_conf_req++;
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 4/6] Bluetooth: Fix inconsistent lock state with RFCOMM
From: Yuri Kululin @ 2010-09-29 7:41 UTC (permalink / raw)
To: ext Gustavo F. Padovan; +Cc: linux-bluetooth, Gustavo F. Padovan
In-Reply-To: <1285376504-3541-5-git-send-email-gustavo@padovan.org>
ext Gustavo F. Padovan wrote:
> From: Gustavo F. Padovan <padovan@profusion.mobi>
>
> When receiving a rfcomm connection with the old dund deamon a
> inconsistent lock state happens. That's because interrupts were already
> disabled by l2cap_conn_start() when rfcomm_sk_state_change() try to lock
> the spin_lock.
>
> As result we may have a inconsistent lock state for l2cap_conn_start()
> after rfcomm_sk_state_change() calls bh_lock_sock() and disable interrupts
> as well.
>
> [ 2833.151999]
> [ 2833.151999] =================================
> [ 2833.151999] [ INFO: inconsistent lock state ]
> [ 2833.151999] 2.6.36-rc3 #2
> [ 2833.151999] ---------------------------------
> [ 2833.151999] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
> [ 2833.151999] krfcommd/2306 [HC0[0]:SC0[0]:HE1:SE1] takes:
> [ 2833.151999] (slock-AF_BLUETOOTH){+.?...}, at: [<ffffffffa00bcb56>] rfcomm_sk_state_change+0x46/0x170 [rfcomm]
> [ 2833.151999] {IN-SOFTIRQ-W} state was registered at:
> [ 2833.151999] [<ffffffff81094346>] __lock_acquire+0x5b6/0x1560
> [ 2833.151999] [<ffffffff8109534a>] lock_acquire+0x5a/0x70
> [ 2833.151999] [<ffffffff81392b6c>] _raw_spin_lock+0x2c/0x40
> [ 2833.151999] [<ffffffffa00a5092>] l2cap_conn_start+0x92/0x640 [l2cap]
> [ 2833.151999] [<ffffffffa00a6a3f>] l2cap_sig_channel+0x6bf/0x1320 [l2cap]
> [ 2833.151999] [<ffffffffa00a9173>] l2cap_recv_frame+0x133/0x770 [l2cap]
> [ 2833.151999] [<ffffffffa00a997b>] l2cap_recv_acldata+0x1cb/0x390 [l2cap]
> [ 2833.151999] [<ffffffffa000db4b>] hci_rx_task+0x2ab/0x450 [bluetooth]
> [ 2833.151999] [<ffffffff8106b22b>] tasklet_action+0xcb/0xe0
> [ 2833.151999] [<ffffffff8106b91e>] __do_softirq+0xae/0x150
> [ 2833.151999] [<ffffffff8102bc0c>] call_softirq+0x1c/0x30
> [ 2833.151999] [<ffffffff8102ddb5>] do_softirq+0x75/0xb0
> [ 2833.151999] [<ffffffff8106b56d>] irq_exit+0x8d/0xa0
> [ 2833.151999] [<ffffffff8104484b>] smp_apic_timer_interrupt+0x6b/0xa0
> [ 2833.151999] [<ffffffff8102b6d3>] apic_timer_interrupt+0x13/0x20
> [ 2833.151999] [<ffffffff81029dfa>] cpu_idle+0x5a/0xb0
> [ 2833.151999] [<ffffffff81381ded>] rest_init+0xad/0xc0
> [ 2833.151999] [<ffffffff817ebc4d>] start_kernel+0x2dd/0x2e8
> [ 2833.151999] [<ffffffff817eb2e6>] x86_64_start_reservations+0xf6/0xfa
> [ 2833.151999] [<ffffffff817eb3ce>] x86_64_start_kernel+0xe4/0xeb
> [ 2833.151999] irq event stamp: 731
> [ 2833.151999] hardirqs last enabled at (731): [<ffffffff8106b762>] local_bh_enable_ip+0x82/0xe0
> [ 2833.151999] hardirqs last disabled at (729): [<ffffffff8106b93e>] __do_softirq+0xce/0x150
> [ 2833.151999] softirqs last enabled at (730): [<ffffffff8106b96e>] __do_softirq+0xfe/0x150
> [ 2833.151999] softirqs last disabled at (711): [<ffffffff8102bc0c>] call_softirq+0x1c/0x30
> [ 2833.151999]
> [ 2833.151999] other info that might help us debug this:
> [ 2833.151999] 2 locks held by krfcommd/2306:
> [ 2833.151999] #0: (rfcomm_mutex){+.+.+.}, at: [<ffffffffa00bb744>] rfcomm_run+0x174/0xb20 [rfcomm]
> [ 2833.151999] #1: (&(&d->lock)->rlock){+.+...}, at: [<ffffffffa00b9223>] rfcomm_dlc_accept+0x53/0x100 [rfcomm]
> [ 2833.151999]
> [ 2833.151999] stack backtrace:
> [ 2833.151999] Pid: 2306, comm: krfcommd Tainted: G W 2.6.36-rc3 #2
> [ 2833.151999] Call Trace:
> [ 2833.151999] [<ffffffff810928e1>] print_usage_bug+0x171/0x180
> [ 2833.151999] [<ffffffff810936c3>] mark_lock+0x333/0x400
> [ 2833.151999] [<ffffffff810943ca>] __lock_acquire+0x63a/0x1560
> [ 2833.151999] [<ffffffff810948b5>] ? __lock_acquire+0xb25/0x1560
> [ 2833.151999] [<ffffffff8109534a>] lock_acquire+0x5a/0x70
> [ 2833.151999] [<ffffffffa00bcb56>] ? rfcomm_sk_state_change+0x46/0x170 [rfcomm]
> [ 2833.151999] [<ffffffff81392b6c>] _raw_spin_lock+0x2c/0x40
> [ 2833.151999] [<ffffffffa00bcb56>] ? rfcomm_sk_state_change+0x46/0x170 [rfcomm]
> [ 2833.151999] [<ffffffffa00bcb56>] rfcomm_sk_state_change+0x46/0x170 [rfcomm]
> [ 2833.151999] [<ffffffffa00b9239>] rfcomm_dlc_accept+0x69/0x100 [rfcomm]
> [ 2833.151999] [<ffffffffa00b9a49>] rfcomm_check_accept+0x59/0xd0 [rfcomm]
> [ 2833.151999] [<ffffffffa00bacab>] rfcomm_recv_frame+0x9fb/0x1320 [rfcomm]
> [ 2833.151999] [<ffffffff813932bb>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
> [ 2833.151999] [<ffffffff81093acd>] ? trace_hardirqs_on_caller+0x13d/0x180
> [ 2833.151999] [<ffffffff81093b1d>] ? trace_hardirqs_on+0xd/0x10
> [ 2833.151999] [<ffffffffa00bb7f1>] rfcomm_run+0x221/0xb20 [rfcomm]
> [ 2833.151999] [<ffffffff813905e7>] ? schedule+0x287/0x780
> [ 2833.151999] [<ffffffffa00bb5d0>] ? rfcomm_run+0x0/0xb20 [rfcomm]
> [ 2833.151999] [<ffffffff81081026>] kthread+0x96/0xa0
> [ 2833.151999] [<ffffffff8102bb14>] kernel_thread_helper+0x4/0x10
> [ 2833.151999] [<ffffffff813936bc>] ? restore_args+0x0/0x30
> [ 2833.151999] [<ffffffff81080f90>] ? kthread+0x0/0xa0
> [ 2833.151999] [<ffffffff8102bb10>] ? kernel_thread_helper+0x0/0x10
>
> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> ---
> net/bluetooth/rfcomm/sock.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 44a6232..194b3a0 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -82,11 +82,14 @@ static void rfcomm_sk_data_ready(struct rfcomm_dlc *d, struct sk_buff *skb)
> static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
> {
> struct sock *sk = d->owner, *parent;
> + unsigned long flags;
> +
> if (!sk)
> return;
>
> BT_DBG("dlc %p state %ld err %d", d, d->state, err);
>
> + local_irq_save(flags);
> bh_lock_sock(sk);
>
> if (err)
> @@ -108,6 +111,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
> }
>
> bh_unlock_sock(sk);
> + local_irq_restore(flags);
>
> if (parent && sock_flag(sk, SOCK_ZAPPED)) {
> /* We have to drop DLC lock here, otherwise
I've faced with the similar warning and looks like this patch fixes the issue.
[ 2917.827178] =================================
[ 2917.833068] [ INFO: inconsistent lock state ]
[ 2917.837432] 2.6.32-09421-g65e7ba7 #54
[ 2917.841125] ---------------------------------
[ 2917.845520] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[ 2917.851562] krfcommd/1516 [HC0[0]:SC0[0]:HE1:SE1] takes:
[ 2917.856903] (slock-AF_BLUETOOTH){+.?...}, at: [<bf057b50>]
rfcomm_sk_state_change+0x78/0x160 [rfcomm]
[ 2917.866363] {IN-SOFTIRQ-W} state was registered at:
[ 2917.871276] [<c008d33c>] mark_lock+0x298/0x630
[ 2917.875946] [<c008ed3c>] __lock_acquire+0x5f4/0x175c
[ 2917.881134] [<c008ff0c>] lock_acquire+0x68/0x7c
[ 2917.885864] [<c036938c>] _spin_lock+0x48/0x58
[ 2917.890441] [<bf025960>] l2cap_conn_start+0x80/0x388 [l2cap]
[ 2917.896362] [<bf028f44>] l2cap_recv_frame+0x1c58/0x2fe0 [l2cap]
[ 2917.902526] [<bf02a3cc>] l2cap_recv_acldata+0x100/0x350 [l2cap]
[ 2917.908691] [<bf0035a8>] hci_rx_task+0x244/0x478 [bluetooth]
[ 2917.914642] [<c006c4d0>] tasklet_action+0x78/0xd8
[ 2917.919555] [<c006cc34>] __do_softirq+0xa8/0x154
[ 2917.924407] [<c006cd40>] irq_exit+0x60/0xb4
[ 2917.928802] [<c0030078>] asm_do_IRQ+0x78/0x90
[ 2917.933380] [<c0030af0>] __irq_svc+0x50/0xbc
[ 2917.937866] [<c0043c74>] omap3_enter_idle_bm+0x1d0/0x238
[ 2917.943389] [<c029ee94>] cpuidle_idle_call+0xb4/0x114
[ 2917.948669] [<c00320b0>] cpu_idle+0x58/0xac
[ 2917.953063] [<c0360b18>] rest_init+0x70/0x84
[ 2917.957550] [<c00089fc>] start_kernel+0x2b4/0x318
[ 2917.962493] [<80008034>] 0x80008034
[ 2917.966186] irq event stamp: 312
[ 2917.969421] hardirqs last enabled at (312): [<c03691ac>]
_spin_unlock_irqrestore+0x44/0x70
[ 2917.977844] hardirqs last disabled at (311): [<c036947c>]
_spin_lock_irqsave+0x24/0x68
[ 2917.985809] softirqs last enabled at (261): [<c006ccc8>]
__do_softirq+0x13c/0x154
[ 2917.993438] softirqs last disabled at (244): [<c006cde8>]
do_softirq+0x54/0x78
[ 2918.000732]
[ 2918.000732] other info that might help us debug this:
[ 2918.007293] 2 locks held by krfcommd/1516:
[ 2918.011413] #0: (rfcomm_mutex){+.+.+.}, at: [<bf054df4>]
rfcomm_run+0x1f0/0xb00 [rfcomm]
[ 2918.019805] #1: (&d->lock){+.+...}, at: [<bf055220>]
rfcomm_run+0x61c/0xb00 [rfcomm]
[ 2918.027832]
[ 2918.027832] stack backtrace:
[ 2918.032226] Backtrace:
[ 2918.034729] [<c00348d0>] (dump_backtrace+0x0/0x110) from [<c036616c>]
(dump_stack+0x18/0x1c)
[ 2918.043212] r7:dc8f6c00 r6:c0425252 r5:00000001 r4:00000001
[ 2918.048950] [<c0366154>] (dump_stack+0x0/0x1c) from [<c008d060>]
(print_usage_bug+0x178/0x1bc)
[ 2918.057617] [<c008cee8>] (print_usage_bug+0x0/0x1bc) from [<c008d408>]
(mark_lock+0x364/0x630)
[ 2918.066284] [<c008d0a4>] (mark_lock+0x0/0x630) from [<c008edcc>]
(__lock_acquire+0x684/0x175c)
[ 2918.074951] [<c008e748>] (__lock_acquire+0x0/0x175c) from [<c008ff0c>]
(lock_acquire+0x68/0x7c)
[ 2918.083709] [<c008fea4>] (lock_acquire+0x0/0x7c) from [<c036938c>]
(_spin_lock+0x48/0x58)
[ 2918.091949] r7:dba9402c r6:dba5c3c0 r5:dba9402c r4:bf057b50
[ 2918.097717] [<c0369344>] (_spin_lock+0x0/0x58) from [<bf057b50>]
(rfcomm_sk_state_change+0x78/0x160 [rfcomm])
[ 2918.107696] r5:dba94000 r4:00000000
[ 2918.111358] [<bf057ad8>] (rfcomm_sk_state_change+0x0/0x160 [rfcomm]) from
[<bf055238>] (rfcomm_run+0x634/0xb00 [rfcomm])
[ 2918.122283] r7:dba5c450 r6:dba5d6c0 r5:dba5c3c0 r4:dba5c430
[ 2918.128051] [<bf054c04>] (rfcomm_run+0x0/0xb00 [rfcomm]) from [<c007cc10>]
(kthread+0x88/0x90)
[ 2918.136749] [<c007cb88>] (kthread+0x0/0x90) from [<c006a86c>]
(do_exit+0x0/0x678)
[ 2918.144256] r7:00000000 r6:00000000 r5:00000000 r4:00000000
Thanks,
Yuri K.
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: set sk_sndbuf and sk_rcvbuf properly in ERTM
From: Gustavo F. Padovan @ 2010-09-29 0:32 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth
In-Reply-To: <alpine.DEB.2.00.1009271355090.27215@linux-sea-02>
Hi Mat,
* Mat Martineau <mathewm@codeaurora.org> [2010-09-27 14:22:07 -0700]:
>
> Gustavo -
>
> On Mon, 27 Sep 2010, Gustavo F. Padovan wrote:
>
> > Setting both this value to MPS * TxWin * 1.2 guarantees that we are
> > reserving space to fit the whole txwindow in the memory, and that
> > sendmsg() will block when the transmission window is full avoid
> > overloading the system memory.
> > I don't have a strong reason about the 1.2 constant in the account, we
> > can do another tests in the future and change that value.
> >
> > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > ---
> > net/bluetooth/l2cap_core.c | 10 +++++++++-
> > 1 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 44aa034..1e2ab05 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -3129,9 +3129,17 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > l2cap_pi(sk)->next_tx_seq = 0;
> > l2cap_pi(sk)->expected_tx_seq = 0;
> > __skb_queue_head_init(TX_QUEUE(sk));
> > - if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM)
> > + if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM) {
> > l2cap_ertm_init(sk);
> >
> > + sk->sk_sndbuf = (l2cap_pi(sk)->remote_tx_win * 1.2 *
> > + (sizeof(struct l2cap_pinfo) +
> > + l2cap_pi(sk)->mps));
> > + sk->sk_rcvbuf = (l2cap_pi(sk)->tx_win * 1.2 *
> > + (sizeof(struct l2cap_pinfo) +
> > + l2cap_pi(sk)->remote_mps));
> > + }
> > +
> > l2cap_chan_ready(sk);
> > }
> >
> > --
> > 1.7.3
Thanks a lot for your comments, I'll rework the patches and then resend.
>
> I think sizeof(struct sk_buff) would be better than
> sizeof(struct l2cap_pinfo), since these limits apply to data buffers,
> not per-socket overhead.
Sure.
>
> The 1.2 constant would need to be increased if we allow ERTM MPS
> bigger than the HCI MTU, since there would be multiple sk_buffs per
> PDU. However, the calculation could be updated when those MPS changes
> are made.
Yes, we can change that when we start to allow MPS greater than HCI MTU.
>
> It would also help to enforce some limits:
> SOCK_MIN_SNDBUF < sk->sk_sndbuf < sysctl_wmem_max
> SOCK_MIN_RCVBUF < sk->sk_rcvbuf < sysctl_rmem_max
Sure.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: Update conf_state before send config_req out.
From: Gustavo F. Padovan @ 2010-09-28 23:49 UTC (permalink / raw)
To: haijun liu; +Cc: linux-bluetooth@vger.kernel.org, dantian.ip
In-Reply-To: <AANLkTimQtkORjmsiO3wPULRErNXORBtHATbBvdCEsY27@mail.gmail.com>
Hi Haijun,
* haijun liu <liuhaijun.er@gmail.com> [2010-09-20 09:33:31 +0800]:
> Update conf_state with L2CAP_CONF_REQ_SENT before send config_req out.
>
>
> Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
Both patches are corrupted, please fix that and rebase it against
http://git.kernel.org/?p=linux/kernel/git/padovan/bluetooth-next-2.6.git
Thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH] Bluetooth: Replace hard code of configuration continuation flag.
From: Gustavo F. Padovan @ 2010-09-28 23:48 UTC (permalink / raw)
To: haijun liu; +Cc: linux-bluetooth, Mat Martineau, dantian.ip
In-Reply-To: <AANLkTi=mS2212wTd3nu_ENJ8ySyNSVMWgTmc-MT0421c@mail.gmail.com>
Hi Haijun,
* haijun liu <liuhaijun.er@gmail.com> [2010-09-16 17:21:40 +0800]:
> Replace hard code of configuration continuation flag with self-comment macro.
>
> Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
Your patch is corrupted, please fix that and rebase it against
http://git.kernel.org/?p=linux/kernel/git/padovan/bluetooth-next-2.6.git
Thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCHv4 3/3] Bluetooth: check L2CAP length in first ACL fragment
From: Gustavo F. Padovan @ 2010-09-28 23:45 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
In-Reply-To: <1284550124-31201-4-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-09-15 14:28:44 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> Current Bluetooth code assembles fragments of big L2CAP packets
> in l2cap_recv_acldata and then checks allowed L2CAP size in
> assemled L2CAP packet (pi->imtu < skb->len).
>
> The patch moves allowed L2CAP size check to the early stage when
> we receive the first fragment of L2CAP packet. We do not need to
> reserve and keep L2CAP fragments for bad packets.
>
> Updated version after comments from Mat Martineau <mathewm@codeaurora.org>
> and Gustavo Padovan <padovan@profusion.mobi>.
>
> Trace below is received when using stress tools sending big
> fragmented L2CAP packets.
> ...
> [ 1712.798492] swapper: page allocation failure. order:4, mode:0x4020
> [ 1712.804809] [<c0031870>] (unwind_backtrace+0x0/0xdc) from [<c00a1f70>]
> (__alloc_pages_nodemask+0x4)
> [ 1712.814666] [<c00a1f70>] (__alloc_pages_nodemask+0x47c/0x4d4) from
> [<c00a1fd8>] (__get_free_pages+)
> [ 1712.824645] [<c00a1fd8>] (__get_free_pages+0x10/0x3c) from [<c026eb5c>]
> (__alloc_skb+0x4c/0xfc)
> [ 1712.833465] [<c026eb5c>] (__alloc_skb+0x4c/0xfc) from [<bf28c738>]
> (l2cap_recv_acldata+0xf0/0x1f8 )
> [ 1712.843322] [<bf28c738>] (l2cap_recv_acldata+0xf0/0x1f8 [l2cap]) from
> [<bf0094ac>] (hci_rx_task+0x)
> ...
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
> net/bluetooth/l2cap.c | 16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
Patch 1/3 was applied to my bluetooth-2.6 tree and 2/3 and 3/3 to
bluetooth-next-2.6. Thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: bluetooth: add support for btusb devices in recent MacBook Pros
From: Gustavo F. Padovan @ 2010-09-28 22:58 UTC (permalink / raw)
To: Will Woods; +Cc: Marcel Holtmann, linux-bluetooth, kernel
In-Reply-To: <1284757761-11994-1-git-send-email-wwoods@redhat.com>
Hi Will,
* Will Woods <wwoods@redhat.com> [2010-09-17 17:09:19 -0400]:
> These two patches add support for the USB bluetooth controllers found
> in recent MacBook Pro systems. In both cases the device class is
> ff (vend.) rather than the usual e0. The iMac11,1 has the same
> problem and is handled with an explicit USB_DEVICE() entry, so
> these two devices are handled the same way.
We already have these patches, yours are a duplicate. Thanks anyway. ;)
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: pull-request: bluetooth-2.6 2010-09-27
From: Gustavo F. Padovan @ 2010-09-28 22:49 UTC (permalink / raw)
To: David Miller; +Cc: linville, marcel, linux-bluetooth, netdev
In-Reply-To: <20100927.200016.226762808.davem@davemloft.net>
* David Miller <davem@davemloft.net> [2010-09-27 20:00:16 -0700]:
> From: "Gustavo F. Padovan" <padovan@profusion.mobi>
> Date: Mon, 27 Sep 2010 23:30:35 -0300
>
> > And a fix for a deadlock issue between the sk_sndbuf and the backlog
> > queue in ERTM. The rest are also needed bug fixes.
>
> This fix is still under discussion.
>
> That change effects quite a few code paths. And when I looked
> at them, I was not at all convinced that dropping the socket
> lock like that is safe.
>
> Are you sure there are no pieces of socket or socket related state
> that might change under us while we drop that lock, which would thus
> make the operation suddenly invalid or cause a state corruption or
> crash?
We can group all the code paths in only two different code paths. One
wirh SCO, L2CAP Basic Mode and L2CAP Streaming Mode once they are very
similar and other for ERTM, a more complicated protocol.
For the first group the only bottom half action we have are incoming data,
which doesn't affect the sk states, and disconnection request, that can
change the sk states. We guarantee that this won't affect by checking the
sk_err after get the lock again. Looking to the code again we might
also want to check the sk->sk_shutdown value like TCP does inside
sk_stream_wait_memory().
Actually sk_stream_wait_memory is another point why it's safe to release
the lock and block waiting for memory. We've been doing that safely in
protocols like TCP, SCTP and DCCP for a long time.
Back to patch, the other code path it affects is the ERTM one, besides
the incoming data we have other bottom halves actions, but in the end the
only action that can affect ERTM flow is closing the channeli, but we are
prepared for that by checking the sk->sk_err and sk->sk_shutdown when we
get the lock back.
---
Bluetooth: Fix deadlock in the ERTM logic
The Enhanced Retransmission Mode(ERTM) is a realiable mode of operation
of the Bluetooth L2CAP layer. Think on it like a simplified version of
TCP.
The problem we were facing here was a deadlock. ERTM uses a backlog
queue to queue incomimg packets while the user is helding the lock. At
some moment the sk_sndbuf can be exceeded and we can't alloc new skbs
then the code sleep with the lock to wait for memory, that stalls the
ERTM connection once we can't read the acknowledgements packets in the
backlog queue to free memory and make the allocation of outcoming skb
successful.
This patch actually affect all users of bt_skb_send_alloc(), i.e., all
L2CAP modes and SCO.
We are safe against socket states changes or channels deletion while the
we are sleeping wait memory. Checking for the sk->sk_err and
sk->sk_shutdown make the code safe, since any action that can leave the
socket or the channel in a not usable state set one of the struct
members at least. Then we can check both of them when getting the lock
again and return with the proper error if something unexpected happens.
Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
Signed-off-by: Ulisses Furquim <ulisses@profusion.mobi>
---
include/net/bluetooth/bluetooth.h | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a902d..e8d64ba 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -161,12 +161,30 @@ static inline struct sk_buff *bt_skb_send_alloc(struct sock *sk, unsigned long l
{
struct sk_buff *skb;
+ release_sock(sk);
if ((skb = sock_alloc_send_skb(sk, len + BT_SKB_RESERVE, nb, err))) {
skb_reserve(skb, BT_SKB_RESERVE);
bt_cb(skb)->incoming = 0;
}
+ lock_sock(sk);
+
+ if (!skb && *err)
+ return NULL;
+
+ *err = sock_error(sk);
+ if (*err)
+ goto out;
+
+ if (sk->sk_shutdown) {
+ *err = ECONNRESET;
+ goto out;
+ }
return skb;
+
+out:
+ kfree_skb(skb);
+ return NULL;
}
int bt_err(__u16 code);
--
1.7.3
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply related
* [PATCH 4/4] Fix object path in the register watcher method
From: Claudio Takahasi @ 2010-09-28 17:49 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1285696155-32766-1-git-send-email-claudio.takahasi@openbossa.org>
The path argument in RegisterCharacteristicsWatcher method shall be
the agent's path. An agent will monitor changes in all characteristics.
---
doc/attribute-api.txt | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/doc/attribute-api.txt b/doc/attribute-api.txt
index d0ce6f8..f66e9bc 100644
--- a/doc/attribute-api.txt
+++ b/doc/attribute-api.txt
@@ -69,18 +69,15 @@ Methods dict GetProperties()
Returns all properties for the interface. See the
Properties section for the available properties.
- RegisterCharacteristicsWatcher(object path)
+ RegisterCharacteristicsWatcher(object agent)
- Register a watcher for changes in specific characteristics
- to monitor changes.
+ Register a watcher to monitor characteristic changes.
A watcher will be registered for this service and will
- notifier about any changed characteristics in the service.
+ notify about any changed characteristics in the service.
This also notifies about any included characteristics.
- Method for the watch objects still need to be defined.
-
- UnregisterCharacteristicsWatcher(object path)
+ UnregisterCharacteristicsWatcher(object agent)
Unregister a watcher.
--
1.7.3
^ permalink raw reply related
* [PATCH 3/4] Add SetProperty in the Device characteristic hierarchy
From: Claudio Takahasi @ 2010-09-28 17:49 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1285696155-32766-1-git-send-email-claudio.takahasi@openbossa.org>
Add SetProperty method to allow characteristic watchers or any other
D-Bus client application to change the value of a given characteristic.
Use cases: Changing sensors thresholds, client characteristic
configuration for notification/indication.
---
doc/attribute-api.txt | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/doc/attribute-api.txt b/doc/attribute-api.txt
index a5e475b..d0ce6f8 100644
--- a/doc/attribute-api.txt
+++ b/doc/attribute-api.txt
@@ -118,6 +118,13 @@ Methods dict GetProperties()
Returns all properties for the characteristic. See the
properties section for available properties.
+ void SetProperty(string name, variant value)
+
+ Changes the value of the specified property. Only
+ read-write properties can be changed. On success
+ this will emit a PropertyChanged signal.
+
+ Possible Errors: org.bluez.Error.InvalidArguments
Properties string UUID [readonly]
--
1.7.3
^ 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