* [PATCH 1/6] Implement Find by Type request encode/decoding
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
From: Bruna Moreira <bruna.moreira@openbossa.org>
---
attrib/att.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
attrib/att.h | 4 ++-
2 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/attrib/att.c b/attrib/att.c
index fe41d0e..6c889f2 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -198,9 +198,77 @@ struct att_data_list *dec_read_by_grp_resp(const uint8_t *pdu, int len)
}
uint16_t enc_find_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
- uint8_t *pdu, int len)
+ const uint8_t *value, int vlen, uint8_t *pdu, int len)
+{
+ uint16_t min_len = sizeof(pdu[0]) + sizeof(start) + sizeof(end) +
+ sizeof(uint16_t);
+
+ if (pdu == NULL)
+ return 0;
+
+ if (!uuid)
+ return 0;
+
+ if (uuid->type != SDP_UUID16)
+ return 0;
+
+ if (len < min_len)
+ return 0;
+
+ if (vlen > len - min_len)
+ vlen = len - min_len;
+
+ pdu[0] = ATT_OP_FIND_BY_TYPE_REQ;
+ att_put_u16(start, &pdu[1]);
+ att_put_u16(end, &pdu[3]);
+ att_put_u16(uuid->value.uuid16, &pdu[5]);
+
+ if (vlen > 0) {
+ memcpy(&pdu[7], value, vlen);
+ return min_len + vlen;
+ }
+
+ return min_len;
+}
+
+uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
+ uint16_t *end, uuid_t *uuid, uint8_t *value, int *vlen)
{
- return 0;
+ int valuelen;
+ uint16_t min_len = sizeof(pdu[0]) + sizeof(*start) +
+ sizeof(*end) + sizeof(uint16_t);
+
+ if (pdu == NULL)
+ return 0;
+
+ if (len < min_len)
+ return 0;
+
+ if (pdu[0] != ATT_OP_FIND_BY_TYPE_REQ)
+ return 0;
+
+ /* First requested handle number */
+ if (start)
+ *start = att_get_u16(&pdu[1]);
+
+ /* Last requested handle number */
+ if (end)
+ *end = att_get_u16(&pdu[3]);
+
+ /* Always UUID16 */
+ if (uuid)
+ sdp_uuid16_create(uuid, att_get_u16(&pdu[5]));
+
+ valuelen = len - min_len;
+
+ /* Attribute value to find */
+ if (valuelen > 0 && value)
+ memcpy(value, pdu + min_len, valuelen);
+
+ if (vlen)
+ *vlen = valuelen;
+
+ return len;
}
uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
diff --git a/attrib/att.h b/attrib/att.h
index ea49dc2..9de338d 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -165,7 +165,9 @@ uint16_t dec_read_by_grp_req(const uint8_t *pdu, int len, uint16_t *start,
uint16_t *end, uuid_t *uuid);
uint16_t enc_read_by_grp_resp(struct att_data_list *list, uint8_t *pdu, int len);
uint16_t enc_find_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
- uint8_t *pdu, int len);
+ const uint8_t *value, int vlen, uint8_t *pdu, int len);
+uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
+ uint16_t *end, uuid_t *uuid, uint8_t *value, int *vlen);
struct att_data_list *dec_read_by_grp_resp(const uint8_t *pdu, int len);
uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
uint8_t *pdu, int len);
--
1.7.3.2
^ permalink raw reply related
* Re: [PATCH 3/4] Add DBus OOB API documentation.
From: jaikumar Ganesh @ 2010-11-17 17:15 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <201011161112.05202.szymon.janc@tieto.com>
Hi Szymon:
On Tue, Nov 16, 2010 at 2:12 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>> >> Why are we enforcing this limitation ?
>> >
>> > Spec requires that each hash&randomizer values should be used only for one OOB
>> > transfer. Implementation enforces that by allowing only one active OOB plugin
>> > at time and DBUS OOB plugin only 'expose' plugin API over DBUS. So this
>> > limitation is a consequence of that.
>>
>> I don't think the spec says that. It just says every single time the
>> call to read the local adapters hash
>> and randomizer is done you get new values. You can use the value that
>> you have obtained for as many OOB pairings
>> as you wish.
>
> Please see note in Vol2. Part E. 7.3.60:
>
> "Each OOB transfer will have unique C and R values so after each OOB
> transfer this command shall be used to obtain a new set of values for the next
> OOB transfer."
>
Yes we were talking about the same thing, just at different layers. I
was talking about it from the user of the provider.
This looks fine to me.
>
> --
> Szymon Janc
>
^ permalink raw reply
* RE: [PATCH] Adding a new option to specify security level for gatttool
From: Mike Tsai @ 2010-11-17 16:57 UTC (permalink / raw)
To: tim.howes@accenture.com; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1AFE20D16950C745A2A83986B72E8748011F571E7497@EMEXM3131.dir.svc.accenture.com>
Hi Tim,
-----Original Message-----
From: tim.howes@accenture.com [mailto:tim.howes@accenture.com]
Sent: Wednesday, November 17, 2010 7:04 AM
To: Mike Tsai
Cc: linux-bluetooth@vger.kernel.org
Subject: RE: [PATCH] Adding a new option to specify security level for gatttool
Hi Mike,
-----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Johan Hedberg
> Sent: Tuesday, November 16, 2010 7:37 AM
> To: Sheldon Demario
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Adding a new option to specify security level for
> gatttool
>
> Hi Sheldon,
>
> On Tue, Nov 16, 2010, Sheldon Demario wrote:
> > ---
> > TODO | 6 ------
> > attrib/gatttool.c | 15 +++++++++++++--
> > 2 files changed, 13 insertions(+), 8 deletions(-)
>
> In general the patch seems fine, but:
>
> > + if (strncmp(opt_sec_level, "medium", 6) == 0)
> > + sec_level = BT_IO_SEC_MEDIUM;
> > + else if (strncmp(opt_sec_level, "high", 4) == 0)
> > + sec_level = BT_IO_SEC_HIGH;
> > + else
> > + sec_level = BT_IO_SEC_LOW;
>
> Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
> think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM
> ;)
>
> [Mtsai] I am not sure what are the definition of "low", "medium" or
> "high". By the spec of Core 4.0, LE has 2 security modes and different
> security levels based on the method of pairing (or bonding). It may be
> appeal to end user with "low", "medium" and "high" definition, but it
> can't be reference with LE spec. I would suggest, instead, following
> terms,
>
> "No security",
> "unauthenticated encryption",
> "authenticated encryption",
> "unauthenticated data signing",
> "authenticated data signing,
>
> Mike
To some extent I agree; however, the semantics of such an API would have to be careful. A particular profile should not "force" data signing because if the link is already encrypted there is little point using data signing. So from that point of view exposing a more abstract API (a bit like "high") is better. However, it is hard to map "high" onto any of the ones you listed (which I agree is a good list). So perhaps it is better to have the API semantics as "advisory" or "requests" which can be fulfilled by the underlying stack in other ways (eg encryption for data-signing).
Cheers
Tim
[Mtsai] I fully embrace the "abstract" API concept. However, all new LE profiles have clearly defined the security request per each attribute now. As a generic attribute profile (as GATT), without clear indication from Application what level of security is requested, GATT/GAP can handle the security request incorrectly and cause IOP issues. For example, what does "low" mean when GAP/SM gets this request from Application? It does not tell GAP/SM what kind of pairing shall be performed and if encryption or data signing for accessing the attributes.
Cheers,
Mike
This message is for the designated recipient only and may contain privileged, proprietary, or otherwise private information. If you have received it in error, please notify the sender immediately and delete the original. Any other use of the email by you is prohibited.
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Gustavo F. Padovan @ 2010-11-17 16:52 UTC (permalink / raw)
To: Pavan Savoy; +Cc: Vitaly Wool, marcel, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTi=NH8gbW8iCKOQN0bmbL=nqewsZkUik+VkvWUgi@mail.gmail.com>
Hi Pavan,
* Pavan Savoy <pavan_savoy@sify.com> [2010-11-17 11:13:26 +0530]:
> On Wed, Nov 17, 2010 at 4:50 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> >>> + =A0 =A0 /* Registration with ST layer is successful,
> >>> + =A0 =A0 =A0* hardware is ready to accept commands from HCI core.
> >>> + =A0 =A0 =A0*/
> >>> + =A0 =A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 clear_bit(HCI_RUNNING, &hdev->flags);
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D st_unregister(ST_BT);
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (err)
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister() fai=
led with error %d", err);
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D NULL;
> >>> + =A0 =A0 }
> >>
> >>
> >> What are you trying to do here? test_and_set_bit() result doesn't say
> >> nothing about error and you shall put test_and_set_bit should be in the
> >> beginning, to know if your device is already opened or not and then
> >> clear_bit if some error ocurrs during the function.
> >>
> >
> > Yeap, this piece of code beats me is well. Why is it an error if this
> > bit wasn't already set?
>=20
> Vitaly, Gustavo,
>=20
> I suppose I never understood HCI_RUNNING flag that way, as in an error
> check mechanism to avoid multiple hci0 ups.
>=20
> What I understood was that HCI_RUNNING suggested as to when hci0 was
> ready to be used. With this understanding, I wanted to make sure I
> downloaded the firmware for the chip before I proclaim to the world
> that the hci0 is ready to be used, as in HCI_RUNNING.
>=20
> For example, I didn't want my _send_frame to be called before I did
> the firmware download - since firmware download takes time - 45kb
> send/wait commands :(
>=20
> But I suppose I now understand - What I would rather do is test_bit in
> the beginning of function and do a set_bit at the end of function -
> does this make sense ?
It does, but does it as test_and_set and then clear if error as we do in
other drivers.
--=20
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Gustavo F. Padovan @ 2010-11-17 16:50 UTC (permalink / raw)
To: Pavan Savoy; +Cc: marcel, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTikbhaZOvM43Vv6wdcbmK8nh2Nq2Hc1i_naxWjYM@mail.gmail.com>
Hi Pavan,
* Pavan Savoy <pavan_savoy@sify.com> [2010-11-17 11:04:59 +0530]:
> Gustavo,
>=20
> On Wed, Nov 17, 2010 at 4:24 AM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Pavan,
> >
> > * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-10 08:07:26 -0500]:
> >
> >> From: Pavan Savoy <pavan_savoy@ti.com>
> >>
> >> Marcel,
> >>
> >> Thanks for the comments...
> >> This patch contains,
> >> v5 comments :-
> >> declaration and assiging of variables and private data fixed up.
> >> proper casting.
> >> removed redundant un-necessary checks in send_frame.
> >> HCI_RUNNING fixes in terms of test_and_set/clear bit instead of set and
> >> clear.
> >> removed redundant checks for hdev, skb being NULL.
> >> removed module_param of reset, also WiLink don't need HCI_RESET anyway=
s.
> >> removed ti_st_register_dev function and functionality moved to _probe.
> >> module_init/exit function names fixed up.
> >>
> >> stat byte counter increments and tx_complete is similar to hci_ldisc.
> >> Also I have not implemented the flush routine, since the functionality
> >> which needs to be done in flush routine is done in the underlying driv=
er
> >> which is the shared transport driver and moreover the btwilink driver =
by
> >> itself doesn't maintains queue or data relevant to transport, so nothi=
ng
> >> to do.
> >>
> >> And Yes, I have verified this driver with multiple up/down reset on
> >> hci0.
> >> Also I generally test a2dp/ftp to verify large data transfers.
> >
> > Before re-submit a patch you have to fix all issues reported by the
> > reviewer or reply to patch thread why do you think you right and don't
> > want to change that. That is not happening with your patches, you are
> > not fixing all the stuff before re-submiting and it is not the first
> > time. =A0If you do it right we can review it fast and your code goes
> > earlier to mainline.
> > You should also answer the questions first =A0like "Where is the
> > ti_st_proto.write coming from?" You just ignored the
> > review and submitted a new patch.
>=20
> This is the reason, I tend to keep the patch comments in the mail.
> I have mentioned here the
> 1. comments I have taken care of,
> 2. few comments which I don't understand why it is like that and which
> are not taken care of.
You should reply inline, and not do top posting like this.
>=20
> Also the question on ti_st_proto.write, I have answered it twice in
> mail, once to you and once to marcel, for more clarity I have even
> added it in the code comments, Please have a look @ line
> >> + /* ti_st_proto.write is filled up by the underlying shared
> >> + * transport driver upon registration
> >> + */
> As to why this function is not EXPORT_SYMBOL and just an function
> pointer, Yes I had it as function pointer - But since something like
> _read is callback from the protocol driver perspective - to maintain
> uniformity I have this as function pointer.
> (Note: comments to other drivers which are based on ST driver intended
> read/write to be pointers which register/unregister to be EXPORTs).
>=20
> Ok, apart from this there was an open question as why I am checking
> for only 2 error conditions, it is because the underlying driver only
> can send those 2 errors and nothing else (st_register has few errors
> it can throw...)
I don't care if it can send only two errors, someone can change the
underlying driver and we at the Bluetooth subsystem may never know that.
So check for all errors there, instead of two.
--=20
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: [PATCHv3 5/7] Add approval request for incoming pairing requests with OOB mechanism
From: Szymon Janc @ 2010-11-17 16:03 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org
In-Reply-To: <20101116162412.GB3638@jh-x301>
Hi Johan,
> > + req->msg = dbus_message_new_method_call(agent->name, agent->path,
> > + "org.bluez.Agent", "RequestApproval");
>
> I think the name of this method is one of the more important things to
> get right before we push this upstream. I'm not so sure of the current
> proposal. I have a feeling that it'd be a good idea to have the term
> "pair" somewhere explicitly in the name. How about "RequestPairing"? I
> know Marcel will have something to say about this too.
Maybe make it more descriptive like RequestPairingApproval ?
Yet, RequestPairing sounds ok.
--
Szymon Janc
on behalf of ST-Ericsson
^ permalink raw reply
* [PATCH 3/3] Emit "DeviceFound" signal for LE devices
From: Bruna Moreira @ 2010-11-17 15:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1290009593-13658-1-git-send-email-bruna.moreira@openbossa.org>
The adapter_emit_device_found() function was modified to emit
DeviceFound signal for LE devices as well.
---
src/adapter.c | 30 +++++++++++++++++++++++++-----
1 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index a7e78bc..e376f59 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2974,6 +2974,27 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
if (device)
paired = device_is_paired(device);
+ /* Extract UUIDs from extended inquiry response if any */
+ dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+ uuid_count = g_slist_length(dev->services);
+
+ if (dev->services)
+ uuids = strlist2array(dev->services);
+
+ if (dev->le) {
+ emit_device_found(adapter->path, paddr,
+ "Address", DBUS_TYPE_STRING, &paddr,
+ "RSSI", DBUS_TYPE_INT16, &rssi,
+ "Name", DBUS_TYPE_STRING, &dev->name,
+ "Paired", DBUS_TYPE_BOOLEAN, &paired,
+ "UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
+ NULL);
+
+ g_strfreev(uuids);
+
+ return;
+ }
+
icon = class_to_icon(dev->class);
if (!dev->alias) {
@@ -2985,12 +3006,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
} else
alias = g_strdup(dev->alias);
- /* Extract UUIDs from extended inquiry response if any */
- dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
- uuid_count = g_slist_length(dev->services);
-
if (dev->services) {
- uuids = strlist2array(dev->services);
g_slist_foreach(dev->services, (GFunc) g_free, NULL);
g_slist_free(dev->services);
dev->services = NULL;
@@ -3071,6 +3087,10 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
dev->name = tmp_name;
}
}
+
+ /* FIXME: check if other information was changed before emitting the
+ * signal */
+ adapter_emit_device_found(adapter, dev, info->data, info->length);
}
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/3] Extract service UUIDs from advertising data
From: Bruna Moreira @ 2010-11-17 15:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1290009593-13658-1-git-send-email-bruna.moreira@openbossa.org>
Make get_eir_uuids() return a GSList of strings, so it can be reused to
extract UUIDs from LE advertising data. The bt_strlist2array() helper
function was created to convert a GSList into a plain array of strings
(needed to send through D-Bus).
---
src/adapter.c | 71 ++++++++++++++++++++++++++++++++++++++++++++-------------
src/adapter.h | 1 +
2 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 0089d6d..a7e78bc 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -202,6 +202,8 @@ static void dev_info_free(struct remote_dev_info *dev)
{
g_free(dev->name);
g_free(dev->alias);
+ g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+ g_slist_free(dev->services);
g_free(dev);
}
@@ -2826,11 +2828,27 @@ static void emit_device_found(const char *path, const char *address,
g_dbus_send_message(connection, signal);
}
-static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
- size_t *uuid_count)
+static char **strlist2array(GSList *list)
+{
+ GSList *l;
+ unsigned int i, n;
+ char **array;
+
+ if (list == NULL)
+ return NULL;
+
+ n = g_slist_length(list);
+ array = g_new0(char *, n + 1);
+
+ for (l = list, i = 0; l; l = l->next, i++)
+ array[i] = g_strdup((const gchar *) l->data);
+
+ return array;
+}
+
+static GSList *get_eir_uuids(uint8_t *eir_data, size_t eir_length, GSList *list)
{
uint16_t len = 0;
- char **uuids;
size_t total;
size_t uuid16_count = 0;
size_t uuid32_count = 0;
@@ -2839,10 +2857,11 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
uint8_t *uuid32;
uint8_t *uuid128;
uuid_t service;
+ char *uuid_str;
unsigned int i;
if (eir_data == NULL || eir_length == 0)
- return NULL;
+ return list;
while (len < eir_length - 1) {
uint8_t field_len = eir_data[0];
@@ -2875,15 +2894,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
/* Bail out if got incorrect length */
if (len > eir_length)
- return NULL;
+ return list;
total = uuid16_count + uuid32_count + uuid128_count;
- *uuid_count = total;
if (!total)
- return NULL;
-
- uuids = g_new0(char *, total + 1);
+ return list;
/* Generate uuids in SDP format (EIR data is Little Endian) */
service.type = SDP_UUID16;
@@ -2892,7 +2908,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
val16 = (val16 << 8) + uuid16[0];
service.value.uuid16 = val16;
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid16 += 2;
}
@@ -2905,7 +2926,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
val32 = (val32 << 8) + uuid32[k];
service.value.uuid32 = val32;
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid32 += 4;
}
@@ -2916,11 +2942,16 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
for (k = 0; k < 16; k++)
service.value.uuid128.data[k] = uuid128[16 - k - 1];
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid128 += 16;
}
- return uuids;
+ return list;
}
void adapter_emit_device_found(struct btd_adapter *adapter,
@@ -2934,7 +2965,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
dbus_int16_t rssi = dev->rssi;
char *alias;
char **uuids = NULL;
- size_t uuid_count = 0;
+ size_t uuid_count;
ba2str(&dev->bdaddr, peer_addr);
ba2str(&adapter->bdaddr, local_addr);
@@ -2954,8 +2985,16 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
} else
alias = g_strdup(dev->alias);
- /* Extract UUIDs from extended inquiry response if any*/
- uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
+ /* Extract UUIDs from extended inquiry response if any */
+ dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+ uuid_count = g_slist_length(dev->services);
+
+ if (dev->services) {
+ uuids = strlist2array(dev->services);
+ g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+ g_slist_free(dev->services);
+ dev->services = NULL;
+ }
emit_device_found(adapter->path, paddr,
"Address", DBUS_TYPE_STRING, &paddr,
diff --git a/src/adapter.h b/src/adapter.h
index a744f61..4af69b3 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -71,6 +71,7 @@ struct remote_dev_info {
name_status_t name_status;
gboolean le;
/* LE adv data */
+ GSList *services;
uint8_t evt_type;
uint8_t bdaddr_type;
};
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/3] Advertising data: extract local name
From: Bruna Moreira @ 2010-11-17 15:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
Move extract_eir_name() to glib-helper.c file and rename function to
bt_extract_eir_name(). The local name is extracted from the advertising
data.
---
src/adapter.c | 9 +++++++++
src/event.c | 23 +----------------------
src/glib-helper.c | 22 ++++++++++++++++++++++
src/glib-helper.h | 1 +
4 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 7fb0d3f..0089d6d 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3007,6 +3007,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr;
gboolean new_dev;
int8_t rssi;
+ uint8_t type;
rssi = *(info->data + info->length);
bdaddr = info->bdaddr;
@@ -3023,6 +3024,14 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
adapter->found_devices = g_slist_sort(adapter->found_devices,
(GCompareFunc) dev_rssi_cmp);
+
+ if (info->length) {
+ char *tmp_name = bt_extract_eir_name(info->data, &type);
+ if (tmp_name) {
+ g_free(dev->name);
+ dev->name = tmp_name;
+ }
+ }
}
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
diff --git a/src/event.c b/src/event.c
index 008bbe3..daab71a 100644
--- a/src/event.c
+++ b/src/event.c
@@ -301,27 +301,6 @@ void btd_event_simple_pairing_complete(bdaddr_t *local, bdaddr_t *peer,
device_simple_pairing_complete(device, status);
}
-static char *extract_eir_name(uint8_t *data, uint8_t *type)
-{
- if (!data || !type)
- return NULL;
-
- if (data[0] == 0)
- return NULL;
-
- *type = data[1];
-
- switch (*type) {
- case 0x08:
- case 0x09:
- if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
- return strdup("");
- return strndup((char *) (data + 2), data[0] - 1);
- }
-
- return NULL;
-}
-
void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
{
struct btd_adapter *adapter;
@@ -410,7 +389,7 @@ void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- tmp_name = extract_eir_name(data, &name_type);
+ tmp_name = bt_extract_eir_name(data, &name_type);
if (tmp_name) {
if (name_type == 0x09) {
write_device_name(local, peer, tmp_name);
diff --git a/src/glib-helper.c b/src/glib-helper.c
index 9d76626..33668d7 100644
--- a/src/glib-helper.c
+++ b/src/glib-helper.c
@@ -43,6 +43,7 @@
#include <glib.h>
#include "glib-helper.h"
+#include "sdpd.h"
/* Number of seconds to keep a sdp_session_t in the cache */
#define CACHE_TIMEOUT 2
@@ -576,3 +577,24 @@ GSList *bt_string2list(const gchar *str)
return l;
}
+
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type)
+{
+ if (!data || !type)
+ return NULL;
+
+ if (data[0] == 0)
+ return NULL;
+
+ *type = data[1];
+
+ switch (*type) {
+ case EIR_NAME_SHORT:
+ case EIR_NAME_COMPLETE:
+ if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
+ return strdup("");
+ return strndup((char *) (data + 2), data[0] - 1);
+ }
+
+ return NULL;
+}
diff --git a/src/glib-helper.h b/src/glib-helper.h
index e89c2c6..dfe4123 100644
--- a/src/glib-helper.h
+++ b/src/glib-helper.h
@@ -38,3 +38,4 @@ char *bt_name2string(const char *string);
int bt_string2uuid(uuid_t *uuid, const char *string);
gchar *bt_list2string(GSList *list);
GSList *bt_string2list(const gchar *str);
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type);
--
1.7.0.4
^ permalink raw reply related
* RE: [PATCH] Adding a new option to specify security level for gatttool
From: tim.howes @ 2010-11-17 15:03 UTC (permalink / raw)
To: Mike.Tsai; +Cc: linux-bluetooth
In-Reply-To: <35B17FE5076C7040809188FBE7913F98406D465C43@SC1EXMB-MBCL.global.atheros.com>
Hi Mike,
-----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Johan Hedberg
> Sent: Tuesday, November 16, 2010 7:37 AM
> To: Sheldon Demario
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Adding a new option to specify security level for
> gatttool
>
> Hi Sheldon,
>
> On Tue, Nov 16, 2010, Sheldon Demario wrote:
> > ---
> > TODO | 6 ------
> > attrib/gatttool.c | 15 +++++++++++++--
> > 2 files changed, 13 insertions(+), 8 deletions(-)
>
> In general the patch seems fine, but:
>
> > + if (strncmp(opt_sec_level, "medium", 6) == 0)
> > + sec_level = BT_IO_SEC_MEDIUM;
> > + else if (strncmp(opt_sec_level, "high", 4) == 0)
> > + sec_level = BT_IO_SEC_HIGH;
> > + else
> > + sec_level = BT_IO_SEC_LOW;
>
> Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
> think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM
> ;)
>
> [Mtsai] I am not sure what are the definition of "low", "medium" or
> "high". By the spec of Core 4.0, LE has 2 security modes and different
> security levels based on the method of pairing (or bonding). It may be
> appeal to end user with "low", "medium" and "high" definition, but it
> can't be reference with LE spec. I would suggest, instead, following
> terms,
>
> "No security",
> "unauthenticated encryption",
> "authenticated encryption",
> "unauthenticated data signing",
> "authenticated data signing,
>
> Mike
To some extent I agree; however, the semantics of such an API would have to be careful. A particular profile should not "force" data signing because if the link is already encrypted there is little point using data signing. So from that point of view exposing a more abstract API (a bit like "high") is better. However, it is hard to map "high" onto any of the ones you listed (which I agree is a good list). So perhaps it is better to have the API semantics as "advisory" or "requests" which can be fulfilled by the underlying stack in other ways (eg encryption for data-signing).
Cheers
Tim
This message is for the designated recipient only and may contain privileged, proprietary, or otherwise private information. If you have received it in error, please notify the sender immediately and delete the original. Any other use of the email by you is prohibited.
^ permalink raw reply
* RE: [PATCH] Adding a new option to specify security level for gatttool
From: Mike Tsai @ 2010-11-17 14:25 UTC (permalink / raw)
To: Johan Hedberg, Sheldon Demario; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101116153648.GA2710@jh-x301>
Hi,
-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Johan Hedberg
Sent: Tuesday, November 16, 2010 7:37 AM
To: Sheldon Demario
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] Adding a new option to specify security level for gatttool
Hi Sheldon,
On Tue, Nov 16, 2010, Sheldon Demario wrote:
> ---
> TODO | 6 ------
> attrib/gatttool.c | 15 +++++++++++++--
> 2 files changed, 13 insertions(+), 8 deletions(-)
In general the patch seems fine, but:
> + if (strncmp(opt_sec_level, "medium", 6) == 0)
> + sec_level = BT_IO_SEC_MEDIUM;
> + else if (strncmp(opt_sec_level, "high", 4) == 0)
> + sec_level = BT_IO_SEC_HIGH;
> + else
> + sec_level = BT_IO_SEC_LOW;
Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM ;)
[Mtsai] I am not sure what are the definition of "low", "medium" or "high". By the spec of Core 4.0, LE has 2 security modes and different security levels based on the method of pairing (or bonding). It may be appeal to end user with "low", "medium" and "high" definition, but it can't be reference with LE spec. I would suggest, instead, following terms,
"No security",
"unauthenticated encryption",
"authenticated encryption",
"unauthenticated data signing",
"authenticated data signing,
Mike
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3] Adding a new option to specify security level for gatttool
From: Sheldon Demario @ 2010-11-17 13:26 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Sheldon Demario
---
TODO | 6 ------
attrib/gatttool.c | 15 +++++++++++++--
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/TODO b/TODO
index c0a25f1..49a9e76 100644
--- a/TODO
+++ b/TODO
@@ -123,12 +123,6 @@ ATT/GATT
Priority: Low
Complexity: C1
-- Add command line support to use medium instead of (default) low
- security level with gatttool (--sec-level)
-
- Priority: Low
- Complexity: C1
-
- Implement Server characteristic Configuration support in the attribute
server to manage characteristic value broadcasting. There is a single
instance of the Server Characteristic Configuration for all clients.
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index b9f5138..636e0c3 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -49,6 +49,7 @@
static gchar *opt_src = NULL;
static gchar *opt_dst = NULL;
static gchar *opt_value = NULL;
+static gchar *opt_sec_level = "low";
static int opt_start = 0x0001;
static int opt_end = 0xffff;
static int opt_handle = -1;
@@ -84,6 +85,7 @@ static GIOChannel *do_connect(gboolean le)
GIOChannel *chan;
bdaddr_t sba, dba;
GError *err = NULL;
+ BtIOSecLevel sec_level;
/* This check is required because currently setsockopt() returns no
* errors for MTU values smaller than the allowed minimum. */
@@ -109,13 +111,20 @@ static GIOChannel *do_connect(gboolean le)
} else
bacpy(&sba, BDADDR_ANY);
+ if (strcmp(opt_sec_level, "medium") == 0)
+ sec_level = BT_IO_SEC_MEDIUM;
+ else if (strcmp(opt_sec_level, "high") == 0)
+ sec_level = BT_IO_SEC_HIGH;
+ else
+ sec_level = BT_IO_SEC_LOW;
+
if (le)
chan = bt_io_connect(BT_IO_L2CAP, connect_cb, NULL, NULL, &err,
BT_IO_OPT_SOURCE_BDADDR, &sba,
BT_IO_OPT_DEST_BDADDR, &dba,
BT_IO_OPT_CID, GATT_CID,
BT_IO_OPT_OMTU, opt_mtu,
- BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+ BT_IO_OPT_SEC_LEVEL, sec_level,
BT_IO_OPT_INVALID);
else
chan = bt_io_connect(BT_IO_L2CAP, connect_cb, NULL, NULL, &err,
@@ -123,7 +132,7 @@ static GIOChannel *do_connect(gboolean le)
BT_IO_OPT_DEST_BDADDR, &dba,
BT_IO_OPT_PSM, opt_psm,
BT_IO_OPT_OMTU, opt_mtu,
- BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+ BT_IO_OPT_SEC_LEVEL, sec_level,
BT_IO_OPT_INVALID);
if (err) {
@@ -519,6 +528,8 @@ static GOptionEntry options[] = {
"Specify the MTU size", "MTU" },
{ "psm", 'p', 0, G_OPTION_ARG_INT, &opt_psm,
"Specify the PSM for GATT/ATT over BR/EDR", "PSM" },
+ { "sec-level", 'l', 0, G_OPTION_ARG_STRING, &opt_sec_level,
+ "Set security level. Default: low", "[low | medium | high]"},
{ NULL },
};
--
1.7.3.2
^ permalink raw reply related
* Re: Does OBEX authentication works in OPP?
From: liang yu @ 2010-11-17 13:03 UTC (permalink / raw)
To: liang yu, linux-bluetooth
In-Reply-To: <20101117121526.GA16972@jh-x301>
On Wed, Nov 17, 2010 at 8:15 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Wed, Nov 17, 2010, liang yu wrote:
>> According to the Bluetooth SIG profile OBEX authentication is not used
>> in OPP. But in source code of Obexd-0.36/plugins/opp.c, the function
>> opp_chkput invokes manager_request_authorization. Does this mean that
>> in your implementation it is still checked OBEX authentication in opp?
>> Doea anybody give me some tips.
>
> The code you found (and as the function name also implies) is about
> authorization and not authentication. So no, OBEX authentication is not
> used with OPP.
>
> Johan
>
Get it .Thanks ,Hedberg
^ permalink raw reply
* Re: [PATCHv3 3/7] Add DBus OOB API documentation.
From: Szymon Janc @ 2010-11-17 12:49 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101116163729.GA3854@jh-x301>
Hi Johan,
> > +Service org.bluez
> > +Interface org.bluez.OOB
> > +Object path /org/bluez
>
> I think this should use the same path as the Manager API, which at the
> moment is /
I was following HealthManager way, but I'm fine if you prefer / path.
> > + array{byte} hash, array{byte} randomizer
> > + ReadLocalOobData(object adapter)
>
> Having to pass an adapter path as a parameter seems weird. Wouldn't it
> make more sense to have this method in the Adapter path/interface
> instead? Also, we could toy around with the thought of putting the two
> other methods into the current Manager interface.
It is OK to me to move ReadLocalOobData to adapter path as this is an adapter
related data.
My initial idea was to keep all oob related methods in org.bluez.OOB interface
but I'm not going to insist on it.
--
Szymon Janc
on behalf of ST-Ericsson
^ permalink raw reply
* Re: Does OBEX authentication works in OPP?
From: Johan Hedberg @ 2010-11-17 12:15 UTC (permalink / raw)
To: liang yu; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikTyH+3LLbr=ytu+9jcE8Sk_wTqdRyn9jwK821a@mail.gmail.com>
Hi,
On Wed, Nov 17, 2010, liang yu wrote:
> According to the Bluetooth SIG profile OBEX authentication is not used
> in OPP. But in source code of Obexd-0.36/plugins/opp.c, the function
> opp_chkput invokes manager_request_authorization. Does this mean that
> in your implementation it is still checked OBEX authentication in opp?
> Doea anybody give me some tips.
The code you found (and as the function name also implies) is about
authorization and not authentication. So no, OBEX authentication is not
used with OPP.
Johan
^ permalink raw reply
* Re: Apple Wireless Keyboard connection issue
From: Johan Hedberg @ 2010-11-17 9:05 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: André Kühne, linux-bluetooth
In-Reply-To: <20101117013009.GA15855@vigoh>
[-- Attachment #1: Type: text/plain, Size: 716 bytes --]
Hi Gustavo,
On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> * "André Kühne" <andre.kuehne@gmx.net> [2010-11-16 22:27:01 +0100]:
> > I noticed the following on my system: After upgrading to bluez-4.79
> > connecting my Apple Wireless Keyboard does not work anymore. With
> > bluez-4.77 the connection works just fine.
>
> My Microsoft keyboard is not working too. That is probably due to commit
> abe7cd44124a from Johan. It should be fixed soon.
The only real difference that patch makes is the reuse of the HCI socket
inside hciops.c. Maybe that screws up the event filters somehow or
something similar. I don't have a keyboard to verify a fix, but could
you try the attached patch and see if it helps?
Johan
[-- Attachment #2: hciops_encrypt.patch --]
[-- Type: text/x-diff, Size: 1783 bytes --]
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 9d25558..4492983 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -338,31 +338,37 @@ static int hciops_encrypt_link(int index, bdaddr_t *dst, bt_hci_result_t cb,
uint32_t link_mode;
uint16_t handle;
+ dd = hci_open_dev(index);
+ if (dd < 0)
+ return -errno;
+
cr = g_malloc0(sizeof(*cr) + sizeof(struct hci_conn_info));
cr->type = ACL_LINK;
bacpy(&cr->bdaddr, dst);
- err = ioctl(SK(index), HCIGETCONNINFO, cr);
+ err = ioctl(dd, HCIGETCONNINFO, cr);
link_mode = cr->conn_info->link_mode;
handle = cr->conn_info->handle;
g_free(cr);
- if (err < 0)
- return -errno;
+ if (err < 0) {
+ err = -errno;
+ goto fail;
+ }
- if (link_mode & HCI_LM_ENCRYPT)
- return -EALREADY;
+ if (link_mode & HCI_LM_ENCRYPT) {
+ err = -EALREADY;
+ goto fail;
+ }
memset(&cp, 0, sizeof(cp));
cp.handle = htobs(handle);
- if (hci_send_cmd(SK(index), OGF_LINK_CTL, OCF_AUTH_REQUESTED,
- AUTH_REQUESTED_CP_SIZE, &cp) < 0)
- return -errno;
-
- dd = dup(SK(index));
- if (dd < 0)
- return -errno;
+ if (hci_send_cmd(dd, OGF_LINK_CTL, OCF_AUTH_REQUESTED,
+ AUTH_REQUESTED_CP_SIZE, &cp) < 0) {
+ err = -errno;
+ goto fail;
+ }
cmd = g_new0(struct hci_cmd_data, 1);
cmd->handle = handle;
@@ -379,8 +385,7 @@ static int hciops_encrypt_link(int index, bdaddr_t *dst, bt_hci_result_t cb,
if (setsockopt(dd, SOL_HCI, HCI_FILTER, &nf, sizeof(nf)) < 0) {
err = -errno;
g_free(cmd);
- close(dd);
- return -err;
+ goto fail;
}
io = g_io_channel_unix_new(dup(SK(index)));
@@ -391,6 +396,10 @@ static int hciops_encrypt_link(int index, bdaddr_t *dst, bt_hci_result_t cb,
g_io_channel_unref(io);
return 0;
+
+fail:
+ close(dd);
+ return err;
}
/* End async HCI command handling */
^ permalink raw reply related
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Pavan Savoy @ 2010-11-17 5:43 UTC (permalink / raw)
To: Vitaly Wool; +Cc: Gustavo F. Padovan, marcel, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTi=TxKKxjLdK-6Jwu9oBCiGHXJ8UxrAQH+C82wNK@mail.gmail.com>
On Wed, Nov 17, 2010 at 4:50 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
>>> + =C2=A0 =C2=A0 /* Registration with ST layer is successful,
>>> + =C2=A0 =C2=A0 =C2=A0* hardware is ready to accept commands from HCI c=
ore.
>>> + =C2=A0 =C2=A0 =C2=A0*/
>>> + =C2=A0 =C2=A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(HCI_RUNNING, &hde=
v->flags);
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST_BT=
);
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
BT_ERR("st_unregister() failed with error %d", err);
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
>>> + =C2=A0 =C2=A0 }
>>
>>
>> What are you trying to do here? test_and_set_bit() result doesn't say
>> nothing about error and you shall put test_and_set_bit should be in the
>> beginning, to know if your device is already opened or not and then
>> clear_bit if some error ocurrs during the function.
>>
>
> Yeap, this piece of code beats me is well. Why is it an error if this
> bit wasn't already set?
Vitaly, Gustavo,
I suppose I never understood HCI_RUNNING flag that way, as in an error
check mechanism to avoid multiple hci0 ups.
What I understood was that HCI_RUNNING suggested as to when hci0 was
ready to be used. With this understanding, I wanted to make sure I
downloaded the firmware for the chip before I proclaim to the world
that the hci0 is ready to be used, as in HCI_RUNNING.
For example, I didn't want my _send_frame to be called before I did
the firmware download - since firmware download takes time - 45kb
send/wait commands :(
But I suppose I now understand - What I would rather do is test_bit in
the beginning of function and do a set_bit at the end of function -
does this make sense ?
> ~Vitaly
> --
> 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 =C2=A0http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Pavan Savoy @ 2010-11-17 5:34 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel
In-Reply-To: <20101116225418.GA15101@vigoh>
Gustavo,
On Wed, Nov 17, 2010 at 4:24 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Pavan,
>
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-10 08:07:26 -0500]:
>
>> From: Pavan Savoy <pavan_savoy@ti.com>
>>
>> Marcel,
>>
>> Thanks for the comments...
>> This patch contains,
>> v5 comments :-
>> declaration and assiging of variables and private data fixed up.
>> proper casting.
>> removed redundant un-necessary checks in send_frame.
>> HCI_RUNNING fixes in terms of test_and_set/clear bit instead of set and
>> clear.
>> removed redundant checks for hdev, skb being NULL.
>> removed module_param of reset, also WiLink don't need HCI_RESET anyways.
>> removed ti_st_register_dev function and functionality moved to _probe.
>> module_init/exit function names fixed up.
>>
>> stat byte counter increments and tx_complete is similar to hci_ldisc.
>> Also I have not implemented the flush routine, since the functionality
>> which needs to be done in flush routine is done in the underlying driver
>> which is the shared transport driver and moreover the btwilink driver by
>> itself doesn't maintains queue or data relevant to transport, so nothing
>> to do.
>>
>> And Yes, I have verified this driver with multiple up/down reset on
>> hci0.
>> Also I generally test a2dp/ftp to verify large data transfers.
>
> Before re-submit a patch you have to fix all issues reported by the
> reviewer or reply to patch thread why do you think you right and don't
> want to change that. That is not happening with your patches, you are
> not fixing all the stuff before re-submiting and it is not the first
> time. =C2=A0If you do it right we can review it fast and your code goes
> earlier to mainline.
> You should also answer the questions first =C2=A0like "Where is the
> ti_st_proto.write coming from?" You just ignored the
> review and submitted a new patch.
This is the reason, I tend to keep the patch comments in the mail.
I have mentioned here the
1. comments I have taken care of,
2. few comments which I don't understand why it is like that and which
are not taken care of.
Also the question on ti_st_proto.write, I have answered it twice in
mail, once to you and once to marcel, for more clarity I have even
added it in the code comments, Please have a look @ line
>> + /* ti_st_proto.write is filled up by the underlying shared
>> + * transport driver upon registration
>> + */
As to why this function is not EXPORT_SYMBOL and just an function
pointer, Yes I had it as function pointer - But since something like
_read is callback from the protocol driver perspective - to maintain
uniformity I have this as function pointer.
(Note: comments to other drivers which are based on ST driver intended
read/write to be pointers which register/unregister to be EXPORTs).
Ok, apart from this there was an open question as why I am checking
for only 2 error conditions, it is because the underlying driver only
can send those 2 errors and nothing else (st_register has few errors
it can throw...)
I understand my problem with test_and_set_bit, I will correct it, But
I still feel I do nothing critical before test/set so as to return
error if already opened - But will correct it and do it at the
beginning ...
Also what should I be doing regarding the deferred stats update? I
checked up hci_ldisc which is what this code is pretty much based on,
and see that it does pretty much the same thing ....
hci_uart_tx_complete is called after the tty->ops->write correct ?
I also now understand the issue in module_init regarding the
platform_driver registration and will fix that.
Thanks,
Pavan
>>
>> v4 comments :-
>> module init now returns what platform_driver_register returns.
>> type casting of void* private data has been removed
>>
>> v3 comments :-
>> Lizardo,
>> I have taken care of most of the comments you had.
>> Have re-wrote some of the code commenting you've mentioned.
>> Thanks for the comments,
>>
>> The other few like -EPERM for platform driver registration is to keep
>> it similar to other drivers, type casting is maintained just to feel saf=
e
>> and have style similar to other drivers.
>> BT_WILINK in Kconfig is similar to BT_MRVL.
>> I hope those aren't too critical.
>>
>> -- patch description --
>>
>> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
>> Texas Instrument's WiLink chipsets combine wireless technologies
>> like BT, FM, GPS and WLAN onto a single chip.
>>
>> This Bluetooth driver works on top of the TI_ST shared transport
>> line discipline driver which also allows other drivers like
>> FM V4L2 and GPS character driver to make use of the same UART interface.
>>
>> Kconfig and Makefile modifications to enable the Bluetooth
>> driver for Texas Instrument's WiLink 7 chipset.
>>
>> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
>> ---
>> =C2=A0drivers/bluetooth/Kconfig =C2=A0 =C2=A0| =C2=A0 10 +
>> =C2=A0drivers/bluetooth/Makefile =C2=A0 | =C2=A0 =C2=A01 +
>> =C2=A0drivers/bluetooth/btwilink.c | =C2=A0379 +++++++++++++++++++++++++=
+++++++++++++++++
>> =C2=A03 files changed, 390 insertions(+), 0 deletions(-)
>> =C2=A0create mode 100644 drivers/bluetooth/btwilink.c
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 02deef4..8e0de9a 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -219,4 +219,14 @@ config BT_ATH3K
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 Say Y here to compile support for "Atheros f=
irmware download driver"
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 into the kernel or say M to compile it as mo=
dule (ath3k).
>>
>> +config BT_WILINK
>> + =C2=A0 =C2=A0 tristate "Texas Instruments WiLink7 driver"
>> + =C2=A0 =C2=A0 depends on TI_ST
>> + =C2=A0 =C2=A0 help
>> + =C2=A0 =C2=A0 =C2=A0 This enables the Bluetooth driver for Texas Instr=
ument's BT/FM/GPS
>> + =C2=A0 =C2=A0 =C2=A0 combo devices. This makes use of shared transport=
line discipline
>> + =C2=A0 =C2=A0 =C2=A0 core driver to communicate with the BT core of th=
e combo chip.
>> +
>> + =C2=A0 =C2=A0 =C2=A0 Say Y here to compile support for Texas Instrumen=
t's WiLink7 driver
>> + =C2=A0 =C2=A0 =C2=A0 into the kernel or say M to compile it as module.
>> =C2=A0endmenu
>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
>> index 71bdf13..f4460f4 100644
>> --- a/drivers/bluetooth/Makefile
>> +++ b/drivers/bluetooth/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) =C2=A0+=3D btsdio.o
>> =C2=A0obj-$(CONFIG_BT_ATH3K) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 +=3D ath3k.o
>> =C2=A0obj-$(CONFIG_BT_MRVL) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0+=3D btmrvl.o
>> =C2=A0obj-$(CONFIG_BT_MRVL_SDIO) =C2=A0 +=3D btmrvl_sdio.o
>> +obj-$(CONFIG_BT_WILINK) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0+=3D btwilink.o
>>
>> =C2=A0btmrvl-y =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 :=3D btmrvl_main.o
>> =C2=A0btmrvl-$(CONFIG_DEBUG_FS) =C2=A0 =C2=A0+=3D btmrvl_debugfs.o
>> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
>> new file mode 100644
>> index 0000000..1b1c4bc
>> --- /dev/null
>> +++ b/drivers/bluetooth/btwilink.c
>> @@ -0,0 +1,379 @@
>> +/*
>> + * =C2=A0Texas Instrument's Bluetooth Driver For Shared Transport.
>> + *
>> + * =C2=A0Bluetooth Driver acts as interface between HCI core and
>> + * =C2=A0TI Shared Transport Layer.
>> + *
>> + * =C2=A0Copyright (C) 2009-2010 Texas Instruments
>> + * =C2=A0Author: Raja Mani <raja_mani@ti.com>
>> + * =C2=A0 Pavan Savoy <pavan_savoy@ti.com>
>> + *
>> + * =C2=A0This program is free software; you can redistribute it and/or =
modify
>> + * =C2=A0it under the terms of the GNU General Public License version 2=
as
>> + * =C2=A0published by the Free Software Foundation.
>> + *
>> + * =C2=A0This program is distributed in the hope that it will be useful=
,
>> + * =C2=A0but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * =C2=A0MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =C2=A0See=
the
>> + * =C2=A0GNU General Public License for more details.
>> + *
>> + * =C2=A0You should have received a copy of the GNU General Public Lice=
nse
>> + * =C2=A0along with this program; if not, write to the Free Software
>> + * =C2=A0Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA =C2=
=A002111-1307 =C2=A0USA
>> + *
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <net/bluetooth/bluetooth.h>
>> +#include <net/bluetooth/hci_core.h>
>> +
>> +#include <linux/ti_wilink_st.h>
>> +
>> +/* Bluetooth Driver Version */
>> +#define VERSION =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "1.0"
>> +
>> +/* Number of seconds to wait for registration completion
>> + * when ST returns PENDING status.
>> + */
>> +#define BT_REGISTER_TIMEOUT =C2=A0 6000 =C2=A0 /* 6 sec */
>> +
>> +/**
>> + * struct ti_st - driver operation structure
>> + * @hdev: hci device pointer which binds to bt driver
>> + * @reg_status: ST registration callback status
>> + * @st_write: write function provided by the ST driver
>> + * =C2=A0 to be used by the driver during send_frame.
>> + * @wait_reg_completion - completion sync between ti_st_open
>> + * =C2=A0 and ti_st_registration_completion_cb.
>> + */
>> +struct ti_st {
>> + =C2=A0 =C2=A0 struct hci_dev *hdev;
>> + =C2=A0 =C2=A0 char reg_status;
>> + =C2=A0 =C2=A0 long (*st_write) (struct sk_buff *);
>> + =C2=A0 =C2=A0 struct completion wait_reg_completion;
>> +};
>> +
>> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
>> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
>> +{
>> + =C2=A0 =C2=A0 struct hci_dev *hdev =3D hst->hdev;
>> +
>> + =C2=A0 =C2=A0 /* Update HCI stat counters */
>> + =C2=A0 =C2=A0 switch (pkt_type) {
>> + =C2=A0 =C2=A0 case HCI_COMMAND_PKT:
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.cmd_tx++;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>> +
>> + =C2=A0 =C2=A0 case HCI_ACLDATA_PKT:
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.acl_tx++;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>> +
>> + =C2=A0 =C2=A0 case HCI_SCODATA_PKT:
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.sco_tx++;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>> + =C2=A0 =C2=A0 }
>> +}
>> +
>> +/* ------- Interfaces to Shared Transport ------ */
>> +
>> +/* Called by ST layer to indicate protocol registration completion
>> + * status.ti_st_open() function will wait for signal from this
>> + * API when st_register() function returns ST_PENDING.
>> + */
>> +static void st_registration_completion_cb(void *priv_data, char data)
>> +{
>> + =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
>> +
>> + =C2=A0 =C2=A0 /* Save registration status for use in ti_st_open() */
>> + =C2=A0 =C2=A0 lhst->reg_status =3D data;
>> + =C2=A0 =C2=A0 /* complete the wait in ti_st_open() */
>> + =C2=A0 =C2=A0 complete(&lhst->wait_reg_completion);
>> +}
>> +
>> +/* Called by Shared Transport layer when receive data is
>> + * available */
>> +static long st_receive(void *priv_data, struct sk_buff *skb)
>> +{
>> + =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
>> + =C2=A0 =C2=A0 int err;
>> +
>> + =C2=A0 =C2=A0 if (!skb)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
>> +
>> + =C2=A0 =C2=A0 if (!lhst) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
>> + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 skb->dev =3D (void *) lhst->hdev;
>> +
>> + =C2=A0 =C2=A0 /* Forward skb to HCI core layer */
>> + =C2=A0 =C2=A0 err =3D hci_recv_frame(skb);
>> + =C2=A0 =C2=A0 if (err < 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Unable to push skb t=
o HCI core(%d)", err);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
>> + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 lhst->hdev->stat.byte_rx +=3D skb->len;
>> +
>> + =C2=A0 =C2=A0 return 0;
>> +}
>> +
>> +/* ------- Interfaces to HCI layer ------ */
>> +/* protocol structure registered with shared transport */
>> +static struct st_proto_s ti_st_proto =3D {
>> + =C2=A0 =C2=A0 .type =3D ST_BT,
>> + =C2=A0 =C2=A0 .recv =3D st_receive,
>> + =C2=A0 =C2=A0 .reg_complete_cb =3D st_registration_completion_cb,
>> +};
>> +
>> +/* Called from HCI core to initialize the device */
>> +static int ti_st_open(struct hci_dev *hdev)
>> +{
>> + =C2=A0 =C2=A0 unsigned long timeleft;
>> + =C2=A0 =C2=A0 struct ti_st *hst;
>> + =C2=A0 =C2=A0 int err;
>> +
>> + =C2=A0 =C2=A0 BT_DBG("%s %p", hdev->name, hdev);
>> +
>> + =C2=A0 =C2=A0 /* provide contexts for callbacks from ST */
>> + =C2=A0 =C2=A0 hst =3D hdev->driver_data;
>> + =C2=A0 =C2=A0 ti_st_proto.priv_data =3D hst;
>> +
>> + =C2=A0 =C2=A0 err =3D st_register(&ti_st_proto);
>> + =C2=A0 =C2=A0 if (err =3D=3D -EINPROGRESS) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Prepare wait-for-completi=
on handler data structures.
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Needed to synchroniz=
e this and
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* st_registration_comp=
letion_cb() functions.
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 init_completion(&hst->wait_r=
eg_completion);
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Reset ST registration cal=
lback status flag , this value
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* will be updated in t=
i_st_registration_completion_cb()
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* function whenever it=
called from ST driver.
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->reg_status =3D -EINPROG=
RESS;
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* ST is busy with either pr=
otocol registration or firmware
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* download. Wait until=
the registration callback is called
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_DBG(" waiting for registr=
ation completion signal from ST");
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeleft =3D wait_for_comple=
tion_timeout
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
(&hst->wait_reg_completion,
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0msecs_to_jiffies(BT_REGISTER_TIMEOUT));
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!timeleft) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
BT_ERR("Timeout(%d sec),didn't get reg "
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "completion signal =
from ST",
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_REGISTER_TIMEOUT=
/ 1000);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
return -ETIMEDOUT;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Is ST registration callba=
ck called with ERROR status? */
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (hst->reg_status !=3D 0) =
{
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
BT_ERR("ST registration completed with invalid "
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "status %d", hst->r=
eg_status);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
return -EAGAIN;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D 0;
>> + =C2=A0 =C2=A0 } else if (err =3D=3D -EPERM) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_register failed %=
d", err);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
>> + =C2=A0 =C2=A0 }
>
>
> Again, you are checking for only EPERM and EINPROGRESS. You should check
> for everything. I actually don't undertand why you have a special check
> for EPERM.
>
>> +
>> + =C2=A0 =C2=A0 /* ti_st_proto.write is filled up by the underlying shar=
ed
>> + =C2=A0 =C2=A0 =C2=A0* transport driver upon registration
>> + =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 hst->st_write =3D ti_st_proto.write;
>
> I do not like that, the underlying should export the write function.
>
>> + =C2=A0 =C2=A0 if (!hst->st_write) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("undefined ST write f=
unction");
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Undo registration with ST=
*/
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST_BT)=
;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
BT_ERR("st_unregister() failed with error %d", err);
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
>> + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 /* Registration with ST layer is successful,
>> + =C2=A0 =C2=A0 =C2=A0* hardware is ready to accept commands from HCI co=
re.
>> + =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(HCI_RUNNING, &hdev=
->flags);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST_BT)=
;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
BT_ERR("st_unregister() failed with error %d", err);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
>> + =C2=A0 =C2=A0 }
>
>
> What are you trying to do here? test_and_set_bit() result doesn't say
> nothing about error and you shall put test_and_set_bit should be in the
> beginning, to know if your device is already opened or not and then
> clear_bit if some error ocurrs during the function.
>
>> +
>> + =C2=A0 =C2=A0 return err;
>> +}
>> +
>> +/* Close device */
>> +static int ti_st_close(struct hci_dev *hdev)
>> +{
>> + =C2=A0 =C2=A0 int err;
>> + =C2=A0 =C2=A0 struct ti_st *hst =3D hdev->driver_data;
>> +
>> + =C2=A0 =C2=A0 /* continue to unregister from transport */
>> + =C2=A0 =C2=A0 err =3D st_unregister(ST_BT);
>> + =C2=A0 =C2=A0 if (err)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_unregister() fail=
ed with error %d", err);
>> +
>> + =C2=A0 =C2=A0 hst->st_write =3D NULL;
>> +
>> + =C2=A0 =C2=A0 if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
>
> test_and_clear_bit should come first to check if your device is already
> closed.
>
>> +
>> + =C2=A0 =C2=A0 return err;
>> +}
>> +
>> +static int ti_st_send_frame(struct sk_buff *skb)
>> +{
>> + =C2=A0 =C2=A0 struct hci_dev *hdev;
>> + =C2=A0 =C2=A0 struct ti_st *hst;
>> + =C2=A0 =C2=A0 long len;
>> +
>> + =C2=A0 =C2=A0 hdev =3D (struct hci_dev *)skb->dev;
>> +
>> + =C2=A0 =C2=A0 if (!test_bit(HCI_RUNNING, &hdev->flags))
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EBUSY;
>> +
>> + =C2=A0 =C2=A0 hst =3D hdev->driver_data;
>> +
>> + =C2=A0 =C2=A0 /* Prepend skb with frame type */
>> + =C2=A0 =C2=A0 memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>> +
>> + =C2=A0 =C2=A0 BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pk=
t_type,
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
skb->len);
>> +
>> + =C2=A0 =C2=A0 /* Insert skb to shared transport layer's transmit queue=
.
>> + =C2=A0 =C2=A0 =C2=A0* Freeing skb memory is taken care in shared trans=
port layer,
>> + =C2=A0 =C2=A0 =C2=A0* so don't free skb memory here.
>> + =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 len =3D hst->st_write(skb);
>> + =C2=A0 =C2=A0 if (len < 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR(" ST write failed (%l=
d)", len);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EAGAIN;
>
> Why EAGAIN?
>
>> + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 /* ST accepted our skb. So, Go ahead and do rest */
>> + =C2=A0 =C2=A0 hdev->stat.byte_tx +=3D len;
>> + =C2=A0 =C2=A0 ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
>
> From Marcel in the other thread:
> "What is the reason for this deferred stats update. That code looks
> pretty much hackish to me."
>
>> +
>> + =C2=A0 =C2=A0 return 0;
>> +}
>> +
>> +static void ti_st_destruct(struct hci_dev *hdev)
>> +{
>> + =C2=A0 =C2=A0 BT_DBG("%s", hdev->name);
>> +
>> + =C2=A0 =C2=A0 /* free ti_st memory */
>
> get ride of the comment, it's pointless
>
>> + =C2=A0 =C2=A0 kfree(hdev->driver_data);
>> +
>> + =C2=A0 =C2=A0 return;
>
> No return; here
>
>> +}
>> +
>> +static int bt_ti_probe(struct platform_device *pdev)
>> +{
>> + =C2=A0 =C2=A0 static struct ti_st *hst;
>> + =C2=A0 =C2=A0 struct hci_dev *hdev;
>> + =C2=A0 =C2=A0 int err;
>> +
>> + =C2=A0 =C2=A0 hst =3D kzalloc(sizeof(struct ti_st), GFP_KERNEL);
>> + =C2=A0 =C2=A0 if (!hst)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
>> +
>> + =C2=A0 =C2=A0 /* Expose "hciX" device to user space */
>> + =C2=A0 =C2=A0 hdev =3D hci_alloc_dev();
>> + =C2=A0 =C2=A0 if (!hdev)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
>
> You are leaking hst, if hci_alloc_dev() fails.
>
>> +
>> + =C2=A0 =C2=A0 BT_DBG("hdev %p", hdev);
>> +
>> + =C2=A0 =C2=A0 hst->hdev =3D hdev;
>> + =C2=A0 =C2=A0 hdev->bus =3D HCI_UART;
>> + =C2=A0 =C2=A0 hdev->driver_data =3D hst;
>> + =C2=A0 =C2=A0 hdev->open =3D ti_st_open;
>> + =C2=A0 =C2=A0 hdev->close =3D ti_st_close;
>> + =C2=A0 =C2=A0 hdev->flush =3D NULL;
>> + =C2=A0 =C2=A0 hdev->send =3D ti_st_send_frame;
>> + =C2=A0 =C2=A0 hdev->destruct =3D ti_st_destruct;
>> + =C2=A0 =C2=A0 hdev->owner =3D THIS_MODULE;
>> +
>> + =C2=A0 =C2=A0 err =3D hci_register_dev(hdev);
>> + =C2=A0 =C2=A0 if (err < 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Can't register HCI d=
evice error %d", err);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hci_free_dev(hdev);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
>> + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 BT_DBG(" HCI device registered (hdev %p)", hdev);
>> +
>> + =C2=A0 =C2=A0 dev_set_drvdata(&pdev->dev, hst);
>> + =C2=A0 =C2=A0 return err;
>> +}
>> +
>> +static int bt_ti_remove(struct platform_device *pdev)
>> +{
>> + =C2=A0 =C2=A0 struct hci_dev *hdev;
>> + =C2=A0 =C2=A0 struct ti_st *hst =3D dev_get_drvdata(&pdev->dev);
>> +
>> + =C2=A0 =C2=A0 if (!hst)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
>> +
>> + =C2=A0 =C2=A0 hdev =3D hst->hdev;
>> + =C2=A0 =C2=A0 ti_st_close(hdev);
>> + =C2=A0 =C2=A0 hci_unregister_dev(hdev);
>> +
>> + =C2=A0 =C2=A0 /* Free HCI device memory */
>> + =C2=A0 =C2=A0 hci_free_dev(hdev);
>> +
>> + =C2=A0 =C2=A0 /* Free driver data memory */
>
> get ride of both commnets here. The name of the funcion is already
> saying that.
>
>> + =C2=A0 =C2=A0 kfree(hst);
>> +
>> + =C2=A0 =C2=A0 dev_set_drvdata(&pdev->dev, NULL);
>> + =C2=A0 =C2=A0 return 0;
>> +}
>> +
>> +static struct platform_driver btwilink_driver =3D {
>> + =C2=A0 =C2=A0 .probe =3D bt_ti_probe,
>> + =C2=A0 =C2=A0 .remove =3D bt_ti_remove,
>> + =C2=A0 =C2=A0 .driver =3D {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .name =3D "btwilink",
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .owner =3D THIS_MODULE,
>> + =C2=A0 =C2=A0 },
>> +};
>> +
>> +/* ------- Module Init/Exit interfaces ------ */
>> +static int __init btwilink_init(void)
>> +{
>> + =C2=A0 =C2=A0 long ret;
>> +
>> + =C2=A0 =C2=A0 BT_INFO(" Bluetooth Driver for TI WiLink - Version %s", =
VERSION);
>> +
>> + =C2=A0 =C2=A0 ret =3D platform_driver_register(&btwilink_driver);
>> + =C2=A0 =C2=A0 if (ret !=3D 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("btwilink platform dr=
iver registration failed");
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
>> + =C2=A0 =C2=A0 }
>> + =C2=A0 =C2=A0 return 0;
>
> Old issue again:
>
> From Marcel: please just do like we do with all other drivers;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0BT_INFO(...)
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return platform_driver_register(&btwilink_driv=
er);
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
> --
> 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 =C2=A0http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Does OBEX authentication works in OPP?
From: liang yu @ 2010-11-17 3:43 UTC (permalink / raw)
To: linux-bluetooth
Hello all:
I am not sure it is the right place to ask the question.
According to the Bluetooth SIG profile OBEX authentication is not used
in OPP. But in source code of Obexd-0.36/plugins/opp.c, the function
opp_chkput invokes manager_request_authorization. Does this mean that
in your implementation it is still checked OBEX authentication in opp?
Doea anybody give me some tips.
Thanks advance.
Best regards
^ permalink raw reply
* Re: Apple Wireless Keyboard connection issue
From: Gustavo F. Padovan @ 2010-11-17 1:30 UTC (permalink / raw)
To: "André Kühne"; +Cc: linux-bluetooth
In-Reply-To: <20101116212701.50060@gmx.net>
* "André Kühne" <andre.kuehne@gmx.net> [2010-11-16 22:27:01 +0100]:
> Hi everyone
>
> I noticed the following on my system: After upgrading to bluez-4.79 connecting my Apple Wireless Keyboard does not work anymore. With bluez-4.77 the connection works just fine.
>
My Microsoft keyboard is not working too. That is probably due to commit
abe7cd44124a from Johan. It should be fixed soon.
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Vitaly Wool @ 2010-11-16 23:20 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: pavan_savoy, marcel, linux-bluetooth, linux-kernel
In-Reply-To: <20101116225418.GA15101@vigoh>
>> + =A0 =A0 /* Registration with ST layer is successful,
>> + =A0 =A0 =A0* hardware is ready to accept commands from HCI core.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 clear_bit(HCI_RUNNING, &hdev->flags);
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D st_unregister(ST_BT);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister() failed=
with error %d", err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D NULL;
>> + =A0 =A0 }
>
>
> What are you trying to do here? test_and_set_bit() result doesn't say
> nothing about error and you shall put test_and_set_bit should be in the
> beginning, to know if your device is already opened or not and then
> clear_bit if some error ocurrs during the function.
>
Yeap, this piece of code beats me is well. Why is it an error if this
bit wasn't already set?
~Vitaly
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: btwilink driver
From: Gustavo F. Padovan @ 2010-11-16 22:54 UTC (permalink / raw)
To: pavan_savoy; +Cc: marcel, linux-bluetooth, linux-kernel
In-Reply-To: <1289394446-14021-1-git-send-email-pavan_savoy@ti.com>
Hi Pavan,
* pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-10 08:07:26 -0500]:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
> Marcel,
>
> Thanks for the comments...
> This patch contains,
> v5 comments :-
> declaration and assiging of variables and private data fixed up.
> proper casting.
> removed redundant un-necessary checks in send_frame.
> HCI_RUNNING fixes in terms of test_and_set/clear bit instead of set and
> clear.
> removed redundant checks for hdev, skb being NULL.
> removed module_param of reset, also WiLink don't need HCI_RESET anyways.
> removed ti_st_register_dev function and functionality moved to _probe.
> module_init/exit function names fixed up.
>
> stat byte counter increments and tx_complete is similar to hci_ldisc.
> Also I have not implemented the flush routine, since the functionality
> which needs to be done in flush routine is done in the underlying driver
> which is the shared transport driver and moreover the btwilink driver by
> itself doesn't maintains queue or data relevant to transport, so nothing
> to do.
>
> And Yes, I have verified this driver with multiple up/down reset on
> hci0.
> Also I generally test a2dp/ftp to verify large data transfers.
Before re-submit a patch you have to fix all issues reported by the
reviewer or reply to patch thread why do you think you right and don't
want to change that. That is not happening with your patches, you are
not fixing all the stuff before re-submiting and it is not the first
time. If you do it right we can review it fast and your code goes
earlier to mainline.
You should also answer the questions first like "Where is the
ti_st_proto.write coming from?" You just ignored the
review and submitted a new patch.
>
> Please review and comment.
>
> Thanks,
> Pavan
>
> v4 comments :-
> module init now returns what platform_driver_register returns.
> type casting of void* private data has been removed
>
> v3 comments :-
> Lizardo,
> I have taken care of most of the comments you had.
> Have re-wrote some of the code commenting you've mentioned.
> Thanks for the comments,
>
> The other few like -EPERM for platform driver registration is to keep
> it similar to other drivers, type casting is maintained just to feel safe
> and have style similar to other drivers.
> BT_WILINK in Kconfig is similar to BT_MRVL.
> I hope those aren't too critical.
>
> -- patch description --
>
> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> Texas Instrument's WiLink chipsets combine wireless technologies
> like BT, FM, GPS and WLAN onto a single chip.
>
> This Bluetooth driver works on top of the TI_ST shared transport
> line discipline driver which also allows other drivers like
> FM V4L2 and GPS character driver to make use of the same UART interface.
>
> Kconfig and Makefile modifications to enable the Bluetooth
> driver for Texas Instrument's WiLink 7 chipset.
>
> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> ---
> drivers/bluetooth/Kconfig | 10 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/btwilink.c | 379 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 390 insertions(+), 0 deletions(-)
> create mode 100644 drivers/bluetooth/btwilink.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..8e0de9a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> Say Y here to compile support for "Atheros firmware download driver"
> into the kernel or say M to compile it as module (ath3k).
>
> +config BT_WILINK
> + tristate "Texas Instruments WiLink7 driver"
> + depends on TI_ST
> + help
> + This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
> + combo devices. This makes use of shared transport line discipline
> + core driver to communicate with the BT core of the combo chip.
> +
> + Say Y here to compile support for Texas Instrument's WiLink7 driver
> + into the kernel or say M to compile it as module.
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..f4460f4 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o
> obj-$(CONFIG_BT_ATH3K) += ath3k.o
> obj-$(CONFIG_BT_MRVL) += btmrvl.o
> obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK) += btwilink.o
>
> btmrvl-y := btmrvl_main.o
> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..1b1c4bc
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,379 @@
> +/*
> + * Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * Bluetooth Driver acts as interface between HCI core and
> + * TI Shared Transport Layer.
> + *
> + * Copyright (C) 2009-2010 Texas Instruments
> + * Author: Raja Mani <raja_mani@ti.com>
> + * Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> +
> +/**
> + * struct ti_st - driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @reg_status: ST registration callback status
> + * @st_write: write function provided by the ST driver
> + * to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + struct hci_dev *hdev;
> + char reg_status;
> + long (*st_write) (struct sk_buff *);
> + struct completion wait_reg_completion;
> +};
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + struct hci_dev *hdev = hst->hdev;
> +
> + /* Update HCI stat counters */
> + switch (pkt_type) {
> + case HCI_COMMAND_PKT:
> + hdev->stat.cmd_tx++;
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + hdev->stat.acl_tx++;
> + break;
> +
> + case HCI_SCODATA_PKT:
> + hdev->stat.sco_tx++;
> + break;
> + }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> + struct ti_st *lhst = priv_data;
> +
> + /* Save registration status for use in ti_st_open() */
> + lhst->reg_status = data;
> + /* complete the wait in ti_st_open() */
> + complete(&lhst->wait_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> + struct ti_st *lhst = priv_data;
> + int err;
> +
> + if (!skb)
> + return -EFAULT;
> +
> + if (!lhst) {
> + kfree_skb(skb);
> + return -EFAULT;
> + }
> +
> + skb->dev = (void *) lhst->hdev;
> +
> + /* Forward skb to HCI core layer */
> + err = hci_recv_frame(skb);
> + if (err < 0) {
> + BT_ERR("Unable to push skb to HCI core(%d)", err);
> + return err;
> + }
> +
> + lhst->hdev->stat.byte_rx += skb->len;
> +
> + return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto = {
> + .type = ST_BT,
> + .recv = st_receive,
> + .reg_complete_cb = st_registration_completion_cb,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + unsigned long timeleft;
> + struct ti_st *hst;
> + int err;
> +
> + BT_DBG("%s %p", hdev->name, hdev);
> +
> + /* provide contexts for callbacks from ST */
> + hst = hdev->driver_data;
> + ti_st_proto.priv_data = hst;
> +
> + err = st_register(&ti_st_proto);
> + if (err == -EINPROGRESS) {
> + /* Prepare wait-for-completion handler data structures.
> + * Needed to synchronize this and
> + * st_registration_completion_cb() functions.
> + */
> + init_completion(&hst->wait_reg_completion);
> +
> + /* Reset ST registration callback status flag , this value
> + * will be updated in ti_st_registration_completion_cb()
> + * function whenever it called from ST driver.
> + */
> + hst->reg_status = -EINPROGRESS;
> +
> + /* ST is busy with either protocol registration or firmware
> + * download. Wait until the registration callback is called
> + */
> + BT_DBG(" waiting for registration completion signal from ST");
> +
> + timeleft = wait_for_completion_timeout
> + (&hst->wait_reg_completion,
> + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + if (!timeleft) {
> + BT_ERR("Timeout(%d sec),didn't get reg "
> + "completion signal from ST",
> + BT_REGISTER_TIMEOUT / 1000);
> + return -ETIMEDOUT;
> + }
> +
> + /* Is ST registration callback called with ERROR status? */
> + if (hst->reg_status != 0) {
> + BT_ERR("ST registration completed with invalid "
> + "status %d", hst->reg_status);
> + return -EAGAIN;
> + }
> + err = 0;
> + } else if (err == -EPERM) {
> + BT_ERR("st_register failed %d", err);
> + return err;
> + }
Again, you are checking for only EPERM and EINPROGRESS. You should check
for everything. I actually don't undertand why you have a special check
for EPERM.
> +
> + /* ti_st_proto.write is filled up by the underlying shared
> + * transport driver upon registration
> + */
> + hst->st_write = ti_st_proto.write;
I do not like that, the underlying should export the write function.
> + if (!hst->st_write) {
> + BT_ERR("undefined ST write function");
> +
> + /* Undo registration with ST */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> + return err;
> + }
> +
> + /* Registration with ST layer is successful,
> + * hardware is ready to accept commands from HCI core.
> + */
> + if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
> + clear_bit(HCI_RUNNING, &hdev->flags);
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> + hst->st_write = NULL;
> + }
What are you trying to do here? test_and_set_bit() result doesn't say
nothing about error and you shall put test_and_set_bit should be in the
beginning, to know if your device is already opened or not and then
clear_bit if some error ocurrs during the function.
> +
> + return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + int err;
> + struct ti_st *hst = hdev->driver_data;
> +
> + /* continue to unregister from transport */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> +
> + if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
> + return 0;
test_and_clear_bit should come first to check if your device is already
closed.
> +
> + return err;
> +}
> +
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + struct hci_dev *hdev;
> + struct ti_st *hst;
> + long len;
> +
> + hdev = (struct hci_dev *)skb->dev;
> +
> + if (!test_bit(HCI_RUNNING, &hdev->flags))
> + return -EBUSY;
> +
> + hst = hdev->driver_data;
> +
> + /* Prepend skb with frame type */
> + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> + skb->len);
> +
> + /* Insert skb to shared transport layer's transmit queue.
> + * Freeing skb memory is taken care in shared transport layer,
> + * so don't free skb memory here.
> + */
> + len = hst->st_write(skb);
> + if (len < 0) {
> + kfree_skb(skb);
> + BT_ERR(" ST write failed (%ld)", len);
> + return -EAGAIN;
Why EAGAIN?
> + }
> +
> + /* ST accepted our skb. So, Go ahead and do rest */
> + hdev->stat.byte_tx += len;
> + ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
>From Marcel in the other thread:
"What is the reason for this deferred stats update. That code looks
pretty much hackish to me."
> +
> + return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + BT_DBG("%s", hdev->name);
> +
> + /* free ti_st memory */
get ride of the comment, it's pointless
> + kfree(hdev->driver_data);
> +
> + return;
No return; here
> +}
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + static struct ti_st *hst;
> + struct hci_dev *hdev;
> + int err;
> +
> + hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + if (!hst)
> + return -ENOMEM;
> +
> + /* Expose "hciX" device to user space */
> + hdev = hci_alloc_dev();
> + if (!hdev)
> + return -ENOMEM;
You are leaking hst, if hci_alloc_dev() fails.
> +
> + BT_DBG("hdev %p", hdev);
> +
> + hst->hdev = hdev;
> + hdev->bus = HCI_UART;
> + hdev->driver_data = hst;
> + hdev->open = ti_st_open;
> + hdev->close = ti_st_close;
> + hdev->flush = NULL;
> + hdev->send = ti_st_send_frame;
> + hdev->destruct = ti_st_destruct;
> + hdev->owner = THIS_MODULE;
> +
> + err = hci_register_dev(hdev);
> + if (err < 0) {
> + BT_ERR("Can't register HCI device error %d", err);
> + hci_free_dev(hdev);
> + return err;
> + }
> +
> + BT_DBG(" HCI device registered (hdev %p)", hdev);
> +
> + dev_set_drvdata(&pdev->dev, hst);
> + return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + struct hci_dev *hdev;
> + struct ti_st *hst = dev_get_drvdata(&pdev->dev);
> +
> + if (!hst)
> + return -EFAULT;
> +
> + hdev = hst->hdev;
> + ti_st_close(hdev);
> + hci_unregister_dev(hdev);
> +
> + /* Free HCI device memory */
> + hci_free_dev(hdev);
> +
> + /* Free driver data memory */
get ride of both commnets here. The name of the funcion is already
saying that.
> + kfree(hst);
> +
> + dev_set_drvdata(&pdev->dev, NULL);
> + return 0;
> +}
> +
> +static struct platform_driver btwilink_driver = {
> + .probe = bt_ti_probe,
> + .remove = bt_ti_remove,
> + .driver = {
> + .name = "btwilink",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init btwilink_init(void)
> +{
> + long ret;
> +
> + BT_INFO(" Bluetooth Driver for TI WiLink - Version %s", VERSION);
> +
> + ret = platform_driver_register(&btwilink_driver);
> + if (ret != 0) {
> + BT_ERR("btwilink platform driver registration failed");
> + return ret;
> + }
> + return 0;
Old issue again:
>From Marcel: please just do like we do with all other drivers;
BT_INFO(...)
return platform_driver_register(&btwilink_driver);
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 3/3] Bluetooth: Automate remote name requests
From: Gustavo F. Padovan @ 2010-11-16 22:09 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth, Johan Hedberg
In-Reply-To: <1289401913-22982-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2010-11-10 17:11:53 +0200]:
> From: Johan Hedberg <johan.hedberg@nokia.com>
>
> In Bluetooth there are no automatic updates of remote device names when
> they get changed on the remote side. Instead, it is a good idea to do a
> manual name request when a new connection gets created (for whatever
> reason) since at this point it is very cheap (no costly baseband
> connection creation needed just for the sake of the name request).
>
> So far userspace has been responsible for this extra name request but
> tighter control is needed in order not to flood Bluetooth controllers
> with two many commands during connection creation. It has been shown
> that some controllers simply fail to function correctly if they get too
> many (almost) simultaneous commands during connection creation. The
> simplest way to acheive better control of these commands is to move
> their sending completely to the kernel side.
>
> This patch inserts name requests into the sequence of events that the
> kernel performs during connection creation. It does this after the
> remote features have been successfully requested and before any pending
> authentication requests are performed. The code will work sub-optimally
> with userspace versions that still do the name requesting themselves (it
> shouldn't break anything though) so it is recommended to combine this
> with a userspace software version that doesn't have automated name
> requests.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
> net/bluetooth/hci_event.c | 74 ++++++++++++++++++++++++++++++++++----------
> 1 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 45569f2..cef970f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -677,9 +677,8 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
> hci_dev_unlock(hdev);
> }
>
> -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
Can you add a hci_ in the beginning of your functions, just to keep the
coherency with the rest of the code.
> {
> - struct hci_cp_auth_requested cp;
> struct hci_conn *conn;
>
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> @@ -698,15 +697,43 @@ static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> conn->sec_level != BT_SECURITY_HIGH)
> return 0;
>
> - cp.handle = __cpu_to_le16(conn->handle);
> - hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> -
> return 1;
> }
>
> +static int request_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +{
> + struct hci_cp_auth_requested cp;
> + struct hci_conn *conn;
> +
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> + if (!conn)
> + return -ENOTCONN;
I'm not happy with have to lookup the hci_conn twice when we can do that
once here. I've noted that always outgoing_auth_needed() returns 1 you do
a request_auth, and always it returns 0 you don't, so I think we can
embed request_auth() inside outgoing_auth_needed() as it was in patch
2/3 and the give a better name to outgoing_auth_needed() which you
reflect the new behavior.
Regards,
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* Apple Wireless Keyboard connection issue
From: "André Kühne" @ 2010-11-16 21:27 UTC (permalink / raw)
To: linux-bluetooth
Hi everyone
I noticed the following on my system: After upgrading to bluez-4.79 connecting my Apple Wireless Keyboard does not work anymore. With bluez-4.77 the connection works just fine.
Please let me know if I could be of any help e.g. providing more detailed information.
Best regards
Andre
--
Neu: GMX De-Mail - Einfach wie E-Mail, sicher wie ein Brief!
Jetzt De-Mail-Adresse reservieren: http://portal.gmx.net/de/go/demail
^ permalink raw reply
* [RFC] Discover Primary Service by Service UUID
From: Claudio Takahasi @ 2010-11-16 21:24 UTC (permalink / raw)
To: linux-bluetooth
Hi all,
Before I send an official request-pull I need feedbacks about the
changes in the bt_string2uuid function.
Commit 8774e6c1523707b01efd892c665baeeabad41ab4 of my branch "gatt"
extends the bt_string2uuid to support UUID16, this modification
changes the behavior of the method DiscoverServices(string pattern) in
the device. Now, pattern can be also a hex string value instead of
UUID128 and "friendly" service names only. Is it acceptable?
This change will be required to implement Discover Primary Service by
Service UUID and Characteristic Value Read using UUID.
git request-pull info:
The following changes since commit
8ef71548686b3e9e2152aed46177e6bfca749b09:
Fix typos in adapter documentation (2010-11-16 13:39:43 +0000)
are available in the git repository at:
git://git.infradead.org/users/cktakahasi/bluez.git gatt
Bruna Moreira (1):
Implement Find by Type request encode/decoding
Claudio Takahasi (7):
Add Find By Type Value Response encoding/decoding functions
Implement Find by Type Value Request in the atttribute server
Extend bt_string2uuid to convert hex strings to UUID16
Add an extra parameter in the discovery primary to specify the
UUID
Add uuid command line option on gatttool
Implement Discover Primary Service by Service UUID in the
gatttool
Fix corner cases of Discover Primary Service by UUID in the
server
Makefile.am | 3 +-
attrib/att.c | 121
++++++++++++++++++++++++++++++++++++++++++++++++++-
attrib/att.h | 11 ++++-
attrib/client.c | 8 ++--
attrib/gatt.c | 37 +++++++++++++---
attrib/gatt.h | 4 +-
attrib/gatttool.c | 63 +++++++++++++++++++++++++-
src/attrib-server.c | 75 +++++++++++++++++++++++++++++++
src/glib-helper.c | 21 +++++++++
9 files changed, 323 insertions(+), 20 deletions(-)
Claudio.
^ 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