* Re: RFCOMM connection from android device to desktop pc
From: Leandro de Oliveira @ 2010-10-28 18:11 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <AANLkTinBqjTmLiXwd5LkzZRXOkuYkfS=48m6gWQ9AQO7@mail.gmail.com>
So, any ideas? Don't you find it strange that bluez can't communicate
with itself?
2010/10/22 Leandro de Oliveira <lehphyro@gmail.com>:
> Hi,
>
> I'm trying to open an RFCOMM connection from my android device to my
> desktop pc running Ubuntu 10.04, send some data, close the connection
> and then do it again. My desktop program is written in Java and relies
> on BlueCove[1] to handle bluetooth details which calls Bluez 4.x in
> this case.
>
> The problem is that I get a successful connection and data transfer in
> the first communication attempt but nothing in subsequent attempts.
>
> There is this thread[2] in the android-platform mailing list with much
> more detail, but I think you could just take a look at this hcidump
> that has been requested there to let me know if this is a problem in
> bluez on the android or desktop side:
> http://dl.dropbox.com/u/1401233/pc-hcidump.txt
>
> Please, let me know if you need more info.
>
> The same program works perfectly when the android application connects
> to it running on Windows 7 using the Microsoft Bluetooth Stack.
>
> [1] http://www.bluecove.org/
> [2] http://groups.google.com/group/android-platform/browse_thread/thread/485db57409304d3e?hl=en
>
> Thank you very much
>
^ permalink raw reply
* Re: [PATCH] Remove old hcid.conf
From: Johan Hedberg @ 2010-10-28 18:08 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <1288257675-17086-1-git-send-email-padovan@profusion.mobi>
Hi Gustavo,
On Thu, Oct 28, 2010, Gustavo F. Padovan wrote:
> ---
> src/hcid.conf | 57 -------------
> src/hcid.conf.5.in | 227 ----------------------------------------------------
> 2 files changed, 0 insertions(+), 284 deletions(-)
> delete mode 100644 src/hcid.conf
> delete mode 100644 src/hcid.conf.5.in
Thanks. The patch has been pushed upstream.
Johan
^ permalink raw reply
* Re: [PATCH v3] Fix obexd crash for empty listing invalid cache
From: Johan Hedberg @ 2010-10-28 17:51 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1288255408-3967-2-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Oct 28, 2010, Dmitriy Paliy wrote:
> This fixes obexd crash in 3-way calling scenario when filtered listing
> response is empty. Valid cache and empty pbap buffer mean that cache was
> already attempted to be created within a single session, but no data was
> available. Hence, it is not notified and no such file error returned.
>
> Such avoids clearing and creating a new cache operations for each incoming
> call, which is one of possible solution to fix this bug. It can be
> extensive for large phone books. New cache is not created within current
> obex session or unless path is changed.
> ---
> plugins/pbap.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: Unable to get on D-Bus: bluez 4.77
From: Puneet Pant @ 2010-10-28 17:18 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101028120950.GB17930@vigoh>
Hi Gustavo,
On Thu, Oct 28, 2010 at 5:39 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Puneet,
>
> * Puneet Pant <pant.puneet464@gmail.com> [2010-10-28 17:02:48 +0530]:
>
>> Hello,
>>
>> I have compiled Bluez 4.77. But I'm getting the following error when
>> trying to start Bluetooth Daemon:
>>
>> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src#
>> dbus-daemon --system
>> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src# ./bluetoothd -nd
>> bluetoothd[11819]: Bluetooth deamon 4.77
>> bluetoothd[11819]: src/main.c:parse_config() parsing main.conf
>> bluetoothd[11819]: src/main.c:parse_config() discovto=0
>> bluetoothd[11819]: src/main.c:parse_config() pairto=0
>> bluetoothd[11819]: src/main.c:parse_config() pageto=8192
>> bluetoothd[11819]: src/main.c:parse_config() name=%h-%d
>> bluetoothd[11819]: src/main.c:parse_config() class=0x000100
>> bluetoothd[11819]: src/main.c:parse_config() discov_interval=0
>> bluetoothd[11819]: src/main.c:parse_config() Key file does not have
>> key 'DeviceID'
>> bluetoothd[11819]: Unable to get on D-Bus
>
> One of the reasons could be that you already have the bluetoothd daemon
> running.
>
I did "ps -e | grep bluetooth" and found no bluetoothd running.
Could this be related to D-Bus library version incompatibility with ubuntu.
As I see, Ubuntu 10.10 comes with Dbus 1.4.0 installed, but when I run
./configure for bluez, it does not detect the installed D-Bus. I had
to manually download and install D-Bus 1.2.24 for bluez configure to
detect and compile?
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
>
Thanks,
Puneet
^ permalink raw reply
* [PATCH v2] Bluetooth: Automate remote name requests
From: johan.hedberg @ 2010-10-28 16:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Johan Hedberg
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>
---
v2 makes sure that the name request gets always sent instead of just
doing it for cases when we're also going to request authentication.
net/bluetooth/hci_event.c | 151 ++++++++++++++++++++++++++++++++-------------
1 files changed, 107 insertions(+), 44 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 84093b0..6e770e8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -677,9 +677,51 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
hci_dev_unlock(hdev);
}
+static void request_outgoing_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;
+
+ if (conn->state != BT_CONFIG || !conn->out)
+ return;
+
+ if (conn->sec_level == BT_SECURITY_SDP)
+ return;
+
+ /* Only request authentication for SSP connections or non-SSP
+ * devices with sec_level HIGH */
+ if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) &&
+ conn->sec_level != BT_SECURITY_HIGH)
+ return;
+
+ cp.handle = __cpu_to_le16(conn->handle);
+ hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
+}
+
static void hci_cs_remote_name_req(struct hci_dev *hdev, __u8 status)
{
+ struct hci_cp_remote_name_req *cp;
+
BT_DBG("%s status 0x%x", hdev->name, status);
+
+ /* If successful wait for the name req complete event before
+ * checking for the need to do authentication */
+ if (!status)
+ return;
+
+ cp = hci_sent_cmd_data(hdev, HCI_OP_REMOTE_NAME_REQ);
+ if (!cp)
+ return;
+
+ hci_dev_lock(hdev);
+
+ request_outgoing_auth(hdev, &cp->bdaddr);
+
+ hci_dev_unlock(hdev);
}
static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
@@ -1090,9 +1132,17 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
static inline void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb)
{
+ struct hci_ev_remote_name *ev = (void *) skb->data;
+
BT_DBG("%s", hdev->name);
hci_conn_check_pending(hdev);
+
+ hci_dev_lock(hdev);
+
+ request_outgoing_auth(hdev, &ev->bdaddr);
+
+ hci_dev_unlock(hdev);
}
static inline void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1162,33 +1212,39 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
- if (conn) {
- if (!ev->status)
- memcpy(conn->features, ev->features, 8);
+ if (!conn)
+ goto unlock;
- if (conn->state == BT_CONFIG) {
- if (!ev->status && lmp_ssp_capable(hdev) &&
- lmp_ssp_capable(conn)) {
- struct hci_cp_read_remote_ext_features cp;
- cp.handle = ev->handle;
- cp.page = 0x01;
- hci_send_cmd(hdev,
- HCI_OP_READ_REMOTE_EXT_FEATURES,
- sizeof(cp), &cp);
- } else if (!ev->status && conn->out &&
- conn->sec_level == BT_SECURITY_HIGH) {
- struct hci_cp_auth_requested cp;
- cp.handle = ev->handle;
- hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
+ if (!ev->status)
+ memcpy(conn->features, ev->features, 8);
+
+ if (conn->state != BT_CONFIG)
+ goto unlock;
+
+ if (!ev->status && lmp_ssp_capable(hdev) && lmp_ssp_capable(conn)) {
+ struct hci_cp_read_remote_ext_features cp;
+ cp.handle = ev->handle;
+ cp.page = 0x01;
+ hci_send_cmd(hdev, HCI_OP_READ_REMOTE_EXT_FEATURES,
sizeof(cp), &cp);
- } else {
- conn->state = BT_CONNECTED;
- hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
- }
- }
+ goto unlock;
+ }
+
+ if (!ev->status) {
+ struct hci_cp_remote_name_req cp;
+ memset(&cp, 0, sizeof(cp));
+ bacpy(&cp.bdaddr, &conn->dst);
+ cp.pscan_rep_mode = 0x02;
+ hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
+ }
+
+ if (!conn->out || conn->sec_level != BT_SECURITY_HIGH) {
+ conn->state = BT_CONNECTED;
+ hci_proto_connect_cfm(conn, ev->status);
+ hci_conn_put(conn);
}
+unlock:
hci_dev_unlock(hdev);
}
@@ -1646,32 +1702,39 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
- if (conn) {
- if (!ev->status && ev->page == 0x01) {
- struct inquiry_entry *ie;
+ if (!conn)
+ goto unlock;
- if ((ie = hci_inquiry_cache_lookup(hdev, &conn->dst)))
- ie->data.ssp_mode = (ev->features[0] & 0x01);
+ if (!ev->status && ev->page == 0x01) {
+ struct inquiry_entry *ie;
- conn->ssp_mode = (ev->features[0] & 0x01);
- }
+ if ((ie = hci_inquiry_cache_lookup(hdev, &conn->dst)))
+ ie->data.ssp_mode = (ev->features[0] & 0x01);
- if (conn->state == BT_CONFIG) {
- if (!ev->status && hdev->ssp_mode > 0 &&
- conn->ssp_mode > 0 && conn->out &&
- conn->sec_level != BT_SECURITY_SDP) {
- struct hci_cp_auth_requested cp;
- cp.handle = ev->handle;
- hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
- sizeof(cp), &cp);
- } else {
- conn->state = BT_CONNECTED;
- hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
- }
- }
+ conn->ssp_mode = (ev->features[0] & 0x01);
}
+ if (conn->state != BT_CONFIG)
+ goto unlock;
+
+ if (!ev->status) {
+ struct hci_cp_remote_name_req cp;
+ memset(&cp, 0, sizeof(cp));
+ bacpy(&cp.bdaddr, &conn->dst);
+ cp.pscan_rep_mode = 0x02;
+ hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
+ }
+
+ /* If legacy pairing, or an incoming connection, or SDP security
+ * consider ourself connected */
+ if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) || !conn->out ||
+ conn->sec_level == BT_SECURITY_SDP) {
+ conn->state = BT_CONNECTED;
+ hci_proto_connect_cfm(conn, ev->status);
+ hci_conn_put(conn);
+ }
+
+unlock:
hci_dev_unlock(hdev);
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Automate remote name requests
From: Johan Hedberg @ 2010-10-28 13:56 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikKGDtybssKjoNUACAwzg1HA7VC4rWrnNFvJ-2u@mail.gmail.com>
Hi Luiz,
On Thu, Oct 28, 2010, Luiz Augusto von Dentz wrote:
> This seems to be done only for some connections which IMO is not the
> correct, so for instance it wouldn't request name for SDP connections
> nor for incoming connections, is there a reason for that?
>
> Perhaps requesting the name regardless the security level on
> hci_remote_features_evt and then latter, when we got the name
> response, we continue with security level check and authentication
> request if necessary. Wouldn't that work better?
Yeah, you're right. Basicly what the patch does is replace the
occurences of AUTH_REQ with NAME_REQ and then do the AUTH_REQ in the
name_req_complete callback. However that is not the same as userspace is
doing which is an unconditional name request for every single connect
complete event. FWIW, there seems to be a potential bug with the
existing authentication request too: the code doesn't trigger it if the
extended features request fails (hci_cs_read_remote_ext_features
function).
I'll try to come up with a new revision of the name request patch
shortly.
Johan
^ permalink raw reply
* Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session
From: Gustavo F. Padovan @ 2010-10-28 13:07 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, ville.tervo
In-Reply-To: <1282215970-32758-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2010-08-19 14:06:10 +0300]:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> ---
> net/bluetooth/rfcomm/core.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
Patch has been applied to the bluetooth-2.6 tree. Thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-28 12:22 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTik-S50ZX6mFG+vWhBv0x_WmipP=t+4VqmAgssRC@mail.gmail.com>
On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
> >
> > So - NAK this for the moment, it needs to be split cleanly into ldisc
> > (thing which speaks the protocol and eg sees "speed change required" and
> > acts on it) and device (thing which knows about the hardware).
>
> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?
After getting a better idea of what the base mfd driver does, my impression
is now that you should not register a second N_HCI line discipline at all,
but instead extend the existing line discipline with this number.
I'm not sure what happens if you need two modules that try to register
the same ldisc number, but I imagine it is not good.
Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
Arnd
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-28 12:18 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTin72z7AmfkMMH01siwkNNNvHM+1+qXLodmaUx5q@mail.gmail.com>
On Thursday 28 October 2010, Par-Gunnar Hjalmdahl wrote:
> Thanks for this explanation. I would like to see this as a MFD at the
> moment since it is most definitely a silicon, as stated by Linus
> Walleij in a separate answer.
> In the future this could however be extended to be a bus, when we
> could use more chips with this driver.
Ok, fair enough. Time will tell if you need this as a bus then,
or alternatively use multiple multi-function drivers, perhaps
with some shared code in form of another "library" module.
The only important part is that you do not introduce user-visible
interfaces that can not be changed if you decide to rewrite the
implementation.
> > Does that mean that cg2900 is to the host side a standard bluetooth HCI,
> > but it uses extensions to the HCI in order to make the other functions
> > available to the host?
> >
> > What chapter in the bluetooth core specification describes this?
> >
>
> Yes, exactly. Just for Bluetooth functionality you could more or less
> use existing Bluetooth UART drivers (you still have to enable chip and
> so on separately though), but in order to reach FM and GPS this
> protocol has been extended. In order to have an energy efficient
> solution there must also be a common driver that can shut off the chip
> when not in use, hence this CG2900 driver.
> You can read about the original protocol in the Bluetooth Core
> specification Volume 4 Part A - UART Transport Layer.
> This way to extend functionality upon the existing protocol is common
> amongst chip manufacturers for combo chips including Bluetooth
> functionality. Texas Instruments has already posted a driver for their
> chip (in drivers/staging/ti-st) that uses a similar protocol.
Hmm, this sounds like it's at least one level below where I expected
it. So your cg2900 is connected through a UART and behaves like a serial
bluetooth host, right?
If that's the case, I think what you should have done is to make the
low-level interface available to Linux as a tty/serial driver,
which gets switched to the HCI line discipline.
The code for this is in drivers/bluetooth/hci_ldisc.c.
On top of this sits the hci_h4 protocol driver, drivers/bluetooth/hci_h4.c
and you additional functionality would have to hook into that.
Is this a setup you have considered? Any particular reasons why you
decided against it? Or are you actually doing exactly this and I'm
just too blind to see it?
> Oops, I misunderstood your question. The name is supplied by the user
> calling cg2900_register_user. The names are defined for reference in
> include/linux/mfd/cg2900.h.
> We used to have an enum to define this, but since we thought that
> using name lookup to be more future safe (and as well more "Kernel
> like") we chose to use that instead. It can have been a bad choice.
> Problem here is to some extent that we are beginners at Linux coding.
We generally use strings for stuff that comes from hardware or
user and needs to be extensible, like the driver names.
In your case, there is no such requirement, so an enum would probably
have been much simpler.
> A straight minimal API seemed to be easiest way to solve it. As you
> say we could probably use device registration as you state (we are
> already using MFD cells so we could possibly extend that) but I don't
> know if that would improve our code.
Using mfd cells is reasonable, but you really should not try to duplicate
infrastructure code by introducing another layer on top.
I'm not familiar with MFD, but it's clear that your code took a wrong
turn in some place when you had to do that.
>From what I can tell, the slave drivers should only need to register
a platform_driver that binds to the mfd cells created here, but
the base driver should not at all need to interact with the slaves
otherwise.
> We also have a lot of internal dependencies (like our FM driver that
> will be submitted to the Kernel community) so would like to avoid
> changing APIs if possible. But if you make this an absolute demand I
> guess we will have to follow.
Changing internal APIs is not an issue, we do that all the time and you
really need not be afraid of it.
The thing we don't do is to change the user-visible kernel ABI!
I'm not asking to specifically change the string into an enum, the
real question is if you use the right abstraction, and the fact that
you had to choose an identifier like this hints that you are not.
This issue probably disappears once the question of the MFD integration
is solved.
> What is transmitted over each char dev is individual for that channel.
> For BT channels this will be BT data (normally char dev is not used
> for Bluetooth since BT stack connection is done directly in Kernel
> through btcg2900.c), for FM channel this will be FM data, etc. But
> most of all it will not be the same format as the sockets provided by
> the Bluetooth stack (at least I think so) since BT stack have
> protocols below the sockets (such as L2CAP). What is sent over the
> char dev is the same data as sent to the chip minus the H4 header (the
> first byte).
I see.
One point where I think the abstraction went wrong is that you have
a matrix of drivers and channels: The chardev is registered as
an MFD cell and the driver binds to that cell, creating a device
node for each channel (BT_CMD, FM_RADIO, ...).
What you should have instead is a single MFD cell for each channel
and let each channel bind to one driver!
The channel is the primary differentiator between functionality
on the CG2900, so that is what becomes your cell. Since you added
another abstraction on top, you had to duplicate core functionality
of the kernel, which is what I initially saw as "this looks wrong,
no idea what's going on".
> The CG2900 driver is currently written only to support one CG2900
> chip, but of course that could be extended in the future.
Ok. So if you use the MFD framework correctly, future chips
will require a new base driver but the slaves that you already
have drivers for will be able to use their drivers without modification,
right?
Arnd
^ permalink raw reply
* Re: Unable to get on D-Bus: bluez 4.77
From: Gustavo F. Padovan @ 2010-10-28 12:09 UTC (permalink / raw)
To: Puneet Pant; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimTzSMQW3cJQPphWgTBKgn0fbN7zui7tRA1enmi@mail.gmail.com>
Hi Puneet,
* Puneet Pant <pant.puneet464@gmail.com> [2010-10-28 17:02:48 +0530]:
> Hello,
>
> I have compiled Bluez 4.77. But I'm getting the following error when
> trying to start Bluetooth Daemon:
>
> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src#
> dbus-daemon --system
> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src# ./bluetoothd -nd
> bluetoothd[11819]: Bluetooth deamon 4.77
> bluetoothd[11819]: src/main.c:parse_config() parsing main.conf
> bluetoothd[11819]: src/main.c:parse_config() discovto=0
> bluetoothd[11819]: src/main.c:parse_config() pairto=0
> bluetoothd[11819]: src/main.c:parse_config() pageto=8192
> bluetoothd[11819]: src/main.c:parse_config() name=%h-%d
> bluetoothd[11819]: src/main.c:parse_config() class=0x000100
> bluetoothd[11819]: src/main.c:parse_config() discov_interval=0
> bluetoothd[11819]: src/main.c:parse_config() Key file does not have
> key 'DeviceID'
> bluetoothd[11819]: Unable to get on D-Bus
One of the reasons could be that you already have the bluetoothd daemon
running.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Unable to get on D-Bus: bluez 4.77
From: Puneet Pant @ 2010-10-28 11:32 UTC (permalink / raw)
To: linux-bluetooth
Hello,
I have compiled Bluez 4.77. But I'm getting the following error when
trying to start Bluetooth Daemon:
root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src#
dbus-daemon --system
root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src# ./bluetoothd -nd
bluetoothd[11819]: Bluetooth deamon 4.77
bluetoothd[11819]: src/main.c:parse_config() parsing main.conf
bluetoothd[11819]: src/main.c:parse_config() discovto=0
bluetoothd[11819]: src/main.c:parse_config() pairto=0
bluetoothd[11819]: src/main.c:parse_config() pageto=8192
bluetoothd[11819]: src/main.c:parse_config() name=%h-%d
bluetoothd[11819]: src/main.c:parse_config() class=0x000100
bluetoothd[11819]: src/main.c:parse_config() discov_interval=0
bluetoothd[11819]: src/main.c:parse_config() Key file does not have
key 'DeviceID'
bluetoothd[11819]: Unable to get on D-Bus
I've built kernel version 2.6.36-rc7 with LE patches from Ville Tervo
and am running it on an Ubuntu 10.10 distribution. Installed Dbus
Library version is 1.2.24 (I have also tried with 1.4.0, but get same
error)
I have tried to google around on this but couldn't find anything
relevant that could fix this issue.
Can someone kindly suggest if this is related to my linux pc
configuration or more of Bluez issue?
Thanks in advance.
Puneet
^ permalink raw reply
* Re: [PATCH 2/9] mfd: Add char devices for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 11:06 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <201010221749.37928.arnd@arndb.de>
Hi Arnd,
Thanks for your comments. Everyone appreciates the time and effort
you've put into the review of this code.
See below for answer.
/P-G
2010/10/22 Arnd Bergmann <arnd@arndb.de>:
> On Friday 22 October 2010 12:36:09 Par-Gunnar Hjalmdahl wrote:
>> This patch adds char devices to the ST-Ericsson CG2900 driver.
>> The reason for this is to allow users of CG2900, such as GPS, to
>> be placed in user space.
>
> Can you be more specific how you expect this to be used?
>
> I guess you have hardware units that then each correspond to
> one character device and can be talked to over a pseudo socket
> interface, right?
>
> For most devices (radio, audio, bluetooth, gps, ...), we already
> have existing user interfaces, so how do they interact with this
> one?
>
Char dev exposes the interfaces to user space. Each char dev should
correspond to a channel in the cg2900.h API. The API allows one user
for each channel and this user can either be a Kernel user through a
direct call or a User space user through opening the corresponding
char dev.
Our reference system have the following users:
BT (cmd, acl, and evt) through Kernel BT stack (/net/bluetooth).
Connected by btcg2900 in this patch set
FM through V2L2 radio device (/drivers/media/radio). Connected by
CG2900 FM driver for V2L2 (under development, will be submitted to
community).
GPS through stack located in User space
We do not however want to block anyone from using a different
combination, e.g. using different BT or FM stack (placed in Kernel or
User space). The CG2900 driver is only intended to present a reliable
and power efficient transport to the different features within the
chip.
Normal flow for a user space stack is:
User space user opens char dev -> char dev registers corresponding
channel user using cg2900_register_user
User space user can now send and receive data to the chip using write,
read, and poll. Data is in raw format, cg2900 only adds first byte (H4
header) to packet.
When finished, User space user closes char dev -> char dev unregisters
channel using cg2900_deregister_user
>> +#define NAME "CharDev "
>
> ?
>
>> +/* Ioctls */
>> +#define CG2900_CHAR_DEV_IOCTL_RESET _IOW('U', 210, int)
>> +#define CG2900_CHAR_DEV_IOCTL_CHECK4RESET _IOR('U', 212, int)
>> +#define CG2900_CHAR_DEV_IOCTL_GET_REVISION _IOR('U', 213, int)
>> +#define CG2900_CHAR_DEV_IOCTL_GET_SUB_VER _IOR('U', 214, int)
>
> These definitions look wrong -- you never use the ioctl argument...
>
Yes, we return values in the error which is probably not a good
solution. We should probably use argument instead. But I don't see
what's wrong with the defines (but most likely I've misunderstood how
they should be used :-) )?
>> + *
>> + * Returns:
>> + * Bytes successfully read (could be 0).
>> + * -EBADF if NULL pointer was supplied in private data.
>> + * -EFAULT if copy_to_user fails.
>> + * Error codes from wait_event_interruptible.
>> + */
>> +static ssize_t char_dev_read(struct file *filp, char __user *buf, size_t count,
>> + loff_t *f_pos)
>
> The same comment applies here that I made for the test interface:
> Why is this not an AF_BLUETOOTH socket instead of a chardev?
>
> Unless I'm mistaken, you actually send bluetooth frames after all.
>
I've discussed this in another mail, but to repeat in short:
For the BT channels we send raw BT data, but it is not on the same
level as for example the L2CAP socket in the BT stack. So I think it
would not apply, It would only be confusing to have the same kind of
socket on different stack levels (plus it does not apply for FM and
GPS).
>> + case CG2900_CHAR_DEV_IOCTL_RESET:
>> + if (!dev) {
>> + err = -EBADF;
>> + goto error_handling;
>> + }
>> + CG2900_INFO("ioctl reset command for device %s", dev->name);
>> + err = cg2900_reset(dev->dev);
>> + break;
>> +
>> + case CG2900_CHAR_DEV_IOCTL_CHECK4RESET:
>> + if (!dev) {
>> + CG2900_INFO("ioctl check for reset command for device");
>> + /* Return positive value if closed */
>> + err = CG2900_CHAR_DEV_IOCTL_EVENT_CLOSED;
>> + } else if (dev->reset_state == CG2900_CHAR_RESET) {
>> + CG2900_INFO("ioctl check for reset command for device "
>> + "%s", dev->name);
>> + /* Return positive value if reset */
>> + err = CG2900_CHAR_DEV_IOCTL_EVENT_RESET;
>> + }
>> + break;
>
> Strange interface. Why do you need to check for the reset?
>
Any user (Kernel or User space) can reset the chip (this is a HW
reset, not a SW reset). If someone does this, all settings made to the
chip are lost and chip is turned off. We can however not signal up to
User space that it now has to do a close and re-open operation plus
restart its stack (possibly informing the end-user about the restart).
By using poll and ioctl we can do this without disturbing other
operations. If User space application tries to perform operation on a
reset device, e.g. write, this will fail, but using ioctl provides the
opportunity to do this without any fake read or write operations.
>> + case CG2900_CHAR_DEV_IOCTL_GET_REVISION:
>> + CG2900_INFO("ioctl check for local revision info");
>> + if (cg2900_get_local_revision(&rev_data)) {
>> + CG2900_DBG("Read revision data revision %d "
>> + "sub_version %d",
>> + rev_data.revision, rev_data.sub_version);
>> + err = rev_data.revision;
>> + } else {
>> + CG2900_DBG("No revision data available");
>> + err = -EIO;
>> + }
>> + break;
>> +
>> + case CG2900_CHAR_DEV_IOCTL_GET_SUB_VER:
>> + CG2900_INFO("ioctl check for local sub-version info");
>> + if (cg2900_get_local_revision(&rev_data)) {
>> + CG2900_DBG("Read revision data revision %d "
>> + "sub_version %d",
>> + rev_data.revision, rev_data.sub_version);
>> + err = rev_data.sub_version;
>> + } else {
>> + CG2900_DBG("No revision data available");
>> + err = -EIO;
>> + }
>> + break;
>
> These look like could better live in a sysfs attribute of the platform device.
>
> Arnd
>
I guess so. I thought it would be more simple to have it in the ioctl
since I already have other ioctl parameters, but I can probably fix it
quite easily.
/P-G
^ permalink raw reply
* [PATCH] bluetooth: Fix NULL pointer dereference issue
From: Yuri Ershov @ 2010-10-28 10:52 UTC (permalink / raw)
To: marcel, davem, padovan, jprvita
Cc: linux-bluetooth, ville.tervo, andrei.emeltchenko, Yuri Ershov
This patch fixes NULL pointer dereference at running test with
connect-transfer-disconnect in loop. The problem conditions are the
following: there are 2 BT devices. The first one listens and
receives (l2test -r), the second one makes "connect-disconnect-
connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
After some time this will cause the race between functions
bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
sets the socket state to BT_CLOSED, unlinks and kills the socket
in the middle of bt_accept_dequeue, then at running the removed code
kernel oops appears.
Signed-off-by: Yuri Ershov <ext-yuri.ershov@nokia.com>
---
net/bluetooth/af_bluetooth.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..47c107e 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -204,13 +204,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
lock_sock(sk);
- /* FIXME: Is this check still needed */
- if (sk->sk_state == BT_CLOSED) {
- release_sock(sk);
- bt_accept_unlink(sk);
- continue;
- }
-
if (sk->sk_state == BT_CONNECTED || !newsock ||
bt_sk(parent)->defer_setup) {
bt_accept_unlink(sk);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 10:37 UTC (permalink / raw)
To: Alan Cox; +Cc: linus.walleij, linux-bluetooth, linux-kernel, Lukasz.Rymanowski
In-Reply-To: <20101022163359.5b22a61b@lxorguk.ukuu.org.uk>
Thanks again for your comments Alan.
Next patch set will contain resolution to all your comments. See below.
/P-G
2010/10/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> The existence of the callback is checked in the function
>> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
>> sleep and this code will never run.
>
> OK yes see this now.
>
>> >> + CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>> >
>> > Please use the standard dev_dbg etc functionality - or pr_err etc when
>> > you have no device pointer. The newest kernel tty object has a device
>> > pointer added so you could use that.
>> >
>>
>> OK. I like the debug system we have now (using module parameter to set
>> debug level in runtime), but I've received comments regarding this
>> before so I assume we will have to switch to standard printouts.
>> Is it OK to use defines or inline functions to add "CG2900" before and
>> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
> The pr_fmt bit will do what you want for a non dev_dbg type thing. See
> include/linux/kernel.h
>
OK. Added. I'm however using dev_err, dev_dbg, etc where possible.
>> >> + * unset_cts_irq() - Disable interrupt on CTS.
>> >> + */
>> >> +static void unset_cts_irq(void)
>> >
>> > And no ability to support multiple devices
>> >
>>
>> OK. We will try to fix this.
>
> That may go away when you clean up the device side. The line discipline
> can be attached to any device so must be multi-device aware, the hardware
> driver can certainly be single device only if you can only ever have one
>
> [Although its a good idea to design it so it can be fixed because you
> never know when you'll find your sales team just sold someone a two
> device solution ;) ]
>
OK. Design still ongoing, but will be fixed in some way.
>> >> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> >> + len = tty->ops->write(tty, skb->data, skb->len);
>> >
>> > A tty isn't required to have a write function
>>
>> I don't quite understand your comment here. This particular code is
>> inspired of the Bluetooth ldisc code and there it is used like. It
>> seems to work fine so we do it the same way.
>
> You copied a security hole from the bluetooth driver which we found a
> couple of weeks ago
>
>> How would you prefer it to be?
>
> Check it is valid, in your case probably on open just refuse to attach to
> a read only port.
>
OK. Done.
>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> Works for me - there is an ongoing "we must move tty ldiscs and core tty
> code somewhere more sensible of their own" discussion, so if it is
> dropped into char, it'll move with them just fine.
>
> Alan
>
Again, thanks for the comments.
/P-G
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 10:32 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <201010261406.10996.arnd@arndb.de>
Thanks again for your comments Arnd.
Sorry for not answering your mail earlier but I've spent the last few
days changing a few hundred debug comments. :-)
See answers below.
/P-G
2010/10/26 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday 26 October 2010, Par-Gunnar Hjalmdahl wrote:
>
>> 2010/10/22 Arnd Bergmann <arnd@arndb.de>:
>> > On Friday 22 October 2010 12:35:16 Par-Gunnar Hjalmdahl wrote:
>> >> This patch adds support for the ST-Ericsson CG2900 Connectivity
>> >> Combo controller.
>> >> This patch adds the central framework to be able to register CG2900 users,
>> >> transports, and chip handlers.
>> >>
>> >> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
>> >
>> > Looks ok from a coding style perspective, but some important information is
>> > missing from the description:
>> >
>> > * What is a CG2900?
>> > * Why is it a MFD device rather than e.g. a bus or a subsystem?
>> >
>>
>> I will add some more info:
>> - CG2900 is a GPS, Bluetooth, FM controller
>> - I do not really know what makes a device qualify as a certain type,
>> but at least for us this is a multifunction device. But I can't really
>> say that it isn't really a bus (could be stated as multifunction HCI
>> bus). I guess it could be qualified as a subsystem as well, but I
>> cannot give you a reason to have it as either.
>
> It's not often completely clear, but I would draw the distinction like
> this:
>
> * A multi-function device is a single device that sits on a given bus
> with one host I/O interface and provides functionality to different
> logical subsystems like serial or alsa. A typical case for an MFD would
> be a to solve the problem of sharing resources between the child drivers
> because you cannot bind one device to two drivers. If CG2900 is a single
> piece of silicon that always looks the same way, it's probably an MFD.
>
> * A bus is a much more general abstraction which has multiple devices
> and multiple device types behind it. The bus has no idea what devices
> are attached to it at compile time, so you either probe the devices at
> boot time or define a set of devices in a board definition (platform
> devices). Driver modules then register a struct device_driver for the
> bus, which gives all matching devices to the bus. If you can have
> future CG2900 compatible devices that need to have other drivers, e.g.
> for HID or video, it should probably be a bus.
>
> * A subsystem is less clearly defined, I would call bluetooth a subsystem
> instead of just a bus because it not only matches devices with drivers
> but also has its own user-level interfaces for raw communication.
> If you want to be able to have drivers in the kernel and in user space,
> you might want to consider starting a new drivers/cg2900 subsystem, or
> integrating into the bluetooth subsystem if that makes sense.
>
Thanks for this explanation. I would like to see this as a MFD at the
moment since it is most definitely a silicon, as stated by Linus
Walleij in a separate answer.
In the future this could however be extended to be a bus, when we
could use more chips with this driver.
>> >> +config MFD_CG2900
>> >> + tristate "Support ST-Ericsson CG2900 main structure"
>> >> + depends on NET
>> >> + help
>> >> + Support for ST-Ericsson CG2900 Connectivity Combo controller main structure.
>> >> + Supports multiple functionalities muxed over a Bluetooth HCI H:4 interface.
>> >> + CG2900 support Bluetooth, FM radio, and GPS.
>> >> +
>> >
>> > Can you explain what it means to mux over a H:4 interface? Does this mean
>> > you use bluetooth infrastructure that is designed for wireless communication
>> > in order to connect on-board or on-chip devices?
>> >
>>
>> The H:4 protocol is defined in the Bluetooth Core specification. It
>> uses a single byte first in the data packet to determine an HCI
>> channel. In the Bluetooth Core specification there are 4 channels
>> specified, 0x01-0x04 (0x01 is BT command, etc). The rest of the
>> channels are reserved, but these have been used on a proprietary basis
>> to transport data to different IPs within the controller. In our API
>> we have chosen to have a channel separation on an API basis and the
>> H:4 byte is then added within the driver. So we have multiple channels
>> coming in from the users that we mux onto a single data transport.
>> That's what I mean in the text. I guess I could rewrite it a bit to
>> make it clearer.
>
> I still don't understand it, though that may be a problem on my side.
>
> Does that mean that cg2900 is to the host side a standard bluetooth HCI,
> but it uses extensions to the HCI in order to make the other functions
> available to the host?
>
> What chapter in the bluetooth core specification describes this?
>
Yes, exactly. Just for Bluetooth functionality you could more or less
use existing Bluetooth UART drivers (you still have to enable chip and
so on separately though), but in order to reach FM and GPS this
protocol has been extended. In order to have an energy efficient
solution there must also be a common driver that can shut off the chip
when not in use, hence this CG2900 driver.
You can read about the original protocol in the Bluetooth Core
specification Volume 4 Part A - UART Transport Layer.
This way to extend functionality upon the existing protocol is common
amongst chip manufacturers for combo chips including Bluetooth
functionality. Texas Instruments has already posted a driver for their
chip (in drivers/staging/ti-st) that uses a similar protocol.
>> >> +
>> >> +/**
>> >> + * add_h4_user() - Add H4 user to user storage based on supplied H4 channel.
>> >> + * @dev: Stored CG2900 device.
>> >> + * @name: Device name to identify different devices that are using
>> >> + * the same H4 channel.
>> >> + *
>> >> + * Returns:
>> >> + * 0 if there is no error.
>> >> + * -EINVAL if NULL pointer or bad channel is supplied.
>> >> + * -EBUSY if there already is a user for supplied channel.
>> >> + */
>> >> +static int add_h4_user(struct cg2900_device *dev, const char * const name)
>> >> +{
>> >
>> > Where does that name come from? Why not just use an enum if the name is
>> > only use for strncmp?
>> >
>>
>> At a point in time we used to have an enum, but from what I can see it
>> is easier to keep an API stable if you use name lookup instead. If we
>> want to have minimal changes to the API in the future this is quite a
>> flexible solution, but this code should be moved to each respective
>> chip handler.
>
> You haven't really answered the first question, where the name comes from
> (I guess the question was not too clear). Does this e.g. get passed by a
> user application, or does it come from a hardware description of some
> sort?
>
> It looks a bit like a handcoded version of the code we use for device/driver
> matching in drivers/core. If that's the case, it would be better to use
> the existing infrastructure and create a bus with devices that can be
> matched with drivers.
>
Oops, I misunderstood your question. The name is supplied by the user
calling cg2900_register_user. The names are defined for reference in
include/linux/mfd/cg2900.h.
We used to have an enum to define this, but since we thought that
using name lookup to be more future safe (and as well more "Kernel
like") we chose to use that instead. It can have been a bad choice.
Problem here is to some extent that we are beginners at Linux coding.
A straight minimal API seemed to be easiest way to solve it. As you
say we could probably use device registration as you state (we are
already using MFD cells so we could possibly extend that) but I don't
know if that would improve our code.
We also have a lot of internal dependencies (like our FM driver that
will be submitted to the Kernel community) so would like to avoid
changing APIs if possible. But if you make this an absolute demand I
guess we will have to follow.
>> >> +static ssize_t test_char_dev_read(struct file *filp, char __user *buf,
>> >> + size_t count, loff_t *f_pos)
>> >> +{
>> >> + struct sk_buff *skb;
>> >> + int bytes_to_copy;
>> >> + int err;
>> >> + struct sk_buff_head *rx_queue = &core_info->test_char_dev->rx_queue;
>> >
>> > What is this read/write interface used for?
>> >
>> > The name suggests that this is only for testing and not an essential
>> > part of your driver. Could this be made a separate driver?
>> >
>> > It also looks like you do socket communication here, can't you use
>> > a proper AF_BLUETOOTH socket to do the same?
>> >
>>
>> This interface can be used for module testing where you can have a
>> test engine in user space that acts as a chip. Yes, it could be made
>> into a separate driver but it would be a very small driver. Don't know
>> if it will be worth having it in a separate file...
>
> I don't think file size is a major argument here. If it's used for
> testing, I'd make it a separate module that defaults to not being
> built, in order to prevent people from relying on what you intended
> as a testing interface.
>
OK. We can do this.
>> We use the sk_buffer, which I guess is the reason that you mention
>> sockets. We could of course use a socket instead of a char dev, both
>> here and in the cg2900_char_dev file (which of course then would be
>> renamed). It was just a choice we made during development. We think
>> that the transport as such is more like a streaming interface, but I
>> see no real problem to have sockets. We already have several users
>> using char dev though so I would prefer to keep char dev rather than
>> switching to sockets unless you have a strong reason for this.
>> I see no reason to use AF_BLUETOOTH though, the transport is not only
>> for Bluetooth.
>
> The question here is what is appropriate. My guess was that the frame
> format was the one used in bluetooth sockets. If that's not the
> case, AF_BLUETOOTH would obviously be wrong.
>
> For the test interface, it doesn't matter that much what you use, but
> for the cg2900_char_dev we should make sure that you considered all
> the options and made a reasonable decision. What does the data format
> look like there? My impression is that the character device might be
> too low-level and it could be better to use some structured interface
> like a socket, but it really depends on what it actually does.
>
> In particular, if you can address multiple hardware units over one
> character device, it may be better to have separate devices for each
> of them.
>
What is transmitted over each char dev is individual for that channel.
For BT channels this will be BT data (normally char dev is not used
for Bluetooth since BT stack connection is done directly in Kernel
through btcg2900.c), for FM channel this will be FM data, etc. But
most of all it will not be the same format as the sockets provided by
the Bluetooth stack (at least I think so) since BT stack have
protocols below the sockets (such as L2CAP). What is sent over the
char dev is the same data as sent to the chip minus the H4 header (the
first byte).
The CG2900 driver is currently written only to support one CG2900
chip, but of course that could be extended in the future.
>> >> + CG2900_INFO("test_char_dev_read");
>> >> +
>> >> + if (skb_queue_empty(rx_queue))
>> >> + wait_event_interruptible(char_wait_queue,
>> >> + !(skb_queue_empty(rx_queue)));
>> >> +
>> >> + skb = skb_dequeue(rx_queue);
>> >> + if (!skb) {
>> >> + CG2900_INFO("skb queue is empty - return with zero bytes");
>> >> + bytes_to_copy = 0;
>> >> + goto finished;
>> >> + }
>> >> +
>> >> + bytes_to_copy = min(count, skb->len);
>> >> + err = copy_to_user(buf, skb->data, bytes_to_copy);
>> >> + if (err) {
>> >> + skb_queue_head(rx_queue, skb);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + skb_pull(skb, bytes_to_copy);
>> >> +
>> >> + if (skb->len > 0)
>> >> + skb_queue_head(rx_queue, skb);
>> >> + else
>> >> + kfree_skb(skb);
>> >> +
>> >> +finished:
>> >> + return bytes_to_copy;
>> >> +}
>> >
>> > This does not handle short/continued reads.
>> >
>>
>> As I mentioned above this interface is used for testing and we
>> therefore have some restriction of usage. I don't think we need to
>> impose all things necessary for a full interface upon it.
>
> This is where it gets complicated. We don't normally add interfaces
> to the kernel unless we expect to fully support them for the forseeable
> future. It has happened more than enough in the past that somebody
> introduced a debugging interface which subsequently became a supported
> ABI because some application started relying on it.
>
> Is the code that uses the test interface even publically available?
> If not, I would recommend saving the trouble of arguing about it
> and leaving out the interface.
>
OK. Good idea. I will split out the test code to a separate file and
leave it out of next patch set.
>> >> + #define CG2900_ERR(fmt, arg...) \
>> >> + do { \
>> >> + if (cg2900_debug_level >= 1) \
>> >> + pr_err("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
>> >> + } while (0)
>> >> +
>> >> +#endif /* NDEBUG */
>> >
>> > You'll feel relieved when all this is gone...
>> >
>>
>> The only thing is it's been pretty nice to have it, but I will remove it.
>> Is it OK to keep defines so we can have "CG2900" in front and "\n"
>> after the text?
>
> Ideally, you would use the dev_* functions instead of the pr_* functions,
> which print the name of the device before the message. I also wouldn't
> add the "\n" in the macro because all the regular printing functions don't
> do it. It will likely only confuse people and the binary remains the same.
>
> Arnd
>
OK. I've updated the code (will come in next patch set). I use dev_dbg
where possible.
/P-G
^ permalink raw reply
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
From: Gustavo F. Padovan @ 2010-10-28 9:58 UTC (permalink / raw)
To: Yuri Ershov
Cc: marcel, davem, jprvita, linux-bluetooth, ville.tervo,
andrei.emeltchenko
In-Reply-To: <4CC576D3.2090304@nokia.com>
Hi Yuri,
Please no top posting in this mainling list. It's not allowed, thanks.
* Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-25 16:23:47 +0400]:
> Hello Gustavo,
>
> The problem appears in case of multiple connect-transfer-disconnect
> sequence (e.g. by using l2test). The conditions are the following:
> There are 2 BT devices. The first one listens and receives (l2test -r),
> the second one makes "connect-disconnect-connect..." sequence (l2test -c
> -b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race
> between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:
>
> struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
> {
> ...
> list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
> sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
>
> lock_sock(sk);
>
>
>
> In this time the function l2cap_chan_del sets the socket state to
> BT_CLOSED, unlinks and kills the socket.
>
>
>
> /* FIXME: Is this check still needed */
> if (sk->sk_state == BT_CLOSED) {
> release_sock(sk);
> bt_accept_unlink(sk);
> continue;
> }
>
> ...
>
> release_sock(sk);
> }
> return NULL;
> }
I agree with you, just add this info to your commit message and then
resend your patch so I can apply it.
> ext Gustavo F. Padovan wrote:
> > Hi Yuri,
> >
> > * Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-21 20:08:58 +0400]:
> >
> >
> >> This patch fixes NULL pointer dereference at running test with
> >> connect-transfer-disconnect in loop. Sometimes sk_state is
> >> BT_CLOSED and sk_refcnt equal to 0, so there is oops in
> >> bt_accept_unlink. In normal case removed block is not used.
> >>
> >
> > Question here is: Why sk_refcnt is 0 at that point of the code? The
> > socket should be destroyed if it ref is 0, but it wasn't, so something
> > in another point of the code went is wrong. "Sometimes" is not a good
> > description of the problem, you have to show why that happened.
> >
> >
>
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH] Remove old hcid.conf
From: Gustavo F. Padovan @ 2010-10-28 9:21 UTC (permalink / raw)
To: linux-bluetooth
---
src/hcid.conf | 57 -------------
src/hcid.conf.5.in | 227 ----------------------------------------------------
2 files changed, 0 insertions(+), 284 deletions(-)
delete mode 100644 src/hcid.conf
delete mode 100644 src/hcid.conf.5.in
diff --git a/src/hcid.conf b/src/hcid.conf
deleted file mode 100644
index b6ce3b4..0000000
--- a/src/hcid.conf
+++ /dev/null
@@ -1,57 +0,0 @@
-#
-# HCI daemon configuration file.
-#
-
-# HCId options
-options {
- # Automatically initialize new devices
- autoinit yes;
-
- # Security Manager mode
- # none - Security manager disabled
- # auto - Use local PIN for incoming connections
- # user - Always ask user for a PIN
- #
- security user;
-
- # Pairing mode
- # none - Pairing disabled
- # multi - Allow pairing with already paired devices
- # once - Pair once and deny successive attempts
- pairing multi;
-
- # Default PIN code for incoming connections
- passkey "BlueZ";
-}
-
-# Default settings for HCI devices
-device {
- # Local device name
- # %d - device id
- # %h - host name
- name "BlueZ (%d)";
-
- # Local device class
- class 0x000100;
-
- # Default packet type
- #pkt_type DH1,DM1,HV1;
-
- # Inquiry and Page scan
- iscan enable; pscan enable;
-
- # Default link mode
- # none - no specific policy
- # accept - always accept incoming connections
- # master - become master on incoming connections,
- # deny role switch on outgoing connections
- lm accept;
-
- # Default link policy
- # none - no specific policy
- # rswitch - allow role switch
- # hold - allow hold mode
- # sniff - allow sniff mode
- # park - allow park mode
- lp rswitch,hold,sniff,park;
-}
diff --git a/src/hcid.conf.5.in b/src/hcid.conf.5.in
deleted file mode 100644
index b771389..0000000
--- a/src/hcid.conf.5.in
+++ /dev/null
@@ -1,227 +0,0 @@
-.TH "HCID.CONF" "5" "March 2004" "hcid.conf - HCI daemon" "System management commands"
-.SH "NAME"
-@CONFIGDIR@/hcid.conf \- Configuration file for the hcid Bluetooth HCI daemon
-
-.SH "DESCRIPTION"
-@CONFIGDIR@/hcid.conf contains all the options needed by the Bluetooth Host Controller Interface daemon.
-
-It consists of sections and parameters. A section begins with
-the name of the section followed by optional specifiers and the
-parameters inside curly brackets. Sections contain parameters of
-the form:
-.TP
-\fIname\fP \fIvalue1\fP, \fIvalue2\fP ... ;
-
-.PP
-Any character after a hash ('#') character is ignored until newline.
-Whitespace is also ignored.
-
-
-The valid section names for
-.B hcid.conf
-are, at the moment:
-
-.TP
-.B options
-contains generic options for hcid and the pairing policy.
-.TP
-.B device
-contains lower\-level options for the hci devices connected to the computer.
-.SH "OPTIONS SECTION"
-The following parameters may be present in an option section:
-
-
-.TP
-\fBautoinit\fP yes|no
-
-Automatically initialize newly connected devices. The default is \fIno\fP.
-
-
-.TP
-\fBpairing\fP none|multi|once
-
-\fInone\fP means that pairing is disabled. \fImulti\fP allows pairing
-with already paired devices. \fIonce\fP allows pairing once and denies
-successive attempts. The default hcid configuration is shipped with \fBmulti\fP
-enabled
-
-.TP
-\fBoffmode\fP noscan|devdown
-
-\fInoscan\fP means that page and inquiry scans are disabled when you call
-SetMode("off"). \fIdevdown\fP sets the adapter into down state (same what
-\fIhciconfig hci0 down\fP does).
-
-.TP
-\fBdeviceid\fP <vendor>:<product>:<version>
-
-This option allows to specify the vendor and product information of the
-Bluetooth device ID service record.
-
-.TP
-\fBpasskey\fP "\fIpin\fP"
-
-The default PIN for incoming connections if \fBsecurity\fP has been
-set to \fIauto\fP.
-
-.TP
-\fBsecurity\fP none|auto|user
-
-\fInone\fP means the security manager is disabled. \fIauto\fP uses
-local PIN, by default from pin_code, for incoming
-connections. \fIuser\fP always asks the user for a PIN.
-
-.SH "DEVICE SECTION"
-Parameters within a device section with no specifier, the default
-device section, will be applied to all devices and device sections
-where these are unspecified. The following optional device specifiers
-are supported:
-
-.TP
-\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP
-
-Parameters specified within this section will be applied to the device
-with this \fIdevice bluetooth address\fP. All other parameters are applied from
-the default section.
-
-.TP
-\fBhci\fIn\fP
-
-Parameters specified within this section will be applied to the device
-with this \fIdevice interface\fP, unless that device is matched by a
-\fIdevice address\fP section. All other parameters are applied from
-the default section.
-
-
-.PP
-\fBNote\fP: Most of the options supported in the \fBdevice\fP section are described to some extent in the bluetooth specification version 1.2 Vol2, Part E section 6. Please refer to it for technical details.
-
-.PP
-The following parameters may be present in a device section:
-
-.TP
-\fBname\fP "\fIname\fP"
-
-The device name. \fI%d\fP inserts the device id. \fI%h\fP inserts
-the host name.
-
-
-.TP
-\fBclass\fP 0x\fISSDDdd\fP (three bytes)
-
-The Bluetooth Device Class is described in the Bluetooth Specification section 1.2 ("Assigned Numbers \- Bluetooth Baseband").
-
-The default shipped with hcid is 0x000100 which simply stands for "Computer".
-
-The Bluetooth device class is a high\-level description of the bluetooth device, composed of three bytes: the "Major Service Class" (byte "SS" above), the "Major Device Class" (byte "DD" above) and the "Minor Device Class" (byte "dd" above). These classes describe the high\-level capabilities of the device, such as "Networking Device", "Computer", etc. This information is often used by clients who are looking for a certain type of service around them.
-
-Where it becomes tricky is that another type of mechanism for service discovery exists: "SDP", as in "Service Discovery Protocol".
-
-In practice, most Bluetooth clients scan their surroundings in two successive steps: they first look for all bluetooth devices around them and find out their "class". You can do this on Linux with the \fBhcitool scan\fP command. Then, they use SDP in order to check if a device in a given class offers the type of service that they want.
-
-This means that the hcid.conf "class" parameter needs to be set up properly if particular services are running on the host, such as "PAN", or "OBEX Obect Push", etc: in general a device looking for a service such as "Network Access Point" will only scan for this service on devices containing "Networking" in their major service class.
-
-
-.IP
-Major service class byte allocation (from LSB to MSB):
-
-Bit 1: Positioning (Location identification)
-
-Bit 2: Networking (LAN, Ad hoc, ...)
-
-Bit 3: Rendering (Printing, Speaker, ...)
-
-Bit 4: Capturing (Scanner, Microphone, ...)
-
-Bit 5: Object Transfer (v\-Inbox, v\-Folder, ...)
-
-Bit 6: Audio (Speaker, Microphone, Headset service, ...)
-
-Bit 7: Telephony (Cordless telephony, Modem, Headset service, ...)
-
-Bit 8: Information (WEB\-server, WAP\-server, ...)
-
-.IP
-Example: class 0x02hhhh : the device offers networking service
-
-
-.IP
-Major device class allocation:
-
-0x00: Miscellaneous
-
-0x01: Computer (desktop,notebook, PDA, organizers, .... )
-
-0x02: Phone (cellular, cordless, payphone, modem, ...)
-
-0x03: LAN /Network Access point
-
-0x04: Audio/Video (headset,speaker,stereo, video display, vcr.....
-
-0x05: Peripheral (mouse, joystick, keyboards, ..... )
-
-0x06: Imaging (printing, scanner, camera, display, ...)
-
-Other values are not defined (refer to the Bluetooth specification for more details
-
-.IP
-Minor device class allocation: the meaning of this byte depends on the major class allocation, please refer to the Bluetooth specifications for more details).
-
-.IP
-.B Example:
-if PAND runs on your server, you need to set up at least \fBclass 0x020100\fP, which stands for "Service Class: Networking" and "Device Class: Computer, Uncategorized".
-
-
-.TP
-\fBiscan\fP enable|disable
-.TP
-\fBpscan\fP enable|disable
-
-Bluetooth devices discover and connect to each other through the use of two special Bluetooth channels, the Inquiry and Page channels (described in the Bluetooth Spec Volume 1, Part A, Section 3.3.3, page 35). These two options enable the channels on the bluetooth device.
-
-\fBiscan enable\fP: makes the bluetooth device "discoverable" by enabling it to answer "inquiries" from other nearby bluetooth devices.
-
-\fBpscan enable\fP: makes the bluetooth device "connectable to" by enabling the use of the "page scan" channel.
-
-.TP
-\fBlm\fP none|accept,master
-
-\fInone\fP means no specific policy. \fIaccept\fP means always accept
-incoming connections. \fImaster\fP means become master on incoming
-connections and deny role switch on outgoing connections.
-
-.TP
-\fBlp\fP none|rswitch,hold,sniff,park
-
-\fInone\fP means no specific policy. \fIrswitch\fP means allow role
-switch. \fIhold\fP means allow hold mode. \fIsniff\fP means allow
-sniff mode. \fIpark\fP means allow park mode. Several options can be
-combined.
-
-This option determines the various operational modes that are allowed for this device when it participates to a piconet. Normally hold and sniff should be enabled for standard operations.
-
-hold: this mode is related to synchronous communications (SCO voice channel for example).
-
-sniff: when in this mode, a device is only present on the piconet during determined slots of time, allowing it to do other things when it is "absent", for example to scan for other bluetooth devices.
-
-park: this is a mode where the device is put on standby on the piconet, for power\-saving purposes for example.
-
-rswitch: this is a mode that enables role\-switch (master <\-> slave) between two devices in a piconet. It is not clear whether this needs to be enabled in order to make the "lm master" setting work properly or not.
-
-.TP
-\fBpageto\fP \fIn\fP
-
-Page Timeout measured in number of baseband slots. Interval length = N * 0.625 msec (1 baseband slot)
-
-.TP
-\fBdiscovto\fP \fIn\fP
-
-The time in seconds that the device will stay in discoverable mode. 0 disables this feature and forces the device to be always discoverable.
-
-.SH "FILES"
-.TP
-.I @CONFIGDIR@/hcid.conf
-Default location of the global configuration file.
-
-.SH "AUTHOR"
-This manual page was written by Edouard Lafargue, Fredrik Noring, Maxim Krasnyansky and Marcel Holtmann.
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Gustavo F. Padovan @ 2010-10-28 9:17 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Ville Tervo, Anderson Briglia, linux-bluetooth@vger.kernel.org,
Vinicius Costa Gomes
In-Reply-To: <AANLkTikVG-gjd1d7T6ETbfXCU0jp2GsR0miWJFO55hps@mail.gmail.com>
* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2010-10-26 08:22:16 -0700]:
> Hi,
>
> On Tue, Oct 26, 2010 at 2:26 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > On Mon, Oct 25, 2010 at 03:03:56PM +0200, ext Gustavo F. Padovan wrote:
> >> Hi Vinicius,
> >>
> >> * Anderson Briglia <anderson.briglia@openbossa.org> [2010-10-22 19:56:57 -0400]:
> >>
> >> > From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >> >
> >> > These simple commands will allow the SMP procedure to be started
> >> > and terminated with a not supported error. This is the first step
> >> > toward something useful.
> >> >
> >> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >> > ---
> >> > net/bluetooth/l2cap.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> > 1 files changed, 117 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >> > index 1ac44f4..ba87c84 100644
> >> > --- a/net/bluetooth/l2cap.c
> >> > +++ b/net/bluetooth/l2cap.c
> >> > @@ -54,6 +54,7 @@
> >> > #include <net/bluetooth/bluetooth.h>
> >> > #include <net/bluetooth/hci_core.h>
> >> > #include <net/bluetooth/l2cap.h>
> >> > +#include <net/bluetooth/smp.h>
> >> >
> >> > #define VERSION "2.15"
> >> >
> >> > @@ -307,6 +308,85 @@ static void l2cap_chan_del(struct sock *sk, int err)
> >> > }
> >> > }
> >> >
> >> > +static struct sk_buff *smp_build_cmd(struct l2cap_conn *conn, u8 code,
> >> > + u16 dlen, void *data)
> >>
> >> Call this l2cap_smp_build_cmd()
> >
> > Should the whole smp stuff be in separate file (smp.c)? It's not a l2cap feature but a
> > protocol using l2cap. In that case smp_build_cmd would be good name.
>
> +1
>
> It is also much better for maintenance and development since there is
> less patches touching the l2cap.c so less chances of conflicts,
> rebases and regressions on l2cap.
Yep, we may need a new smp.c file.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: HFP Pulseaudio Source destroyed "too quickly" at the end of a call
From: Thomas Wälti @ 2010-10-28 8:57 UTC (permalink / raw)
To: johan.hedberg, linux-bluetooth, Krisztian.Litkey, Jyri.Sarha
In-Reply-To: <20101027222602.GB24756@jh-x301>
2010/10/28 Johan Hedberg <johan.hedberg@gmail.com>:
> Probably this isn't really BlueZ related, but the audio policy module
> (on the PulseAudio side) in the N900 kicks in and requests these changes
> to the audio streams. Unfortunately I'm not really an expert with that
> code (and afaik the audio policy part is closed) so I can't really offer
> more insight on the issue. Just speculating, but you might be able to
> accomplish something by hacking in some delays into the pulse bluetooth
> modules (but again, I'm not really familiar with them so this might not
> be a feasible approach).
Thanks for the prompt feedback, Johan.
I had looked at the PulseAudio Bluetooth Modules source
(http://git.0pointer.de/?p=pulseaudio.git;a=tree;f=src/modules/bluetooth;hb=HEAD)
before, but didn't see anything (but then, it's a bit over my head :-)
It seems like Joao Paulo Rechi Vita is the author of the relevant
source, and as he also did the Bluez HFP implementation (IIRC), I hope
he might shed a light on this. I'm asking on this list because once
HFP "hangs up", it probably issues some teardown event to its
consumers.
(It would be nice if I could get this solved this before heading to
the MeeGo conf in Dublin - see you there :-)
Best regards
-Tom
^ permalink raw reply
* Re: [PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer()
From: Gustavo F. Padovan @ 2010-10-28 8:49 UTC (permalink / raw)
To: haijun liu; +Cc: Haijun Liu, linux-bluetooth
In-Reply-To: <AANLkTin2RaOZbRbg4faCFUxMLVi-FoY2xnNqq0a8cHpN@mail.gmail.com>
Hi Haijun,
* haijun liu <liuhaijun.er@gmail.com> [2010-10-26 09:32:19 +0800]:
> Hi Gustavo,
>
> >> >> During test session with another vendor's bt stack, found that in
> >> >> l2cap_chan_del() using del_timer() caused l2cap_monitor_timeout()
> >> >> be called after the sock was freed, so it raised a system crash.
> >> >> So I just replaced del_timer() with del_timer_sync() to solve it.
> >> >
> >> > NAK on this. If you read the del_timer_sync() documentation you can
> >> > see that you can't call del_timer_sync() on interrupt context. The
> >> > possible solution here is to check in the beginning of
> >> > l2cap_monitor_timeout() if your sock is still valid.
> >> >
> >>
> >> You are right, I only considered close() interface, so missed the interrupt
> >> context.
> >>
> >> It's very difficult to check sock valid or not in timeout procedure, since it's
> >> an interrupt context, and only can get context from parameter pre-stored,
> >> except global variables.
> >
> > I think you can check for sk == null there.
> >
>
> It's a pre-stored parameter, it will not change by itself.
I looked a bit into this and a good solution seems to be to hold a
reference to the sock when we call a mod_timer() and then put the
reference when we call del_timer() and the timer is inactive or when
l2cap_monitor_timeout(). Look net/sctp/ for examples.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH v3] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1288255408-3967-1-git-send-email-dmitriy.paliy@nokia.com>
This fixes obexd crash in 3-way calling scenario when filtered listing
response is empty. Valid cache and empty pbap buffer mean that cache was
already attempted to be created within a single session, but no data was
available. Hence, it is not notified and no such file error returned.
Such avoids clearing and creating a new cache operations for each incoming
call, which is one of possible solution to fix this bug. It can be
extensive for large phone books. New cache is not created within current
obex session or unless path is changed.
---
plugins/pbap.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 11cb678..3ea7d6b 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -751,6 +751,15 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
/* PullvCardListing always get the contacts from the cache */
if (pbap->cache.valid) {
+ /*
+ * Valid cache and empty buffer mean that cache was already
+ * created within a single session, but no data is available.
+ */
+ if (!pbap->buffer) {
+ ret = -ENOENT;
+ goto fail;
+ }
+
cache_ready_notify(pbap);
goto done;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 0/1 v3] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:43 UTC (permalink / raw)
To: linux-bluetooth
Hi,
In this revision comments are improved to be more consitent and more
focused on the bug fixed. Todo note is removed since there is already
similar fixme note in pbap_setpath function.
BR,
Dmitriy
^ permalink raw reply
* Re: [PATCH v2] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:07 UTC (permalink / raw)
To: ext Johan Hedberg; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101027190347.GA8025@jh-x301>
Hi Johan,
On Wed, 2010-10-27 at 21:03 +0200, ext Johan Hedberg wrote:
> Hi Dmitriy,
>
> On Wed, Oct 27, 2010, Dmitriy Paliy wrote:
> > This fixes obexd crash in 3-way calling scenario when listing response is
> > empty. Valid cache and empty pbap buffer mean that cache was already attempted
> > to be created within a single session, but no data was available. Hence, it
> > is not notified and no such file error returned. New cache is not created
> > within current obex session or unless path is changed. Such removes necessity
> > of querying and filtering contacts for each incoming call in the other case,
> > which is extensive for large phone books. On the other hand, if user updates
> > contacts, cache will not be renewed till obex session is closed or path is
> > changed. Therefore TODO: note is added that clear of cache should be defined
> > besides of end of session or change of path.
>
> The commit message seems to have a max width of 78 characters which
> means that it's not viewable with git log on a 80 character terminal
> (git log indents the output by 4 characters). Please keep the max commit
> message width to 74 characters or so. Also, if possible try to split it
> up into multiple paragraphs. Paragraphs longer than 6 lines tend to be a
> bit harder to follow.
>
> > + * TODO: Define clear cache besides end of session or change
> > + * of path.
>
> That doesn't sound like proper english to me and I'm not sure what
> you're trying to say. Should it be "Define a clear distinction between
> end of session and change of path"?
Thanks for comments. What I meant is following. Cache is created at the
moment when there is an incoming call in the middle of another ongoing
call. Clear cache and create a new one when the call is retried, which
is one of possible solutions to fix this bug, looks inefficient to me.
Therefore, already created cache is valid either till session is closed
or path is changed. However, if user updates his contacts, then changes
will not affect cache within these limits. What I was trying to say in
the todo note is that in order to improve logic when cache is created
and cleared, besides two facts written above (end of session and change
of path), another way to renew cache should be defined.
Br,
Dmitriy
^ permalink raw reply
* [PATCH 5/5] Fix not being able to set discoverable when discoverable timeout is set
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
Since setting Discoverable property now has the ability to turn on the
adapter if off we should no longer ignore discoverable mode if a timeout
is set.
---
src/adapter.c | 51 +++++++++++++++++++++++++++++----------------------
1 files changed, 29 insertions(+), 22 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index b5e73b7..19fe5a8 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -529,6 +529,32 @@ static struct session_req *create_session(struct btd_adapter *adapter,
return session_ref(req);
}
+static int adapter_set_mode(struct btd_adapter *adapter, uint8_t mode)
+{
+ int err;
+
+ if (mode == MODE_CONNECTABLE)
+ err = adapter_ops->set_connectable(adapter->dev_id);
+ else
+ err = adapter_ops->set_discoverable(adapter->dev_id);
+
+ if (err < 0)
+ return err;
+
+ if (mode == MODE_CONNECTABLE)
+ return 0;
+
+ adapter_remove_discov_timeout(adapter);
+
+ if (adapter->discov_timeout)
+ adapter_set_discov_timeout(adapter, adapter->discov_timeout);
+
+ if (mode != MODE_LIMITED && adapter->mode == MODE_LIMITED)
+ adapter_set_limited_discoverable(adapter, FALSE);
+
+ return 0;
+}
+
static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
DBusMessage *msg)
{
@@ -559,25 +585,11 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
if (new_mode == adapter->mode)
return 0;
- if (new_mode == MODE_CONNECTABLE)
- err = adapter_ops->set_connectable(adapter->dev_id);
- else
- err = adapter_ops->set_discoverable(adapter->dev_id);
+ err = adapter_set_mode(adapter, new_mode);
if (err < 0)
return err;
- if (new_mode > MODE_CONNECTABLE) {
- adapter_remove_discov_timeout(adapter);
-
- if (adapter->discov_timeout)
- adapter_set_discov_timeout(adapter,
- adapter->discov_timeout);
-
- if (new_mode != MODE_LIMITED && adapter->mode == MODE_LIMITED)
- adapter_set_limited_discoverable(adapter, FALSE);
- }
-
done:
modestr = mode2str(new_mode);
write_device_mode(&adapter->bdaddr, modestr);
@@ -2429,18 +2441,13 @@ static int adapter_up(struct btd_adapter *adapter, const char *mode)
write_device_mode(&adapter->bdaddr, onmode);
return adapter_up(adapter, onmode);
- } else if (!g_str_equal(mode, "connectable") &&
- adapter->discov_timeout == 0) {
- /* Set discoverable only if timeout is 0 */
+ } else if (!g_str_equal(mode, "connectable")) {
adapter->mode = MODE_DISCOVERABLE;
scan_mode = SCAN_PAGE | SCAN_INQUIRY;
}
proceed:
- if (scan_mode == SCAN_PAGE)
- err = adapter_ops->set_connectable(adapter->dev_id);
- else
- err = adapter_ops->set_discoverable(adapter->dev_id);
+ err = adapter_set_mode(adapter, adapter->mode);
if (err < 0)
return err;
--
1.7.1
^ permalink raw reply related
* [PATCH 4/5] Fix not replying when mode is limited discoverable or discoverable
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
When changing from/to limited discoverable or discoverable it won't
change the scan mode thus causing set_mode_complete to not be called.
---
src/adapter.c | 49 +++++++++++++++++++++++++++++++++----------------
1 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e5f7730..b5e73b7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -584,25 +584,41 @@ done:
DBG("%s", modestr);
- if (msg != NULL)
- /* Wait for mode change to reply */
- adapter->pending_mode = create_session(adapter, connection,
- msg, new_mode, NULL);
- else
+ if (msg != NULL) {
+ /* Limited to Discoverable and vice-versa doesn't cause any
+ change to scan mode */
+ if (g_str_equal(modestr, mode2str(adapter->mode)) == TRUE) {
+ DBusMessage *reply;
+
+ reply = g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+
+ g_dbus_send_message(connection, reply);
+ } else
+ /* Wait for mode change to reply */
+ adapter->pending_mode = create_session(adapter,
+ connection,
+ msg, new_mode,
+ NULL);
+ } else
/* Nothing to reply just write the new mode */
adapter->mode = new_mode;
return 0;
}
-static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
- gboolean powered, void *data)
+static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
+ gboolean discoverable, void *data)
{
struct btd_adapter *adapter = data;
uint8_t mode;
int err;
- mode = powered ? get_mode(&adapter->bdaddr, "on") : MODE_OFF;
+ mode = discoverable ? MODE_DISCOVERABLE : MODE_CONNECTABLE;
+
+ if (mode == MODE_DISCOVERABLE && adapter->pairable &&
+ adapter->discov_timeout > 0 &&
+ adapter->discov_timeout <= 60)
+ mode = MODE_LIMITED;
if (mode == adapter->mode)
return dbus_message_new_method_return(msg);
@@ -614,19 +630,20 @@ static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
return NULL;
}
-static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
- gboolean discoverable, void *data)
+static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
+ gboolean powered, void *data)
{
struct btd_adapter *adapter = data;
uint8_t mode;
int err;
- mode = discoverable ? MODE_DISCOVERABLE : MODE_CONNECTABLE;
+ if (powered) {
+ mode = get_mode(&adapter->bdaddr, "on");
+ return set_discoverable(conn, msg, mode == MODE_DISCOVERABLE,
+ data);
+ }
- if (mode == MODE_DISCOVERABLE && adapter->pairable &&
- adapter->discov_timeout > 0 &&
- adapter->discov_timeout <= 60)
- mode = MODE_LIMITED;
+ mode = MODE_OFF;
if (mode == adapter->mode)
return dbus_message_new_method_return(msg);
@@ -635,7 +652,7 @@ static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
if (err < 0)
return failed_strerror(msg, -err);
- return 0;
+ return NULL;
}
static DBusMessage *set_pairable(DBusConnection *conn, DBusMessage *msg,
--
1.7.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox