* [PATCH v2 3/9] Remove btd_device_add_service function
From: Claudio Takahasi @ 2011-04-11 18:24 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-3-git-send-email-claudio.takahasi@openbossa.org>
btd_device_add_service is no longer necessary if the object paths for
the primary services can be returned during the registration.
---
attrib/client.c | 16 ++++++++--------
attrib/client.h | 2 +-
src/device.c | 11 ++---------
src/device.h | 1 -
4 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/attrib/client.c b/attrib/client.c
index 28e5704..2dd70c9 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -1020,11 +1020,11 @@ static GDBusMethodTable prim_methods[] = {
{ }
};
-static void register_primaries(struct gatt_service *gatt, GSList *primaries)
+static GSList *register_primaries(struct gatt_service *gatt, GSList *primaries)
{
- GSList *l;
+ GSList *l, *paths;
- for (l = primaries; l; l = l->next) {
+ for (paths = NULL, l = primaries; l; l = l->next) {
struct att_primary *att = l->data;
struct primary *prim;
@@ -1040,12 +1040,14 @@ static void register_primaries(struct gatt_service *gatt, GSList *primaries)
DBG("Registered: %s", prim->path);
gatt->primary = g_slist_append(gatt->primary, prim);
- btd_device_add_service(gatt->dev, prim->path);
+ paths = g_slist_append(paths, g_strdup(prim->path));
load_characteristics(prim, gatt);
}
+
+ return paths;
}
-int attrib_client_register(DBusConnection *connection,
+GSList *attrib_client_register(DBusConnection *connection,
struct btd_device *device, int psm,
GAttrib *attrib, GSList *primaries)
{
@@ -1069,11 +1071,9 @@ int attrib_client_register(DBusConnection *connection,
if (attrib)
gatt->attrib = g_attrib_ref(attrib);
- register_primaries(gatt, primaries);
-
gatt_services = g_slist_append(gatt_services, gatt);
- return 0;
+ return register_primaries(gatt, primaries);
}
void attrib_client_unregister(struct btd_device *device)
diff --git a/attrib/client.h b/attrib/client.h
index b4a4ecc..b29797c 100644
--- a/attrib/client.h
+++ b/attrib/client.h
@@ -22,7 +22,7 @@
*
*/
-int attrib_client_register(DBusConnection *connection,
+GSList *attrib_client_register(DBusConnection *connection,
struct btd_device *device, int psm,
GAttrib *attrib, GSList *primaries);
void attrib_client_unregister(struct btd_device *device);
diff --git a/src/device.c b/src/device.c
index f9b7a73..efe9938 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2336,18 +2336,11 @@ void device_set_authorizing(struct btd_device *device, gboolean auth)
device->authorizing = auth;
}
-void btd_device_add_service(struct btd_device *device, const char *path)
-{
- if (g_slist_find_custom(device->services, path, (GCompareFunc) strcmp))
- return;
-
- device->services = g_slist_append(device->services, g_strdup(path));
-}
-
void device_register_services(DBusConnection *conn, struct btd_device *device,
GSList *prim_list, int psm)
{
- attrib_client_register(conn, device, psm, NULL, prim_list);
+ device->services = attrib_client_register(conn, device, psm, NULL,
+ prim_list);
device->primaries = g_slist_concat(device->primaries, prim_list);
}
diff --git a/src/device.h b/src/device.h
index 285364f..2432884 100644
--- a/src/device.h
+++ b/src/device.h
@@ -56,7 +56,6 @@ void device_probe_drivers(struct btd_device *device, GSList *profiles);
const sdp_record_t *btd_device_get_record(struct btd_device *device,
const char *uuid);
GSList *btd_device_get_primaries(struct btd_device *device);
-void btd_device_add_service(struct btd_device *device, const char *path);
void device_register_services(DBusConnection *conn, struct btd_device *device,
GSList *prim_list, int psm);
void btd_device_add_uuid(struct btd_device *device, const char *uuid);
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 4/9] Move the primary service storage code to a local function
From: Claudio Takahasi @ 2011-04-11 18:25 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-4-git-send-email-claudio.takahasi@openbossa.org>
---
src/device.c | 27 ++++++++++++++++-----------
1 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/device.c b/src/device.c
index efe9938..86f605a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1562,14 +1562,26 @@ static char *primary_list_to_string(GSList *primary_list)
return g_string_free(services, FALSE);
}
+static void store_services(struct btd_device *device)
+{
+ struct btd_adapter *adapter = device->adapter;
+ bdaddr_t dba, sba;
+ char *str = primary_list_to_string(device->services);
+
+ adapter_get_address(adapter, &sba);
+ device_get_address(device, &dba);
+
+ write_device_type(&sba, &dba, device->type);
+ write_device_services(&sba, &dba, str);
+
+ g_free(str);
+}
+
static void primary_cb(GSList *services, guint8 status, gpointer user_data)
{
struct browse_req *req = user_data;
struct btd_device *device = req->device;
- struct btd_adapter *adapter = device->adapter;
GSList *l, *uuids = NULL;
- bdaddr_t dba, sba;
- char *str;
if (status) {
DBusMessage *reply;
@@ -1594,14 +1606,7 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
create_device_reply(device, req);
- str = primary_list_to_string(services);
-
- adapter_get_address(adapter, &sba);
- device_get_address(device, &dba);
-
- write_device_type(&sba, &dba, device->type);
- write_device_services(&sba, &dba, str);
- g_free(str);
+ store_services(device);
done:
device->browse = NULL;
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 5/9] Fix device type when creating from primary services storage
From: Claudio Takahasi @ 2011-04-11 18:25 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-5-git-send-email-claudio.takahasi@openbossa.org>
GATT services exported through basic rate need to be created based
on the "profiles" file. For GATT over LE, "primary" file entries need
to be used to create the devices and primary services objects.
---
src/adapter.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 6caff9a..8380a58 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2176,7 +2176,7 @@ static void create_stored_device_from_primary(char *key, char *value,
if (l)
device = l->data;
else {
- device = device_create(connection, adapter, key, DEVICE_TYPE_BREDR);
+ device = device_create(connection, adapter, key, DEVICE_TYPE_LE);
if (!device)
return;
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 6/9] Fix LE device creation from storage
From: Claudio Takahasi @ 2011-04-11 18:25 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-6-git-send-email-claudio.takahasi@openbossa.org>
Ignore the device if it already created. This patch adds a consistency
check to avoid registering the same service over basic rate and LE.
---
src/adapter.c | 20 +++++++++-----------
1 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 8380a58..9a0f688 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2171,18 +2171,16 @@ static void create_stored_device_from_primary(char *key, char *value,
struct btd_device *device;
GSList *services, *uuids, *l;
- l = g_slist_find_custom(adapter->devices,
- key, (GCompareFunc) device_address_cmp);
- if (l)
- device = l->data;
- else {
- device = device_create(connection, adapter, key, DEVICE_TYPE_LE);
- if (!device)
- return;
+ if (g_slist_find_custom(adapter->devices,
+ key, (GCompareFunc) device_address_cmp))
+ return;
- device_set_temporary(device, FALSE);
- adapter->devices = g_slist_append(adapter->devices, device);
- }
+ device = device_create(connection, adapter, key, DEVICE_TYPE_LE);
+ if (!device)
+ return;
+
+ device_set_temporary(device, FALSE);
+ adapter->devices = g_slist_append(adapter->devices, device);
services = string_to_primary_list(value);
if (services == NULL)
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 7/9] Fix primary services registration from storage for basic rate
From: Claudio Takahasi @ 2011-04-11 18:26 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-7-git-send-email-claudio.takahasi@openbossa.org>
---
src/adapter.c | 5 ++++-
src/device.c | 4 ++--
src/device.h | 2 ++
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 9a0f688..7049ba6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2004,7 +2004,7 @@ static void create_stored_device_from_profiles(char *key, char *value,
void *user_data)
{
struct btd_adapter *adapter = user_data;
- GSList *uuids = bt_string2list(value);
+ GSList *list, *uuids = bt_string2list(value);
struct btd_device *device;
if (g_slist_find_custom(adapter->devices,
@@ -2019,6 +2019,9 @@ static void create_stored_device_from_profiles(char *key, char *value,
adapter->devices = g_slist_append(adapter->devices, device);
device_probe_drivers(device, uuids);
+ list = device_services_from_record(device, uuids);
+ if (list)
+ device_register_services(connection, device, list, 31);
g_slist_foreach(uuids, (GFunc) g_free, NULL);
g_slist_free(uuids);
diff --git a/src/device.c b/src/device.c
index 86f605a..44bf76f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1367,7 +1367,7 @@ static void create_device_reply(struct btd_device *device, struct browse_req *re
g_dbus_send_message(req->conn, reply);
}
-static GSList *primary_from_record(struct btd_device *device, GSList *profiles)
+GSList *device_services_from_record(struct btd_device *device, GSList *profiles)
{
GSList *l, *prim_list = NULL;
char *att_uuid;
@@ -1440,7 +1440,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
device_probe_drivers(device, req->profiles_added);
- list = primary_from_record(device, req->profiles_added);
+ list = device_services_from_record(device, req->profiles_added);
if (list)
device_register_services(req->conn, device, list, 31);
}
diff --git a/src/device.h b/src/device.h
index 2432884..370382d 100644
--- a/src/device.h
+++ b/src/device.h
@@ -58,6 +58,8 @@ const sdp_record_t *btd_device_get_record(struct btd_device *device,
GSList *btd_device_get_primaries(struct btd_device *device);
void device_register_services(DBusConnection *conn, struct btd_device *device,
GSList *prim_list, int psm);
+GSList *device_services_from_record(struct btd_device *device,
+ GSList *profiles);
void btd_device_add_uuid(struct btd_device *device, const char *uuid);
struct btd_adapter *device_get_adapter(struct btd_device *device);
void device_get_address(struct btd_device *device, bdaddr_t *bdaddr);
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 8/9] TODO: Remove item related to GATT service over basic rate
From: Claudio Takahasi @ 2011-04-11 18:26 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-8-git-send-email-claudio.takahasi@openbossa.org>
---
TODO | 10 ----------
1 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/TODO b/TODO
index d52808b..461b179 100644
--- a/TODO
+++ b/TODO
@@ -114,16 +114,6 @@ Low Energy
ATT/GATT
========
-- For BR/EDR, primary services can be registered based on the information
- extracted from the service records. UUIDs, start and end handles information
- are available in the record, Discover All Primary Services procedure is not
- necessary. If a GATT service doesn't export a service record means that
- it should not be used over BR/EDR. Don't start this task before to move the
- attribute client code to the bluetoothd core.
-
- Priority: Medium
- Complexity: C1
-
- At the moment authentication and authorization is not supported at the
same time, read/write requirements in the attribute server needs to
be extended. According to Bluetooth Specification a server shall check
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 9/9] TODO: Add hard-coded PSM for GATT over basic rate
From: Claudio Takahasi @ 2011-04-11 18:26 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1302197414-833-9-git-send-email-claudio.takahasi@openbossa.org>
---
TODO | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/TODO b/TODO
index 461b179..fab3ec0 100644
--- a/TODO
+++ b/TODO
@@ -136,6 +136,11 @@ ATT/GATT
Priority: Medium
Complexity: C1
+- Fix hard-coded PSM for GATT services over basic rate.
+
+ Priority: Low
+ Complexity: C1
+
- Refactor read_by_group() and read_by_type() in src/attrib-server.c
(they've grown simply too big). First step could be to move out the
long for-loops to new functions called e.g. get_groups() and get_types().
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Fix refcount balance for hci connection
From: Gustavo F. Padovan @ 2011-04-11 18:34 UTC (permalink / raw)
To: Ville Tervo; +Cc: linux-bluetooth, johan.hedberg
In-Reply-To: <1302177590-4107-1-git-send-email-ville.tervo@nokia.com>
Hi Ville,
* Ville Tervo <ville.tervo@nokia.com> [2011-04-07 14:59:50 +0300]:
> hci_io_capa_reply_evt() holds reference for hciconnection. It's useless since
> hci_io_capa_request_evt()/hci_simple_pair_complete_evt() already protects the
> connection. In addition it leaves connection open after failed SSP pairing.
>
> Signed-off-by: Ville Tervo <ville.tervo@nokia.com>
> ---
> net/bluetooth/hci_event.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 7a3398d..6b878c8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2419,8 +2419,6 @@ static inline void hci_io_capa_reply_evt(struct hci_dev *hdev, struct sk_buff *s
> if (!conn)
> goto unlock;
>
> - hci_conn_hold(conn);
> -
Patch has been applied, thanks.
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: Endless print of uhci_result_common: failed with status 440000
From: Alan Stern @ 2011-04-11 18:38 UTC (permalink / raw)
To: Zdenek Kabelac
Cc: USB list, Linux Kernel Mailing List, Thomas Gleixner,
linux-bluetooth
In-Reply-To: <BANLkTikNb7FEDMbX3wSMti2e3muiaLZh-g@mail.gmail.com>
On Mon, 11 Apr 2011, Zdenek Kabelac wrote:
> > I've created:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=33062
> >
> > and I'm cc-ing linux-bluetooth.
> >
>
> In fact this Ooops might be related to:
>
> http://marc.info/?l=linux-kernel&m=130207593728273&w=2
It might be. Did you try reverting the patch mentioned in that email?
Alan Stern
^ permalink raw reply
* Re: [PATCH] Bluetooth: btmrvl: support Marvell Bluetooth device SD8787
From: Gustavo F. Padovan @ 2011-04-11 18:38 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Kevin Gan, Tristan Xu, Marcel Holtmann,
Stefan A. Popa
In-Reply-To: <1302311973-10715-1-git-send-email-bzhao@marvell.com>
Hi Bing,
* Bing Zhao <bzhao@marvell.com> [2011-04-08 18:19:33 -0700]:
> From: Kevin Gan <ganhy@marvell.com>
>
> The SD8787 firmware image is shared with mwifiex driver.
> Whoever gets loaded first will be responsible for firmware
> downloading.
>
> Signed-off-by: Kevin Gan <ganhy@marvell.com>
> Signed-off-by: Tristan Xu <xurf@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> drivers/bluetooth/Kconfig | 4 +-
> drivers/bluetooth/btmrvl_sdio.c | 124 +++++++++++++++++++++++++++++---------
> drivers/bluetooth/btmrvl_sdio.h | 68 +++++++++++----------
> 3 files changed, 132 insertions(+), 64 deletions(-)
Patch is now applied, thanks.
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
From: Marcel Holtmann @ 2011-04-11 21:40 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Vinicius Costa Gomes, Keith Packard, linux-kernel,
linux-bluetooth
In-Reply-To: <alpine.LFD.2.00.1104111807480.4501@localhost6.localdomain6>
Hi Thomas,
> > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > needs to be fixed proper.
> > >
> > > Who will submit this patch? I'd rather have your name on it so that
> > > people come complain at you...
> >
> > I took a shot at it and just sent a patch (also attached for convenience)
> > that should solve the problem.
>
> Aaarg. No. That patch reverts both hunks.
>
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> hci_req_cancel(hdev, ENODEV);
> hci_req_lock(hdev);
>
> - /* Stop timer, it might be running */
> - del_timer_sync(&hdev->cmd_timer);
> -
> if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> hci_req_unlock(hdev);
> return 0;
>
> As I said before you need that first hunk to stay for the case where
> there is no device up and you return via the !HCI_UP check. You just
> moved back to the state before as the stupid timer is active for
> whatever reason even when HCI_UP is not set.
if I read this right then we have the case that we arm this timer for no
real reason. A device in !HCI_UP should have nothing running. Certainly
not the cmd_timer since it will never process any commands.
According to Gustavo, the problem is really in the hci_reset logic were
we arm the timer even when shutting down the device.
Regards
Marcel
^ permalink raw reply
* [bluetooth-2.6 v2] Bluetooth: Fix keeping the command timer running
From: Vinicius Costa Gomes @ 2011-04-11 21:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Vinicius Costa Gomes
In the teardown path the reset command is sent to the controller,
this event causes the command timer to be reactivated.
So the timer is removed in two situations, when the adapter isn't
marked as UP and when we know that some command has been sent.
Reported-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
Changes:
- The case when the adapter isn't up should be taken into account
(thanks Thomas Gleixner);
- the timer is only removed if any command was ever sent;
Could someone confirm that the same crash could potentially happen during
hci_dev_open() if some command sent by __hci_request() fails?
net/bluetooth/hci_core.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2216620..e7dced9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -587,10 +587,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
hci_req_cancel(hdev, ENODEV);
hci_req_lock(hdev);
- /* Stop timer, it might be running */
- del_timer_sync(&hdev->cmd_timer);
-
if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
+ del_timer_sync(&hdev->cmd_timer);
hci_req_unlock(hdev);
return 0;
}
@@ -629,6 +627,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
/* Drop last sent command */
if (hdev->sent_cmd) {
+ del_timer_sync(&hdev->cmd_timer);
kfree_skb(hdev->sent_cmd);
hdev->sent_cmd = NULL;
}
--
1.7.4.3
^ permalink raw reply related
* Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
From: Thomas Gleixner @ 2011-04-11 22:16 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Vinicius Costa Gomes, Keith Packard, linux-kernel,
linux-bluetooth
In-Reply-To: <1302558033.2572.219.camel@aeonflux>
On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> Hi Thomas,
>
> > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > needs to be fixed proper.
> > > >
> > > > Who will submit this patch? I'd rather have your name on it so that
> > > > people come complain at you...
> > >
> > > I took a shot at it and just sent a patch (also attached for convenience)
> > > that should solve the problem.
> >
> > Aaarg. No. That patch reverts both hunks.
> >
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > hci_req_cancel(hdev, ENODEV);
> > hci_req_lock(hdev);
> >
> > - /* Stop timer, it might be running */
> > - del_timer_sync(&hdev->cmd_timer);
> > -
> > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > hci_req_unlock(hdev);
> > return 0;
> >
> > As I said before you need that first hunk to stay for the case where
> > there is no device up and you return via the !HCI_UP check. You just
> > moved back to the state before as the stupid timer is active for
> > whatever reason even when HCI_UP is not set.
>
> if I read this right then we have the case that we arm this timer for no
> real reason. A device in !HCI_UP should have nothing running. Certainly
> not the cmd_timer since it will never process any commands.
>
> According to Gustavo, the problem is really in the hci_reset logic were
> we arm the timer even when shutting down the device.
The reason why the original patch was sent is, that the timer was
running when the thing went out via the !HCI_UP path, which caused the
whole thing to explode in the first place. I had no time to figure out
why, but moving the del_timer_sync above the
if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
Thanks,
tglx
^ permalink raw reply
* Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
From: Thomas Gleixner @ 2011-04-11 22:19 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Vinicius Costa Gomes, Keith Packard, linux-kernel,
linux-bluetooth
In-Reply-To: <alpine.LFD.2.00.1104120014130.2702@localhost6.localdomain6>
On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> On Mon, 11 Apr 2011, Marcel Holtmann wrote:
>
> > Hi Thomas,
> >
> > > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > > needs to be fixed proper.
> > > > >
> > > > > Who will submit this patch? I'd rather have your name on it so that
> > > > > people come complain at you...
> > > >
> > > > I took a shot at it and just sent a patch (also attached for convenience)
> > > > that should solve the problem.
> > >
> > > Aaarg. No. That patch reverts both hunks.
> > >
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > > hci_req_cancel(hdev, ENODEV);
> > > hci_req_lock(hdev);
> > >
> > > - /* Stop timer, it might be running */
> > > - del_timer_sync(&hdev->cmd_timer);
> > > -
> > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > hci_req_unlock(hdev);
> > > return 0;
> > >
> > > As I said before you need that first hunk to stay for the case where
> > > there is no device up and you return via the !HCI_UP check. You just
> > > moved back to the state before as the stupid timer is active for
> > > whatever reason even when HCI_UP is not set.
> >
> > if I read this right then we have the case that we arm this timer for no
> > real reason. A device in !HCI_UP should have nothing running. Certainly
> > not the cmd_timer since it will never process any commands.
> >
> > According to Gustavo, the problem is really in the hci_reset logic were
> > we arm the timer even when shutting down the device.
>
> The reason why the original patch was sent is, that the timer was
> running when the thing went out via the !HCI_UP path, which caused the
> whole thing to explode in the first place. I had no time to figure out
> why, but moving the del_timer_sync above the
> if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
Oops. Hit send too fast.
Then it broke the resume on Keith machine and reverting just the hunk
which disarms the timer in the
if (hdev->sent_cmd) {
path made both scenarios working. So there are two problems:
1) Why do we need the del_timer_sync() above the !HCI_UP check
2) Why gets the timer rearmed after that
Thanks,
tglx
^ permalink raw reply
* Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
From: Gustavo F. Padovan @ 2011-04-11 22:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Marcel Holtmann, Vinicius Costa Gomes, Keith Packard,
linux-kernel, linux-bluetooth
In-Reply-To: <alpine.LFD.2.00.1104120016490.2702@localhost6.localdomain6>
* Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:
> On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> > On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> >
> > > Hi Thomas,
> > >
> > > > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > > > needs to be fixed proper.
> > > > > >
> > > > > > Who will submit this patch? I'd rather have your name on it so that
> > > > > > people come complain at you...
> > > > >
> > > > > I took a shot at it and just sent a patch (also attached for convenience)
> > > > > that should solve the problem.
> > > >
> > > > Aaarg. No. That patch reverts both hunks.
> > > >
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > > > hci_req_cancel(hdev, ENODEV);
> > > > hci_req_lock(hdev);
> > > >
> > > > - /* Stop timer, it might be running */
> > > > - del_timer_sync(&hdev->cmd_timer);
> > > > -
> > > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > > hci_req_unlock(hdev);
> > > > return 0;
> > > >
> > > > As I said before you need that first hunk to stay for the case where
> > > > there is no device up and you return via the !HCI_UP check. You just
> > > > moved back to the state before as the stupid timer is active for
> > > > whatever reason even when HCI_UP is not set.
> > >
> > > if I read this right then we have the case that we arm this timer for no
> > > real reason. A device in !HCI_UP should have nothing running. Certainly
> > > not the cmd_timer since it will never process any commands.
> > >
> > > According to Gustavo, the problem is really in the hci_reset logic were
> > > we arm the timer even when shutting down the device.
> >
> > The reason why the original patch was sent is, that the timer was
> > running when the thing went out via the !HCI_UP path, which caused the
> > whole thing to explode in the first place. I had no time to figure out
> > why, but moving the del_timer_sync above the
> > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
>
> Oops. Hit send too fast.
>
> Then it broke the resume on Keith machine and reverting just the hunk
> which disarms the timer in the
>
> if (hdev->sent_cmd) {
>
> path made both scenarios working. So there are two problems:
>
> 1) Why do we need the del_timer_sync() above the !HCI_UP check
That is still a mysterious to me, the real bug the hiding here. I'm trying to
track this down but no luck yet.
>
> 2) Why gets the timer rearmed after that
It is armed at each HCI command we send. In the close path we send out an HCI
RESET command that rearms it.
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* [RFC] Bluetooth: MTU for LE links
From: Anderson Briglia @ 2011-04-12 0:12 UTC (permalink / raw)
To: linux-bluetooth, ville.tervo, johan.hedberg, marcel
Hi all,
This patch is about MTU configuration for LE links. The current implementation
is wrong since the MTU value is not being negotiated.
Comments will be appreciated.
This patch is part of LE/SMP patches available here [1]
Regards,
Anderson Briglia
[1] git://gitorious.org/bluetooth-next/for-upstream.git
^ permalink raw reply
* [PATCH] Bluetooth: MTU configuration for LE links
From: Anderson Briglia @ 2011-04-12 0:12 UTC (permalink / raw)
To: linux-bluetooth, ville.tervo, johan.hedberg, marcel
Cc: Anderson Briglia, Vinicius Costa Gomes
This patch implements MTU configuration for LE links. The previous
implementation was setting the MTU as a fixed value.
Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
net/bluetooth/l2cap_core.c | 6 +++++-
net/bluetooth/l2cap_sock.c | 18 ++++++++++++++++--
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index be6c5aa..4a8b388 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -177,7 +177,11 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
if (sk->sk_type == SOCK_SEQPACKET || sk->sk_type == SOCK_STREAM) {
if (conn->hcon->type == LE_LINK) {
/* LE connection */
- l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
+ if (l2cap_pi(sk)->imtu < L2CAP_LE_DEFAULT_MTU)
+ l2cap_pi(sk)->imtu = L2CAP_LE_DEFAULT_MTU;
+ if (l2cap_pi(sk)->omtu < L2CAP_LE_DEFAULT_MTU)
+ l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
+
l2cap_pi(sk)->scid = L2CAP_CID_LE_DATA;
l2cap_pi(sk)->dcid = L2CAP_CID_LE_DATA;
} else {
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0059dda..b5796fd 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -530,16 +530,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
{
struct sock *sk = sock->sk;
struct l2cap_options opts;
- int len, err = 0;
+ int len, le_sock, err = 0;
u32 opt;
BT_DBG("sk %p", sk);
lock_sock(sk);
+ le_sock = l2cap_pi(sk)->scid == L2CAP_CID_LE_DATA;
+
switch (optname) {
case L2CAP_OPTIONS:
- if (sk->sk_state == BT_CONNECTED) {
+ if (sk->sk_state == BT_CONNECTED && !le_sock) {
err = -EINVAL;
break;
}
@@ -558,6 +560,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
break;
}
+ if ((opts.imtu || opts.omtu) && le_sock &&
+ (sk->sk_state == BT_CONNECTED)) {
+ if (opts.imtu >= L2CAP_LE_DEFAULT_MTU)
+ l2cap_pi(sk)->imtu = opts.imtu;
+ if (opts.omtu >= L2CAP_LE_DEFAULT_MTU)
+ l2cap_pi(sk)->omtu = opts.omtu;
+ if (opts.imtu < L2CAP_LE_DEFAULT_MTU ||
+ opts.omtu < L2CAP_LE_DEFAULT_MTU)
+ err = -EINVAL;
+ break;
+ }
+
if (opts.txwin_size > L2CAP_DEFAULT_TX_WINDOW) {
err = -EINVAL;
break;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] Bluetooth: MTU configuration for LE links
From: Marcel Holtmann @ 2011-04-12 4:06 UTC (permalink / raw)
To: Anderson Briglia
Cc: linux-bluetooth, ville.tervo, johan.hedberg, Vinicius Costa Gomes
In-Reply-To: <1302567158-18617-2-git-send-email-anderson.briglia@openbossa.org>
Hi Anderson,
> This patch implements MTU configuration for LE links. The previous
> implementation was setting the MTU as a fixed value.
>
> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
> net/bluetooth/l2cap_core.c | 6 +++++-
> net/bluetooth/l2cap_sock.c | 18 ++++++++++++++++--
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index be6c5aa..4a8b388 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -177,7 +177,11 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> if (sk->sk_type == SOCK_SEQPACKET || sk->sk_type == SOCK_STREAM) {
> if (conn->hcon->type == LE_LINK) {
> /* LE connection */
> - l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
> + if (l2cap_pi(sk)->imtu < L2CAP_LE_DEFAULT_MTU)
> + l2cap_pi(sk)->imtu = L2CAP_LE_DEFAULT_MTU;
> + if (l2cap_pi(sk)->omtu < L2CAP_LE_DEFAULT_MTU)
> + l2cap_pi(sk)->omtu = L2CAP_LE_DEFAULT_MTU;
> +
> l2cap_pi(sk)->scid = L2CAP_CID_LE_DATA;
> l2cap_pi(sk)->dcid = L2CAP_CID_LE_DATA;
> } else {
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 0059dda..b5796fd 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -530,16 +530,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
> {
> struct sock *sk = sock->sk;
> struct l2cap_options opts;
> - int len, err = 0;
> + int len, le_sock, err = 0;
> u32 opt;
>
> BT_DBG("sk %p", sk);
>
> lock_sock(sk);
>
> + le_sock = l2cap_pi(sk)->scid == L2CAP_CID_LE_DATA;
> +
> switch (optname) {
> case L2CAP_OPTIONS:
> - if (sk->sk_state == BT_CONNECTED) {
> + if (sk->sk_state == BT_CONNECTED && !le_sock) {
> err = -EINVAL;
> break;
> }
> @@ -558,6 +560,18 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
> break;
> }
>
> + if ((opts.imtu || opts.omtu) && le_sock &&
> + (sk->sk_state == BT_CONNECTED)) {
> + if (opts.imtu >= L2CAP_LE_DEFAULT_MTU)
> + l2cap_pi(sk)->imtu = opts.imtu;
> + if (opts.omtu >= L2CAP_LE_DEFAULT_MTU)
> + l2cap_pi(sk)->omtu = opts.omtu;
> + if (opts.imtu < L2CAP_LE_DEFAULT_MTU ||
> + opts.omtu < L2CAP_LE_DEFAULT_MTU)
> + err = -EINVAL;
> + break;
> + }
> +
so how do you expect this to work exactly? In BR/EDR L2CAP you set the
MTU details before calling connect(). With LE you expect to be connected
and then tell the kernel what the limits actually are?
This sounds highly convoluted. And especially hijacking the existing
command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
given it away that it is a bad idea to re-use that socket option for a
new technology ;)
So the real fact here is that the MTU handling/negotiation for BR/EDR
and LE are different. Nothing is going to change this. So this should be
also different from a socket option point of view.
Regards
Marcel
^ permalink raw reply
* Re: Endless print of uhci_result_common: failed with status 440000
From: Zdenek Kabelac @ 2011-04-12 9:33 UTC (permalink / raw)
To: Alan Stern
Cc: USB list, Linux Kernel Mailing List, Thomas Gleixner,
linux-bluetooth
In-Reply-To: <Pine.LNX.4.44L0.1104111437440.1975-100000@iolanthe.rowland.org>
2011/4/11 Alan Stern <stern@rowland.harvard.edu>:
> On Mon, 11 Apr 2011, Zdenek Kabelac wrote:
>
>> > I've created:
>> >
>> > https://bugzilla.kernel.org/show_bug.cgi?id=33062
>> >
>> > and I'm cc-ing linux-bluetooth.
>> >
>>
>> In fact this Ooops might be related to:
>>
>> http://marc.info/?l=linux-kernel&m=130207593728273&w=2
>
> It might be. Did you try reverting the patch mentioned in that email?
>
Yep - it looks like the patch
http://marc.info/?l=linux-bluetooth&m=130230770920378&w=2
solved my problem with weird deadlocks.
Zdenek
^ permalink raw reply
* Re: [PATCH] Bluetooth: MTU configuration for LE links
From: Anderson Lizardo @ 2011-04-12 10:56 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Anderson Briglia, linux-bluetooth, ville.tervo, johan.hedberg,
Vinicius Costa Gomes
In-Reply-To: <1302581196.2572.249.camel@aeonflux>
Hi Marcel,
On Tue, Apr 12, 2011 at 12:06 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> so how do you expect this to work exactly? In BR/EDR L2CAP you set the
> MTU details before calling connect(). With LE you expect to be connected
> and then tell the kernel what the limits actually are?
Yes. For LE, the MTU negotiation happens over the connected link
(using specific ATT commands).
> This sounds highly convoluted. And especially hijacking the existing
> command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
> given it away that it is a bad idea to re-use that socket option for a
> new technology ;)
>
> So the real fact here is that the MTU handling/negotiation for BR/EDR
> and LE are different. Nothing is going to change this. So this should be
> also different from a socket option point of view.
One thing to consider is that there are a couple of MTU checks along
the L2CAP code. The issue which originated this patch is code such as:
/* Check outgoing MTU */
if (len > pi->omtu) {
err = -EMSGSIZE;
goto done;
}
We understood that just letting omtu/imtu values on kernel reflect the
current connection MTU settings was the cleanest solution. What do you
propose? Bypassing these checks for LE?
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH] Bluetooth: MTU configuration for LE links
From: Marcel Holtmann @ 2011-04-12 10:59 UTC (permalink / raw)
To: Anderson Lizardo
Cc: Anderson Briglia, linux-bluetooth, ville.tervo, johan.hedberg,
Vinicius Costa Gomes
In-Reply-To: <BANLkTinoW_bjCw=DwS2pMg-eM06Yq3MXLg@mail.gmail.com>
Hi Anderson,
> > so how do you expect this to work exactly? In BR/EDR L2CAP you set the
> > MTU details before calling connect(). With LE you expect to be connected
> > and then tell the kernel what the limits actually are?
>
> Yes. For LE, the MTU negotiation happens over the connected link
> (using specific ATT commands).
>
> > This sounds highly convoluted. And especially hijacking the existing
> > command for it seems wrong. Using l2cap_sock_setsockopt_old() might have
> > given it away that it is a bad idea to re-use that socket option for a
> > new technology ;)
> >
> > So the real fact here is that the MTU handling/negotiation for BR/EDR
> > and LE are different. Nothing is going to change this. So this should be
> > also different from a socket option point of view.
>
> One thing to consider is that there are a couple of MTU checks along
> the L2CAP code. The issue which originated this patch is code such as:
>
> /* Check outgoing MTU */
> if (len > pi->omtu) {
> err = -EMSGSIZE;
> goto done;
> }
>
> We understood that just letting omtu/imtu values on kernel reflect the
> current connection MTU settings was the cleanest solution. What do you
> propose? Bypassing these checks for LE?
this does not look clean to me. And we might have an internal MTU
variable as part of L2CAP, but the way how it gets set and thus its
semantic differs clearly between BR/EDR and LE. So shoehorning an
existing socket option this is clearly a bad idea.
Regards
Marcel
^ permalink raw reply
* Wrong timeout when connecting to device with removed pairing
From: Luiz Augusto von Dentz @ 2011-04-12 11:36 UTC (permalink / raw)
To: linux-bluetooth
Apparently this is a regression caused by commit
769be974d0c7b4fe1a52f9cdaad22259b60953f7 - [Bluetooth] Use ACL config
stage to retrieve remote features, when receiving pincode request in a
new connection the state will be BT_CONFIG so HCI_PAIRING_TIMEOUT is
never set to the connection, this could be reproduced by removing a
link key in the remote side and trying to connect:
2011-04-05 12:11:37.070953 < HCI Command: Authentication Requested
(0x01|0x0011) plen 2
handle 1
2011-04-05 12:11:37.071686 > HCI Event: Command Status (0x0f) plen 4
Authentication Requested (0x01|0x0011) status 0x00 ncmd 1
2011-04-05 12:11:37.071838 > HCI Event: Link Key Request (0x17) plen 6
bdaddr 00:1F:E1:EB:B9:CB
2011-04-05 12:11:37.075073 < HCI Command: Link Key Request Reply
(0x01|0x000b) plen 22
bdaddr 00:1F:E1:EB:B9:CB key 92BA9EAD06542B0600457C609B8F5395
2011-04-05 12:11:37.075775 > HCI Event: Command Complete (0x0e) plen 10
Link Key Request Reply (0x01|0x000b) ncmd 1
status 0x00 bdaddr 00:1F:E1:EB:B9:CB
2011-04-05 12:11:37.087005 > HCI Event: PIN Code Request (0x16) plen 6
bdaddr 00:1F:E1:EB:B9:CB
2011-04-05 12:11:39.038849 < ACL data: handle 1 flags 0x00 dlen 12
L2CAP(s): Disconn req: dcid 0x00a9 scid 0x0040
2011-04-05 12:11:39.100647 > HCI Event: Number of Completed Packets
(0x13) plen 5
handle 1 packets 1
2011-04-05 12:11:39.100830 > ACL data: handle 1 flags 0x02 dlen 12
L2CAP(s): Disconn rsp: dcid 0x00a9 scid 0x0040
2011-04-05 12:11:40.433624 > HCI Event: Auth Complete (0x06) plen 3
status 0x13 handle 1
Error: Remote User Terminated Connection
2011-04-05 12:11:40.437774 > HCI Event: Disconn Complete (0x05) plen 4
status 0x00 handle 1 reason 0x13
Reason: Remote User Terminated Connection
I think the following patch might fix it:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3fbfa50..abdc4de 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1973,10 +1973,14 @@ static inline void hci_pin_code_request_evt(struct
hci_dev *hdev, struct sk_buff
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
- if (conn && conn->state == BT_CONNECTED) {
- hci_conn_hold(conn);
+ if (conn) {
conn->disc_timeout = HCI_PAIRING_TIMEOUT;
- hci_conn_put(conn);
+
+ /* Update disconnect timer */
+ if (conn->state == BT_CONNECTED) {
+ hci_conn_hold(conn);
+ hci_conn_put(conn);
+ }
}
Comments?
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply related
* Re: [PATCH] Bluetooth: MTU configuration for LE links
From: Anderson Lizardo @ 2011-04-12 11:46 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Anderson Briglia, linux-bluetooth, ville.tervo, johan.hedberg,
Vinicius Costa Gomes
In-Reply-To: <1302605974.2572.258.camel@aeonflux>
Hi Marcel,
On Tue, Apr 12, 2011 at 6:59 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>> One thing to consider is that there are a couple of MTU checks along
>> the L2CAP code. The issue which originated this patch is code such as:
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check outgoing MTU */
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (len > pi->omtu) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EMSGSIZE;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>
>> We understood that just letting omtu/imtu values on kernel reflect the
>> current connection MTU settings was the cleanest solution. What do you
>> propose? Bypassing these checks for LE?
>
> this does not look clean to me. And we might have an internal MTU
> variable as part of L2CAP, but the way how it gets set and thus its
> semantic differs clearly between BR/EDR and LE. So shoehorning an
> existing socket option this is clearly a bad idea.
Sure. What to do then if:
1) LE links have MTU (omtu specifically) hard-coded to 23 on kernel.
2) the kernel rejects sending data longer than omtu.
(this is the current situation)
This clearly needs some fix on kernel side, otherwise we can't send
PDUs longer than the LE default MTU (23), *even* if the peer device
supports a bigger MTU.
Suggestions are welcome regarding how to best approach this, without
affecting current BR/EDR MTU semantics.
Regards,
--=20
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: Endless print of uhci_result_common: failed with status 440000
From: Alan Stern @ 2011-04-12 16:10 UTC (permalink / raw)
To: Zdenek Kabelac
Cc: USB list, Linux Kernel Mailing List, Thomas Gleixner,
linux-bluetooth
In-Reply-To: <BANLkTin0bKs1D3JxVODw3F+qH0ZqRwh5KA@mail.gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 647 bytes --]
On Tue, 12 Apr 2011, Zdenek Kabelac wrote:
> 2011/4/11 Alan Stern <stern@rowland.harvard.edu>:
> > On Mon, 11 Apr 2011, Zdenek Kabelac wrote:
> >
> >> > I've created:
> >> >
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=33062
> >> >
> >> > and I'm cc-ing linux-bluetooth.
> >> >
> >>
> >> In fact this Ooops might be related to:
> >>
> >> http://marc.info/?l=linux-kernel&m=130207593728273&w=2
> >
> > It might be. Did you try reverting the patch mentioned in that email?
> >
>
> Yep - it looks like the patch
> http://marc.info/?l=linux-bluetooth&m=130230770920378&w=2
> solved my problem with weird deadlocks.
Okay, great!
Alan Stern
^ permalink raw reply
* Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
From: Gustavo F. Padovan @ 2011-04-12 18:09 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Marcel Holtmann, Vinicius Costa Gomes, Keith Packard,
linux-kernel, linux-bluetooth
In-Reply-To: <20110411222504.GE2195@joana>
* Gustavo F. Padovan <padovan@profusion.mobi> [2011-04-11 19:25:04 -0300]:
> * Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:
>=20
> > On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> > > On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> > >=20
> > > > Hi Thomas,
> > > >=20
> > > > > > > > Can the bluetooth folks please have a look at that ASAP? Th=
e obvious
> > > > > > > > fast fix for Linus tree is to revert the second hunk for no=
w, but this
> > > > > > > > needs to be fixed proper.
> > > > > > >=20
> > > > > > > Who will submit this patch? I'd rather have your name on it s=
o that
> > > > > > > people come complain at you...
> > > > > >=20
> > > > > > I took a shot at it and just sent a patch (also attached for co=
nvenience)=20
> > > > > > that should solve the problem.
> > > > >=20
> > > > > Aaarg. No. That patch reverts both hunks.
> > > > >=20
> > > > > --- a/net/bluetooth/hci_core.c
> > > > > +++ b/net/bluetooth/hci_core.c
> > > > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *h=
dev)
> > > > > hci_req_cancel(hdev, ENODEV);
> > > > > hci_req_lock(hdev);
> > > > > =20
> > > > > - /* Stop timer, it might be running */
> > > > > - del_timer_sync(&hdev->cmd_timer);
> > > > > -
> > > > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > > > hci_req_unlock(hdev);
> > > > > return 0;
> > > > >=20
> > > > > As I said before you need that first hunk to stay for the case wh=
ere
> > > > > there is no device up and you return via the !HCI_UP check. You j=
ust
> > > > > moved back to the state before as the stupid timer is active for
> > > > > whatever reason even when HCI_UP is not set.
> > > >=20
> > > > if I read this right then we have the case that we arm this timer f=
or no
> > > > real reason. A device in !HCI_UP should have nothing running. Certa=
inly
> > > > not the cmd_timer since it will never process any commands.
> > > >=20
> > > > According to Gustavo, the problem is really in the hci_reset logic =
were
> > > > we arm the timer even when shutting down the device.
> > >=20
> > > The reason why the original patch was sent is, that the timer was
> > > running when the thing went out via the !HCI_UP path, which caused the
> > > whole thing to explode in the first place. I had no time to figure out
> > > why, but moving the del_timer_sync above the
> > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
> >=20
> > Oops. Hit send too fast.
> >=20
> > Then it broke the resume on Keith machine and reverting just the hunk
> > which disarms the timer in the=20
> >=20
> > if (hdev->sent_cmd) {
> >=20
> > path made both scenarios working. So there are two problems:
> >=20
> > 1) Why do we need the del_timer_sync() above the !HCI_UP check
>=20
> That is still a mysterious to me, the real bug the hiding here. I'm tryin=
g to
> track this down but no luck yet.
>=20
> >=20
> > 2) Why gets the timer rearmed after that
>=20
> It is armed at each HCI command we send. In the close path we send out an=
HCI
> RESET command that rearms it.
I applied v2 patch from Vin=EDcius that fix all the symptoms. Now we have m=
ore time
to find the real cause of this bug. However I still have no idea, I'm not a=
ble
to reproduce it.
--=20
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox