* Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"
From: Shuah Khan @ 2013-10-03 15:05 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo Padovan, Hans de Goede, USB list, linux-bluetooth,
Gustavo Padovan, stable, Shuah Khan
In-Reply-To: <508150F6-D142-4406-8CC9-57DDE754F391@holtmann.org>
On 10/02/2013 09:39 PM, Marcel Holtmann wrote:
> Hi Gustavo,
>
>>> Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
>>> devices default to hid mode for BIOS use. This means that after having been
>>> reset they will revert to HID mode, and are no longer usable as a HCI.
>>>
>>> Therefor it is a very bad idea to just blindly make reset_resume point to
>>> the regular resume handler. Note that the btusb driver has no clue how to
>>> switch these devices from hid to hci mode, this is done in userspace through
>>> udev rules, so the proper way to deal with this is to not have a reset-resume
>>> handler and instead let the usb-system re-enumerate the device, and re-run
>>> the udev rules.
>>>
>>> I must also note, that the commit message for the commit causing this
>>> problem has a very weak motivation for the change:
>>>
>>> "Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
>>> following message after power management device test. This change sets
>>> reset_resume to btusb_resume().
>>>
>>> [ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
>>> [ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"
>>>
>>> Making a change solely to silence a warning while also changing important
>>> behavior (normal resume handling versus re-enumeration) requires a commit
>>> message with a proper explanation why it is safe to do so, which clearly lacks
>>> here, and unsurprisingly it turns out to not be safe to make this change.
>>>
>>> Reverting the commit in question fixes bt no longer working on my Dell
>>> E6430 after a suspend/resume, and I believe it likely also fixes the
>>> following bugs:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=988481
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1010649
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
>>>
>>> This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.
>>>
>>> Cc: Shuah Khan <shuah.kh@samsung.com>
>>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>
>> Patch has been applied to bluetooth.git. Thanks.
>
> why? Because we have one broken Dell Bluetooth dongle. Do we actually know how this affects other chips. The dell HID Proxy thing has always been special case and that is Dell's fault. Look at the extra code that we have in hid2hci tool and its udev rules for Dell hardware. Is anybody actually willing to investigate this one properly.
>
> Regards
>
> Marcel
>
Sorry for the late reply on this. Sorry for the regression on Dell. I
didn't see any problems when I tested it on the bluetooth mouse I have.
However, I understand what Hans is saying. My change did cause a
regression. I will investigate this further.
The warning message itself is very misleading in the sense that it gives
the impression that the resume routine that should have been installed
is missing. Which is what caused me to fix things in the first place.
thanks,
-- Shuah
--
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658
^ permalink raw reply
* pull request: bluetooth-next 2013-10-03
From: Gustavo Padovan @ 2013-10-03 17:45 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, linux-bluetooth, linux-kernel
Hi John,
A series of patches for 3.12. The big work here is from Marcel and Johan. They
did a lot of work in the L2CAP, HCI and MGMT layers. The most important ones
are the addition of a new MGMT command to enable/disable LE advertisement and
the introduction of the HCI user channel to allow applications to get directly
and exclusive access to Bluetooth devices.
Please pull, or let me know of any issues. Thanks!
Gustavo
--
The following changes since commit f4e1a4d3ecbb9e42bdf8e7869ee8a4ebfa27fb20:
rt2800: change initialization sequence to fix system freeze (2013-09-09 14:44:34 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next for-upstream
for you to fetch changes up to 4f3e219d95a3c31b916dcd5e2631c4e440736f79:
Bluetooth: Only one command per L2CAP LE signalling is supported (2013-10-03 16:09:59 +0300)
----------------------------------------------------------------
Amitkumar Karwar (4):
Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
Bluetooth: btmrvl: get rid of struct btmrvl_cmd
Bluetooth: btmrvl: add setup handler
Bluetooth: btmrvl: add calibration data download support
Andre Guedes (2):
Bluetooth: Fix security level for peripheral role
Bluetooth: Fix encryption key size for peripheral role
DoHyun Pyun (8):
Bluetooth: Add the definition and structure for Set Reserved LT_ADDR
Bluetooth: Add the definition and structure for Delete Reserved LT_ADDR
Bluetooth: Add the definition and structure for Set CSB Data
Bluetooth: Add the structure for Write Sync Train Parameters
Bluetooth: Add the definition and structure for Set CSB
Bluetooth: Add the definition for Start Synchronization Train
Bluetooth: Add the definition and stcuture for Sync Train Complete
Bluetooth: Add the definition for Slave Page Response Timeout
Gianluca Anzolin (1):
Bluetooth: don't release the port in rfcomm_dev_state_change()
Gustavo Padovan (1):
Merge git://git.kernel.org/.../bluetooth/bluetooth
Johan Hedberg (25):
Bluetooth: Introduce a new HCI_RFKILLED flag
Bluetooth: Fix rfkill functionality during the HCI setup stage
Bluetooth: Remove unused event mask struct
Bluetooth: Fix double error response for l2cap_create_chan_req
Bluetooth: Fix L2CAP error return used for failed channel lookups
Bluetooth: Fix L2CAP Disconnect response for unknown CID
Bluetooth: Fix L2CAP command reject reason
Bluetooth: Fix sending responses to identified L2CAP response packets
Bluetooth: Fix responding to invalid L2CAP signaling commands
Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
Bluetooth: Add synchronization train parameters reading support
Bluetooth: Add event mask page 2 setting support
Bluetooth: Add clarifying comment to bt_sock_wait_state()
Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
Bluetooth: Fix busy return for mgmt_set_powered in some cases
Bluetooth: Move mgmt response convenience functions to a better location
Bluetooth: Use async request for LE enable/disable
Bluetooth: Add new mgmt setting for LE advertising
Bluetooth: Add new mgmt_set_advertising command
Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
Bluetooth: Fix workqueue synchronization in hci_dev_open
Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
Bluetooth: Add a new mgmt_set_bredr command
Bluetooth: Fix REJECTED vs NOT_SUPPORTED mgmt responses
Bluetooth: Fix advertising data flags with disabled BR/EDR
Ken O'Brien (1):
Bluetooth: btusb: Add support for Belkin F8065bf
Marcel Holtmann (35):
Bluetooth: Refactor raw socket filter into more readable code
Bluetooth: Fix handling of getpeername() for HCI sockets
Bluetooth: Fix handling of getsockname() for HCI sockets
Bluetooth: Report error for HCI reset ioctl when device is down
Bluetooth: Fix error handling for HCI socket options
Bluetooth: Restrict ioctls to HCI raw channel sockets
Bluetooth: Introduce user channel flag for HCI devices
Bluetooth: Introduce new HCI socket channel for user operation
Bluetooth: Use devname:vhci module alias for virtual HCI driver
Bluetooth: Add support creating virtual AMP controllers
Bluetooth: Disable upper layer connections when user channel is active
Bluetooth: Use GFP_KERNEL when cloning SKB in a workqueue
Bluetooth: Only schedule raw queue when user channel is active
Bluetooth: Use only 2 bits for controller type information
Bluetooth: Replace BDADDR_LOCAL with BDADDR_NONE
Bluetooth: Provide high speed configuration option
Bluetooth: Send new settings event when changing high speed option
Bluetooth: Require CAP_NET_ADMIN for HCI User Channel operation
Bluetooth: Enable -D__CHECK_ENDIAN__ for sparse by default
Bluetooth: Restrict disabling of HS when controller is powered off
Bluetooth: Add management command for setting static address
Bluetooth: Increment management interface revision
Bluetooth: Fix memory leak with L2CAP signal channels
Bluetooth: Restrict SSP setting changes to BR/EDR enabled controllers
Bluetooth: Allow setting static address even if LE is disabled
Bluetooth: Restrict loading of link keys to BR/EDR capable controllers
Bluetooth: Restrict loading of long term keys to LE capable controllers
Bluetooth: Allow changing device class when BR/EDR is disabled
Bluetooth: Fix switch statement order for L2CAP fixed channels
Bluetooth: Don't copy L2CAP LE signalling to raw sockets
Bluetooth: SMP packets are only valid on LE connections
Bluetooth: L2CAP connectionless channels are only valid for BR/EDR
Bluetooth: Drop packets on ATT fixed channel on BR/EDR
Bluetooth: Check minimum length of SMP packets
Bluetooth: Only one command per L2CAP LE signalling is supported
Peng Chen (1):
Bluetooth: Add a new PID/VID 0cf3/e005 for AR3012.
Peter Senna Tschudin (1):
Bluetooth: Fix assignment of 0/1 to bool variables
Raphael Kubo da Costa (1):
Bluetooth: Add support for BCM20702A0 [0b05, 17cb]
Syam Sidhardhan (1):
Bluetooth: Fix ACL alive for long in case of non pariable devices
drivers/bluetooth/Makefile | 2 +
drivers/bluetooth/ath3k.c | 2 +
drivers/bluetooth/btmrvl_drv.h | 12 +-
drivers/bluetooth/btmrvl_main.c | 269 +++++++++++++++---------
drivers/bluetooth/btmrvl_sdio.c | 15 +-
drivers/bluetooth/btmrvl_sdio.h | 2 +
drivers/bluetooth/btusb.c | 5 +
drivers/bluetooth/hci_vhci.c | 170 ++++++++++-----
include/net/bluetooth/bluetooth.h | 5 +-
include/net/bluetooth/hci.h | 82 +++++++-
include/net/bluetooth/hci_core.h | 2 +-
include/net/bluetooth/l2cap.h | 1 +
include/net/bluetooth/mgmt.h | 11 +
net/bluetooth/Makefile | 2 +
net/bluetooth/af_bluetooth.c | 41 ++++
net/bluetooth/hci_conn.c | 4 +
net/bluetooth/hci_core.c | 213 +++++++++++++++----
net/bluetooth/hci_event.c | 23 ++-
net/bluetooth/hci_sock.c | 204 ++++++++++++++----
net/bluetooth/l2cap_core.c | 169 +++++++++------
net/bluetooth/l2cap_sock.c | 20 +-
net/bluetooth/mgmt.c | 562 ++++++++++++++++++++++++++++++++++++++++----------
net/bluetooth/rfcomm/sock.c | 7 +-
net/bluetooth/rfcomm/tty.c | 35 +---
net/bluetooth/smp.c | 15 +-
25 files changed, 1390 insertions(+), 483 deletions(-)
^ permalink raw reply
* Re: pull request: bluetooth-next 2013-10-03
From: Gustavo Padovan @ 2013-10-03 17:48 UTC (permalink / raw)
To: linville, linux-wireless, linux-bluetooth, linux-kernel
In-Reply-To: <20131003174524.GA22728@joana>
[-- Attachment #1: Type: text/plain, Size: 8314 bytes --]
Hi John
I forgot to sign it. Signing now..
Gustavo
2013-10-03 Gustavo Padovan <gustavo@padovan.org>:
> Hi John,
>
> A series of patches for 3.12. The big work here is from Marcel and Johan. They
> did a lot of work in the L2CAP, HCI and MGMT layers. The most important ones
> are the addition of a new MGMT command to enable/disable LE advertisement and
> the introduction of the HCI user channel to allow applications to get directly
> and exclusive access to Bluetooth devices.
>
> Please pull, or let me know of any issues. Thanks!
>
> Gustavo
>
> --
> The following changes since commit f4e1a4d3ecbb9e42bdf8e7869ee8a4ebfa27fb20:
>
> rt2800: change initialization sequence to fix system freeze (2013-09-09 14:44:34 -0400)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next for-upstream
>
> for you to fetch changes up to 4f3e219d95a3c31b916dcd5e2631c4e440736f79:
>
> Bluetooth: Only one command per L2CAP LE signalling is supported (2013-10-03 16:09:59 +0300)
>
> ----------------------------------------------------------------
> Amitkumar Karwar (4):
> Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
> Bluetooth: btmrvl: get rid of struct btmrvl_cmd
> Bluetooth: btmrvl: add setup handler
> Bluetooth: btmrvl: add calibration data download support
>
> Andre Guedes (2):
> Bluetooth: Fix security level for peripheral role
> Bluetooth: Fix encryption key size for peripheral role
>
> DoHyun Pyun (8):
> Bluetooth: Add the definition and structure for Set Reserved LT_ADDR
> Bluetooth: Add the definition and structure for Delete Reserved LT_ADDR
> Bluetooth: Add the definition and structure for Set CSB Data
> Bluetooth: Add the structure for Write Sync Train Parameters
> Bluetooth: Add the definition and structure for Set CSB
> Bluetooth: Add the definition for Start Synchronization Train
> Bluetooth: Add the definition and stcuture for Sync Train Complete
> Bluetooth: Add the definition for Slave Page Response Timeout
>
> Gianluca Anzolin (1):
> Bluetooth: don't release the port in rfcomm_dev_state_change()
>
> Gustavo Padovan (1):
> Merge git://git.kernel.org/.../bluetooth/bluetooth
>
> Johan Hedberg (25):
> Bluetooth: Introduce a new HCI_RFKILLED flag
> Bluetooth: Fix rfkill functionality during the HCI setup stage
> Bluetooth: Remove unused event mask struct
> Bluetooth: Fix double error response for l2cap_create_chan_req
> Bluetooth: Fix L2CAP error return used for failed channel lookups
> Bluetooth: Fix L2CAP Disconnect response for unknown CID
> Bluetooth: Fix L2CAP command reject reason
> Bluetooth: Fix sending responses to identified L2CAP response packets
> Bluetooth: Fix responding to invalid L2CAP signaling commands
> Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
> Bluetooth: Add synchronization train parameters reading support
> Bluetooth: Add event mask page 2 setting support
> Bluetooth: Add clarifying comment to bt_sock_wait_state()
> Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
> Bluetooth: Fix busy return for mgmt_set_powered in some cases
> Bluetooth: Move mgmt response convenience functions to a better location
> Bluetooth: Use async request for LE enable/disable
> Bluetooth: Add new mgmt setting for LE advertising
> Bluetooth: Add new mgmt_set_advertising command
> Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
> Bluetooth: Fix workqueue synchronization in hci_dev_open
> Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
> Bluetooth: Add a new mgmt_set_bredr command
> Bluetooth: Fix REJECTED vs NOT_SUPPORTED mgmt responses
> Bluetooth: Fix advertising data flags with disabled BR/EDR
>
> Ken O'Brien (1):
> Bluetooth: btusb: Add support for Belkin F8065bf
>
> Marcel Holtmann (35):
> Bluetooth: Refactor raw socket filter into more readable code
> Bluetooth: Fix handling of getpeername() for HCI sockets
> Bluetooth: Fix handling of getsockname() for HCI sockets
> Bluetooth: Report error for HCI reset ioctl when device is down
> Bluetooth: Fix error handling for HCI socket options
> Bluetooth: Restrict ioctls to HCI raw channel sockets
> Bluetooth: Introduce user channel flag for HCI devices
> Bluetooth: Introduce new HCI socket channel for user operation
> Bluetooth: Use devname:vhci module alias for virtual HCI driver
> Bluetooth: Add support creating virtual AMP controllers
> Bluetooth: Disable upper layer connections when user channel is active
> Bluetooth: Use GFP_KERNEL when cloning SKB in a workqueue
> Bluetooth: Only schedule raw queue when user channel is active
> Bluetooth: Use only 2 bits for controller type information
> Bluetooth: Replace BDADDR_LOCAL with BDADDR_NONE
> Bluetooth: Provide high speed configuration option
> Bluetooth: Send new settings event when changing high speed option
> Bluetooth: Require CAP_NET_ADMIN for HCI User Channel operation
> Bluetooth: Enable -D__CHECK_ENDIAN__ for sparse by default
> Bluetooth: Restrict disabling of HS when controller is powered off
> Bluetooth: Add management command for setting static address
> Bluetooth: Increment management interface revision
> Bluetooth: Fix memory leak with L2CAP signal channels
> Bluetooth: Restrict SSP setting changes to BR/EDR enabled controllers
> Bluetooth: Allow setting static address even if LE is disabled
> Bluetooth: Restrict loading of link keys to BR/EDR capable controllers
> Bluetooth: Restrict loading of long term keys to LE capable controllers
> Bluetooth: Allow changing device class when BR/EDR is disabled
> Bluetooth: Fix switch statement order for L2CAP fixed channels
> Bluetooth: Don't copy L2CAP LE signalling to raw sockets
> Bluetooth: SMP packets are only valid on LE connections
> Bluetooth: L2CAP connectionless channels are only valid for BR/EDR
> Bluetooth: Drop packets on ATT fixed channel on BR/EDR
> Bluetooth: Check minimum length of SMP packets
> Bluetooth: Only one command per L2CAP LE signalling is supported
>
> Peng Chen (1):
> Bluetooth: Add a new PID/VID 0cf3/e005 for AR3012.
>
> Peter Senna Tschudin (1):
> Bluetooth: Fix assignment of 0/1 to bool variables
>
> Raphael Kubo da Costa (1):
> Bluetooth: Add support for BCM20702A0 [0b05, 17cb]
>
> Syam Sidhardhan (1):
> Bluetooth: Fix ACL alive for long in case of non pariable devices
>
> drivers/bluetooth/Makefile | 2 +
> drivers/bluetooth/ath3k.c | 2 +
> drivers/bluetooth/btmrvl_drv.h | 12 +-
> drivers/bluetooth/btmrvl_main.c | 269 +++++++++++++++---------
> drivers/bluetooth/btmrvl_sdio.c | 15 +-
> drivers/bluetooth/btmrvl_sdio.h | 2 +
> drivers/bluetooth/btusb.c | 5 +
> drivers/bluetooth/hci_vhci.c | 170 ++++++++++-----
> include/net/bluetooth/bluetooth.h | 5 +-
> include/net/bluetooth/hci.h | 82 +++++++-
> include/net/bluetooth/hci_core.h | 2 +-
> include/net/bluetooth/l2cap.h | 1 +
> include/net/bluetooth/mgmt.h | 11 +
> net/bluetooth/Makefile | 2 +
> net/bluetooth/af_bluetooth.c | 41 ++++
> net/bluetooth/hci_conn.c | 4 +
> net/bluetooth/hci_core.c | 213 +++++++++++++++----
> net/bluetooth/hci_event.c | 23 ++-
> net/bluetooth/hci_sock.c | 204 ++++++++++++++----
> net/bluetooth/l2cap_core.c | 169 +++++++++------
> net/bluetooth/l2cap_sock.c | 20 +-
> net/bluetooth/mgmt.c | 562 ++++++++++++++++++++++++++++++++++++++++----------
> net/bluetooth/rfcomm/sock.c | 7 +-
> net/bluetooth/rfcomm/tty.c | 35 +---
> net/bluetooth/smp.c | 15 +-
> 25 files changed, 1390 insertions(+), 483 deletions(-)
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* RE: [PATCH v6 0/4] Bluetooth: btmrvl cal data downloading
From: Bing Zhao @ 2013-10-03 18:20 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <1EB87650-8518-47BE-B0D7-4F78E09F8691@holtmann.org>
Hi Marcel,
> I have decided to apply all 4 patches to bluetooth-next. However please s=
end a follow up patch that
> changes the code to operate on 16-bit opcodes and not the OGC/OCF and its=
packing.
I will send the follow-up patch shortly.
Thanks,
Bing
^ permalink raw reply
* Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler
From: Andre Guedes @ 2013-10-03 18:22 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <349C4B8E-04AA-4122-8CC5-7C4342FA767E@holtmann.org>
Hi Marcel,
On Thu, Oct 3, 2013 at 11:15 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andre,
>
>>>> This patch does some code refactorig in LE Connection Complete HCI
>>>> event handler. It basically adds a switch statement to separate new
>>>> master connection code from new slave connection code.
>>>>
>>>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>>>> ---
>>>> include/net/bluetooth/hci.h | 1 +
>>>> net/bluetooth/hci_event.c | 55 ++++++++++++++++++++++++++++++++-----=
--------
>>>> 2 files changed, 41 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>> index 7ede266..8c98f60 100644
>>>> --- a/include/net/bluetooth/hci.h
>>>> +++ b/include/net/bluetooth/hci.h
>>>> @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
>>>>
>>>> /* Low energy meta events */
>>>> #define LE_CONN_ROLE_MASTER 0x00
>>>> +#define LE_CONN_ROLE_SLAVE 0x01
>>>>
>>>> #define HCI_EV_LE_CONN_COMPLETE 0x01
>>>> struct hci_ev_le_conn_complete {
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index 1d1ffa6..0e4a9f4 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci=
_dev *hdev, struct sk_buff *skb)
>>>>
>>>> hci_dev_lock(hdev);
>>>>
>>>> - conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>>>> - if (!conn) {
>>>> + if (ev->status) {
>>>> + conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CO=
NNECT);
>>>> + if (!conn)
>>>> + goto unlock;
>>>> +
>>>> + mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>>> + conn->dst_type, ev->status);
>>>> + hci_proto_connect_cfm(conn, ev->status);
>>>> + conn->state =3D BT_CLOSED;
>>>> + hci_conn_del(conn);
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + switch (ev->role) {
>>>> + case LE_CONN_ROLE_MASTER:
>>>> + conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bda=
ddr);
>>>> + /* If there is no hci_conn object with the given address=
, it
>>>> + * means this new connection was triggered through HCI s=
ocket
>>>> + * interface. For that case, we should create a new hci_=
conn
>>>> + * object.
>>>> + */
>>>
>>> this comments belong one level down inside the if block. You already co=
mmenting on the negative outcome of the if check.
>>
>> Ok, I'll fix it.
>>
>>>
>>>
>>>> + if (!conn) {
>>>> + conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr=
);
>>>> + if (!conn) {
>>>> + BT_ERR("No memory for new connection");
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + conn->out =3D true;
>>>> + conn->link_mode |=3D HCI_LM_MASTER;
>>>> + conn->sec_level =3D BT_SECURITY_LOW;
>>>> + conn->dst_type =3D ev->bdaddr_type;
>>>> + }
>>>> + break;
>>>> +
>>>> + case LE_CONN_ROLE_SLAVE:
>>>
>>> And why are we not checking for an existing connection here? At least a=
small comment is needed to make that part clear.
>>
>> Differently from master connection, there is no existing hci_conn for
>> slave connections. For that reason we don't check for an existing
>> connection here. I'll add a comment.
>>
>>>
>>>
>>>> conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
>>>> if (!conn) {
>>>> BT_ERR("No memory for new connection");
>>>> @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hc=
i_dev *hdev, struct sk_buff *skb)
>>>> }
>>>>
>>>> conn->dst_type =3D ev->bdaddr_type;
>>>> + conn->sec_level =3D BT_SECURITY_LOW;
>>>> + break;
>>>>
>>>> - if (ev->role =3D=3D LE_CONN_ROLE_MASTER) {
>>>> - conn->out =3D true;
>>>> - conn->link_mode |=3D HCI_LM_MASTER;
>>>> - }
>>>> - }
>>>> -
>>>> - if (ev->status) {
>>>> - mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>>> - conn->dst_type, ev->status);
>>>> - hci_proto_connect_cfm(conn, ev->status);
>>>> - conn->state =3D BT_CLOSED;
>>>> - hci_conn_del(conn);
>>>> + default:
>>>> + BT_ERR("Used reserved Role parameter %d", ev->role);
>>>> goto unlock;
>>>> }
>>>>
>>>> @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_=
dev *hdev, struct sk_buff *skb)
>>>> mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
>>>> conn->dst_type, 0, NULL, 0, NULL);
>>>>
>>>> - conn->sec_level =3D BT_SECURITY_LOW;
>>>> conn->handle =3D __le16_to_cpu(ev->handle);
>>>> conn->state =3D BT_CONNECTED;
>>>
>>> All in all, I am not really understanding why this makes it this code s=
impler. I actually think it turns it into more complicated code. So please =
explain what we are really gaining here. I just see more hash table lookup =
and for hci_conn_add calls with more error checks.
>>
>> When controller sends a LE Connection Complete event to host, we have
>> three different handling: failure, new master connection and new slave
>> connection. Additionally, new master connection has a special handling
>> since the connection could be triggered by HCI socket interface.
>> Before, all these logic were mixed up, making hard to add specific
>> code for new master connection for instance.
>
> I do not follow the difference between master and slave roles. Why do we =
have a difference here in the first place.
>
>> So this patch explicitly separates the three different handling and
>> adds extra comments aiming to improve hci_le_conn_complete_evt()
>> readability. Besides that, keeping these different handling separated,
>> it'll be easier to add the auto connection hooks.
>
> I am getting the feeling the overall code gets more complicated than simp=
ler. The goal is to make the connection handling simpler. And right now, I =
do not see this.
Ok, I'll drop this patch in v2.
I'll recheck if we really need to differentiate between master and
slave new connection for auto connection. If positive, I can add this
patch to auto connection patchset.
Thanks,
Andre
^ permalink raw reply
* [PATCH] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf
From: Bing Zhao @ 2013-10-03 18:23 UTC (permalink / raw)
To: linux-bluetooth
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
Amitkumar Karwar, Bing Zhao
Replace ogf/ocf and its packing with 16-bit opcodes.
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index f9d1833..e3b49c6 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -90,12 +90,12 @@ struct btmrvl_private {
#define MRVL_VENDOR_PKT 0xFE
-/* Bluetooth commands */
-#define BT_CMD_AUTO_SLEEP_MODE 0x23
-#define BT_CMD_HOST_SLEEP_CONFIG 0x59
-#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
-#define BT_CMD_MODULE_CFG_REQ 0x5B
-#define BT_CMD_LOAD_CONFIG_DATA 0x61
+/* Vendor specific Bluetooth commands */
+#define BT_CMD_AUTO_SLEEP_MODE 0xFC23
+#define BT_CMD_HOST_SLEEP_CONFIG 0xFC59
+#define BT_CMD_HOST_SLEEP_ENABLE 0xFC5A
+#define BT_CMD_MODULE_CFG_REQ 0xFC5B
+#define BT_CMD_LOAD_CONFIG_DATA 0xFC61
/* Sub-commands: Module Bringup/Shutdown Request/Response */
#define MODULE_BRINGUP_REQ 0xF1
@@ -104,6 +104,11 @@ struct btmrvl_private {
#define MODULE_SHUTDOWN_REQ 0xF2
+/* Vendor specific Bluetooth events */
+#define BT_EVENT_AUTO_SLEEP_MODE 0x23
+#define BT_EVENT_HOST_SLEEP_CONFIG 0x59
+#define BT_EVENT_HOST_SLEEP_ENABLE 0x5A
+#define BT_EVENT_MODULE_CFG_REQ 0x5B
#define BT_EVENT_POWER_STATE 0x20
/* Bluetooth Power States */
@@ -111,8 +116,6 @@ struct btmrvl_private {
#define BT_PS_DISABLE 0x03
#define BT_PS_SLEEP 0x01
-#define OGF 0x3F
-
/* Host Sleep states */
#define HS_ACTIVATED 0x01
#define HS_DEACTIVATED 0x00
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 6e7bd4e..b3dcb13 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -50,12 +50,10 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
if (hdr->evt == HCI_EV_CMD_COMPLETE) {
struct hci_ev_cmd_complete *ec;
- u16 opcode, ocf, ogf;
+ u16 opcode;
ec = (void *) (skb->data + HCI_EVENT_HDR_SIZE);
opcode = __le16_to_cpu(ec->opcode);
- ocf = hci_opcode_ocf(opcode);
- ogf = hci_opcode_ogf(opcode);
if (priv->btmrvl_dev.sendcmdflag) {
priv->btmrvl_dev.sendcmdflag = false;
@@ -63,9 +61,8 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
wake_up_interruptible(&priv->adapter->cmd_wait_q);
}
- if (ogf == OGF) {
- BT_DBG("vendor event skipped: ogf 0x%4.4x ocf 0x%4.4x",
- ogf, ocf);
+ if ((opcode & 0xfc00) == 0xfc00) {
+ BT_DBG("vendor event skipped: opcode=%#4.4x", opcode);
kfree_skb(skb);
return false;
}
@@ -89,7 +86,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
switch (event->data[0]) {
- case BT_CMD_AUTO_SLEEP_MODE:
+ case BT_EVENT_AUTO_SLEEP_MODE:
if (!event->data[2]) {
if (event->data[1] == BT_PS_ENABLE)
adapter->psmode = 1;
@@ -102,7 +99,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;
- case BT_CMD_HOST_SLEEP_CONFIG:
+ case BT_EVENT_HOST_SLEEP_CONFIG:
if (!event->data[3])
BT_DBG("gpio=%x, gap=%x", event->data[1],
event->data[2]);
@@ -110,7 +107,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
BT_DBG("HSCFG command failed");
break;
- case BT_CMD_HOST_SLEEP_ENABLE:
+ case BT_EVENT_HOST_SLEEP_ENABLE:
if (!event->data[1]) {
adapter->hs_state = HS_ACTIVATED;
if (adapter->psmode)
@@ -121,7 +118,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;
- case BT_CMD_MODULE_CFG_REQ:
+ case BT_EVENT_MODULE_CFG_REQ:
if (priv->btmrvl_dev.sendcmdflag &&
event->data[1] == MODULE_BRINGUP_REQ) {
BT_DBG("EVENT:%s",
@@ -166,7 +163,7 @@ exit:
}
EXPORT_SYMBOL_GPL(btmrvl_process_event);
-static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
+static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 opcode,
const void *param, u8 len)
{
struct sk_buff *skb;
@@ -179,7 +176,7 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
}
hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
- hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
+ hdr->opcode = cpu_to_le16(opcode);
hdr->plen = len;
if (len)
--
1.8.0
^ permalink raw reply related
* Re: pull request: bluetooth-next 2013-10-03
From: John W. Linville @ 2013-10-03 20:19 UTC (permalink / raw)
To: Gustavo Padovan, linux-wireless, linux-bluetooth, linux-kernel
In-Reply-To: <20131003174524.GA22728@joana>
On Thu, Oct 03, 2013 at 02:45:24PM -0300, Gustavo Padovan wrote:
> Hi John,
>
> A series of patches for 3.12. The big work here is from Marcel and Johan. They
> did a lot of work in the L2CAP, HCI and MGMT layers. The most important ones
> are the addition of a new MGMT command to enable/disable LE advertisement and
> the introduction of the HCI user channel to allow applications to get directly
> and exclusive access to Bluetooth devices.
>
> Please pull, or let me know of any issues. Thanks!
>
> Gustavo
>
> --
> The following changes since commit f4e1a4d3ecbb9e42bdf8e7869ee8a4ebfa27fb20:
>
> rt2800: change initialization sequence to fix system freeze (2013-09-09 14:44:34 -0400)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next for-upstream
>
> for you to fetch changes up to 4f3e219d95a3c31b916dcd5e2631c4e440736f79:
>
> Bluetooth: Only one command per L2CAP LE signalling is supported (2013-10-03 16:09:59 +0300)
Pulling now...
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: [PATCH] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf
From: Anderson Lizardo @ 2013-10-03 20:34 UTC (permalink / raw)
To: Bing Zhao
Cc: BlueZ development, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, linux-wireless, Amitkumar Karwar
In-Reply-To: <1380824600-13655-1-git-send-email-bzhao@marvell.com>
Hi Bing,
On Thu, Oct 3, 2013 at 2:23 PM, Bing Zhao <bzhao@marvell.com> wrote:
> @@ -63,9 +61,8 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
> wake_up_interruptible(&priv->adapter->cmd_wait_q);
> }
>
> - if (ogf == OGF) {
> - BT_DBG("vendor event skipped: ogf 0x%4.4x ocf 0x%4.4x",
> - ogf, ocf);
> + if ((opcode & 0xfc00) == 0xfc00) {
> + BT_DBG("vendor event skipped: opcode=%#4.4x", opcode);
I think you could use "if (hci_opcode_ogf(opcode) == 0x3F)" to make it
more readable.
> @@ -166,7 +163,7 @@ exit:
> }
> EXPORT_SYMBOL_GPL(btmrvl_process_event);
>
> -static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
> +static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 opcode,
> const void *param, u8 len)
> {
> struct sk_buff *skb;
> @@ -179,7 +176,7 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
> }
>
> hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
> - hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
> + hdr->opcode = cpu_to_le16(opcode);
Are you sure the callers of btmrvl_send_sync_cmd() do not need to be
changed to pass an opcode instead of just the OCF?
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* RE: [PATCH] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf
From: Bing Zhao @ 2013-10-03 21:06 UTC (permalink / raw)
To: Anderson Lizardo
Cc: BlueZ development, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, linux-wireless@vger.kernel.org, Amitkumar Karwar
In-Reply-To: <CAJdJm_N1=a7URKmU8xOsOzOaYHKQdNXiR7VryeOc4+i2uf7RqQ@mail.gmail.com>
Hi Anderson,
Thanks for your comments.
> > - if (ogf =3D=3D OGF) {
> > - BT_DBG("vendor event skipped: ogf 0x%4.4x ocf 0=
x%4.4x",
> > - ogf, ocf);
> > + if ((opcode & 0xfc00) =3D=3D 0xfc00) {
> > + BT_DBG("vendor event skipped: opcode=3D%#4.4x",=
opcode);
>=20
> I think you could use "if (hci_opcode_ogf(opcode) =3D=3D 0x3F)" to make i=
t
> more readable.
Sure, I will make that change in v2.
> > @@ -179,7 +176,7 @@ static int btmrvl_send_sync_cmd(struct btmrvl_priva=
te *priv, u16 cmd_no,
> > }
> >
> > hdr =3D (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_=
SIZE);
> > - hdr->opcode =3D cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
> > + hdr->opcode =3D cpu_to_le16(opcode);
>=20
> Are you sure the callers of btmrvl_send_sync_cmd() do not need to be
> changed to pass an opcode instead of just the OCF?
Previously we pass the cmd_no which is the OCF bits to the function, and th=
e function packs it to opcode.
Now I changed the macros of the cmd_no from OCF to opcode as shown below, s=
o no change to the callers.
-/* Bluetooth commands */
-#define BT_CMD_AUTO_SLEEP_MODE 0x23
-#define BT_CMD_HOST_SLEEP_CONFIG 0x59
-#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
-#define BT_CMD_MODULE_CFG_REQ 0x5B
-#define BT_CMD_LOAD_CONFIG_DATA 0x61
+/* Vendor specific Bluetooth commands */
+#define BT_CMD_AUTO_SLEEP_MODE 0xFC23
+#define BT_CMD_HOST_SLEEP_CONFIG 0xFC59
+#define BT_CMD_HOST_SLEEP_ENABLE 0xFC5A
+#define BT_CMD_MODULE_CFG_REQ 0xFC5B
+#define BT_CMD_LOAD_CONFIG_DATA 0xFC61
Thanks,
Bing
^ permalink raw reply
* [PATCH v2] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf
From: Bing Zhao @ 2013-10-03 21:19 UTC (permalink / raw)
To: linux-bluetooth
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, Anderson Lizardo,
linux-wireless, Amitkumar Karwar, Bing Zhao
Replace ogf/ocf and its packing with 16-bit opcodes.
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Use hci_opcode_ogf() to make code more readable. (Anderson Lizardo)
drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index f9d1833..e3b49c6 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -90,12 +90,12 @@ struct btmrvl_private {
#define MRVL_VENDOR_PKT 0xFE
-/* Bluetooth commands */
-#define BT_CMD_AUTO_SLEEP_MODE 0x23
-#define BT_CMD_HOST_SLEEP_CONFIG 0x59
-#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
-#define BT_CMD_MODULE_CFG_REQ 0x5B
-#define BT_CMD_LOAD_CONFIG_DATA 0x61
+/* Vendor specific Bluetooth commands */
+#define BT_CMD_AUTO_SLEEP_MODE 0xFC23
+#define BT_CMD_HOST_SLEEP_CONFIG 0xFC59
+#define BT_CMD_HOST_SLEEP_ENABLE 0xFC5A
+#define BT_CMD_MODULE_CFG_REQ 0xFC5B
+#define BT_CMD_LOAD_CONFIG_DATA 0xFC61
/* Sub-commands: Module Bringup/Shutdown Request/Response */
#define MODULE_BRINGUP_REQ 0xF1
@@ -104,6 +104,11 @@ struct btmrvl_private {
#define MODULE_SHUTDOWN_REQ 0xF2
+/* Vendor specific Bluetooth events */
+#define BT_EVENT_AUTO_SLEEP_MODE 0x23
+#define BT_EVENT_HOST_SLEEP_CONFIG 0x59
+#define BT_EVENT_HOST_SLEEP_ENABLE 0x5A
+#define BT_EVENT_MODULE_CFG_REQ 0x5B
#define BT_EVENT_POWER_STATE 0x20
/* Bluetooth Power States */
@@ -111,8 +116,6 @@ struct btmrvl_private {
#define BT_PS_DISABLE 0x03
#define BT_PS_SLEEP 0x01
-#define OGF 0x3F
-
/* Host Sleep states */
#define HS_ACTIVATED 0x01
#define HS_DEACTIVATED 0x00
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 6e7bd4e..ffec74e 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -50,12 +50,10 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
if (hdr->evt == HCI_EV_CMD_COMPLETE) {
struct hci_ev_cmd_complete *ec;
- u16 opcode, ocf, ogf;
+ u16 opcode;
ec = (void *) (skb->data + HCI_EVENT_HDR_SIZE);
opcode = __le16_to_cpu(ec->opcode);
- ocf = hci_opcode_ocf(opcode);
- ogf = hci_opcode_ogf(opcode);
if (priv->btmrvl_dev.sendcmdflag) {
priv->btmrvl_dev.sendcmdflag = false;
@@ -63,9 +61,8 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
wake_up_interruptible(&priv->adapter->cmd_wait_q);
}
- if (ogf == OGF) {
- BT_DBG("vendor event skipped: ogf 0x%4.4x ocf 0x%4.4x",
- ogf, ocf);
+ if (hci_opcode_ogf(opcode) == 0x3F) {
+ BT_DBG("vendor event skipped: opcode=%#4.4x", opcode);
kfree_skb(skb);
return false;
}
@@ -89,7 +86,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
switch (event->data[0]) {
- case BT_CMD_AUTO_SLEEP_MODE:
+ case BT_EVENT_AUTO_SLEEP_MODE:
if (!event->data[2]) {
if (event->data[1] == BT_PS_ENABLE)
adapter->psmode = 1;
@@ -102,7 +99,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;
- case BT_CMD_HOST_SLEEP_CONFIG:
+ case BT_EVENT_HOST_SLEEP_CONFIG:
if (!event->data[3])
BT_DBG("gpio=%x, gap=%x", event->data[1],
event->data[2]);
@@ -110,7 +107,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
BT_DBG("HSCFG command failed");
break;
- case BT_CMD_HOST_SLEEP_ENABLE:
+ case BT_EVENT_HOST_SLEEP_ENABLE:
if (!event->data[1]) {
adapter->hs_state = HS_ACTIVATED;
if (adapter->psmode)
@@ -121,7 +118,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;
- case BT_CMD_MODULE_CFG_REQ:
+ case BT_EVENT_MODULE_CFG_REQ:
if (priv->btmrvl_dev.sendcmdflag &&
event->data[1] == MODULE_BRINGUP_REQ) {
BT_DBG("EVENT:%s",
@@ -166,7 +163,7 @@ exit:
}
EXPORT_SYMBOL_GPL(btmrvl_process_event);
-static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
+static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 opcode,
const void *param, u8 len)
{
struct sk_buff *skb;
@@ -179,7 +176,7 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
}
hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
- hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
+ hdr->opcode = cpu_to_le16(opcode);
hdr->plen = len;
if (len)
--
1.8.0
^ permalink raw reply related
* [PATCH v2 0/4] LE connection refactoring and fixes
From: Andre Guedes @ 2013-10-03 21:25 UTC (permalink / raw)
To: linux-bluetooth
Hi all,
This v2 takes in consideration comments from Marcel and Lizardo. This patchset
is smaller since I squashed 3 patches and removed the last two patches from
the previous patchset.
Regards,
Andre
Andre Guedes (4):
Bluetooth: Rename hci_conn variable in hci_connect_le()
Bluetooth: Initialize hci_conn fields in hci_connect_le
Bluetooth: Use HCI request for LE connection
Bluetooth: Refactor hci_connect_le
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_conn.c | 81 ++++++++++++++++++++--------------------
net/bluetooth/hci_core.c | 46 +++++++++++++++++++++++
net/bluetooth/hci_event.c | 31 ---------------
4 files changed, 89 insertions(+), 71 deletions(-)
--
1.8.4
^ permalink raw reply
* [PATCH v2 1/4] Bluetooth: Rename hci_conn variable in hci_connect_le()
From: Andre Guedes @ 2013-10-03 21:25 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380835547-3640-1-git-send-email-andre.guedes@openbossa.org>
This patch simply rename the hci_conn variable "le" to "conn"
since it is a better name.
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
net/bluetooth/hci_conn.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 514148b..5aba520 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -549,31 +549,31 @@ EXPORT_SYMBOL(hci_get_route);
static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, u8 sec_level, u8 auth_type)
{
- struct hci_conn *le;
+ struct hci_conn *conn;
if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
return ERR_PTR(-ENOTSUPP);
- le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
- if (!le) {
- le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
- if (le)
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
+ if (!conn) {
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (conn)
return ERR_PTR(-EBUSY);
- le = hci_conn_add(hdev, LE_LINK, dst);
- if (!le)
+ conn = hci_conn_add(hdev, LE_LINK, dst);
+ if (!conn)
return ERR_PTR(-ENOMEM);
- le->dst_type = bdaddr_to_le(dst_type);
- hci_le_create_connection(le);
+ conn->dst_type = bdaddr_to_le(dst_type);
+ hci_le_create_connection(conn);
}
- le->pending_sec_level = sec_level;
- le->auth_type = auth_type;
+ conn->pending_sec_level = sec_level;
+ conn->auth_type = auth_type;
- hci_conn_hold(le);
+ hci_conn_hold(conn);
- return le;
+ return conn;
}
static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
--
1.8.4
^ permalink raw reply related
* [PATCH v2 2/4] Bluetooth: Initialize hci_conn fields in hci_connect_le
From: Andre Guedes @ 2013-10-03 21:25 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380835547-3640-1-git-send-email-andre.guedes@openbossa.org>
This patch moves some hci_conn fields initialization from hci_le_
create_connection() to hci_connect_le(). It makes more sense to
initialize these fields within the function that creates the hci_
conn object.
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
net/bluetooth/hci_conn.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 5aba520..08e601c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
struct hci_dev *hdev = conn->hdev;
struct hci_cp_le_create_conn cp;
- conn->state = BT_CONNECT;
- conn->out = true;
- conn->link_mode |= HCI_LM_MASTER;
- conn->sec_level = BT_SECURITY_LOW;
-
memset(&cp, 0, sizeof(cp));
cp.scan_interval = __constant_cpu_to_le16(0x0060);
cp.scan_window = __constant_cpu_to_le16(0x0030);
@@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
return ERR_PTR(-ENOMEM);
conn->dst_type = bdaddr_to_le(dst_type);
+ conn->state = BT_CONNECT;
+ conn->out = true;
+ conn->link_mode |= HCI_LM_MASTER;
+ conn->sec_level = BT_SECURITY_LOW;
+
hci_le_create_connection(conn);
}
--
1.8.4
^ permalink raw reply related
* [PATCH v2 3/4] Bluetooth: Use HCI request for LE connection
From: Andre Guedes @ 2013-10-03 21:25 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380835547-3640-1-git-send-email-andre.guedes@openbossa.org>
This patch introduces a new helper, which uses the HCI request
framework, for creating LE connectons. All the handling is now
done by this function so we can remove the hci_cs_le_create_conn()
event handler.
This patch also removes the old hci_le_create_connection() since
it is not used anymore.
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_conn.c | 26 ++++++-----------------
net/bluetooth/hci_core.c | 46 ++++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_event.c | 31 ---------------------------
4 files changed, 54 insertions(+), 51 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e09c305..0835cf9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1217,6 +1217,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
u8 bdaddr_to_le(u8 bdaddr_type);
+int hci_create_le_conn(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
+
#define SCO_AIRMODE_MASK 0x0003
#define SCO_AIRMODE_CVSD 0x0000
#define SCO_AIRMODE_TRANSP 0x0003
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 08e601c..cb0e5d7 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
{ EDR_ESCO_MASK | ESCO_EV3, 0x0008 }, /* T1 */
};
-static void hci_le_create_connection(struct hci_conn *conn)
-{
- struct hci_dev *hdev = conn->hdev;
- struct hci_cp_le_create_conn cp;
-
- memset(&cp, 0, sizeof(cp));
- cp.scan_interval = __constant_cpu_to_le16(0x0060);
- cp.scan_window = __constant_cpu_to_le16(0x0030);
- bacpy(&cp.peer_addr, &conn->dst);
- cp.peer_addr_type = conn->dst_type;
- cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
- cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
- cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
- cp.min_ce_len = __constant_cpu_to_le16(0x0000);
- cp.max_ce_len = __constant_cpu_to_le16(0x0000);
-
- hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
-}
-
static void hci_le_create_connection_cancel(struct hci_conn *conn)
{
hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
@@ -545,6 +526,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, u8 sec_level, u8 auth_type)
{
struct hci_conn *conn;
+ int err;
if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
return ERR_PTR(-ENOTSUPP);
@@ -565,7 +547,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
conn->link_mode |= HCI_LM_MASTER;
conn->sec_level = BT_SECURITY_LOW;
- hci_le_create_connection(conn);
+ err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
+ if (err) {
+ hci_conn_del(conn);
+ return ERR_PTR(err);
+ }
}
conn->pending_sec_level = sec_level;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 82dbdc6..1002510 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3662,3 +3662,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
return ADDR_LE_DEV_RANDOM;
}
}
+
+static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
+{
+ struct hci_conn *conn;
+
+ if (status == 0)
+ return;
+
+ BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
+ status);
+
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (!conn)
+ return;
+
+ mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
+ status);
+
+ hci_proto_connect_cfm(conn, status);
+
+ hci_dev_lock(hdev);
+ hci_conn_del(conn);
+ hci_dev_unlock(hdev);
+}
+
+int hci_create_le_conn(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
+{
+ struct hci_cp_le_create_conn cp;
+ struct hci_request req;
+
+ hci_req_init(&req, hdev);
+
+ memset(&cp, 0, sizeof(cp));
+ cp.scan_interval = __constant_cpu_to_le16(0x0060);
+ cp.scan_window = __constant_cpu_to_le16(0x0030);
+ bacpy(&cp.peer_addr, addr);
+ cp.peer_addr_type = type;
+ cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
+ cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
+ cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
+ cp.min_ce_len = __constant_cpu_to_le16(0x0000);
+ cp.max_ce_len = __constant_cpu_to_le16(0x0000);
+ hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
+
+ return hci_req_run(&req, create_le_conn_complete);
+}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4785ab0..a6c6c17 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1470,33 +1470,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
hci_dev_unlock(hdev);
}
-static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
-{
- struct hci_conn *conn;
-
- BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
- if (status) {
- hci_dev_lock(hdev);
-
- conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
- if (!conn) {
- hci_dev_unlock(hdev);
- return;
- }
-
- BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
-
- conn->state = BT_CLOSED;
- mgmt_connect_failed(hdev, &conn->dst, conn->type,
- conn->dst_type, status);
- hci_proto_connect_cfm(conn, status);
- hci_conn_del(conn);
-
- hci_dev_unlock(hdev);
- }
-}
-
static void hci_cs_create_phylink(struct hci_dev *hdev, u8 status)
{
struct hci_cp_create_phy_link *cp;
@@ -2347,10 +2320,6 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
hci_cs_disconnect(hdev, ev->status);
break;
- case HCI_OP_LE_CREATE_CONN:
- hci_cs_le_create_conn(hdev, ev->status);
- break;
-
case HCI_OP_CREATE_PHY_LINK:
hci_cs_create_phylink(hdev, ev->status);
break;
--
1.8.4
^ permalink raw reply related
* [PATCH v2 4/4] Bluetooth: Refactor hci_connect_le
From: Andre Guedes @ 2013-10-03 21:25 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380835547-3640-1-git-send-email-andre.guedes@openbossa.org>
This patch does some code refactoring in hci_connect_le() by moving
the exception code into if statements and letting the main flow in
first level of function scope. It also adds extra comments to improve
the code readability.
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
net/bluetooth/hci_conn.c | 55 ++++++++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cb0e5d7..ff15841 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -531,34 +531,49 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
return ERR_PTR(-ENOTSUPP);
+ /* XXX: Some devices send ATT messages as soon as the physical link
+ * is established. To be able to handle these ATT messages, the user-
+ * space first establishes the connection and then starts the pairing
+ * process.
+ *
+ * So if a hci_conn object already exists for the following connection
+ * attempt, we simply update pending_sec_level and auth_type fields
+ * and return the object found.
+ */
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
- if (!conn) {
- conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
- if (conn)
- return ERR_PTR(-EBUSY);
-
- conn = hci_conn_add(hdev, LE_LINK, dst);
- if (!conn)
- return ERR_PTR(-ENOMEM);
+ if (conn) {
+ conn->pending_sec_level = sec_level;
+ conn->auth_type = auth_type;
+ goto out;
+ }
- conn->dst_type = bdaddr_to_le(dst_type);
- conn->state = BT_CONNECT;
- conn->out = true;
- conn->link_mode |= HCI_LM_MASTER;
- conn->sec_level = BT_SECURITY_LOW;
+ /* Since the controller supports only one LE connection attempt at a
+ * time, we return -EBUSY if there is any connection attempt running.
+ */
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (conn)
+ return ERR_PTR(-EBUSY);
- err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
- if (err) {
- hci_conn_del(conn);
- return ERR_PTR(err);
- }
- }
+ conn = hci_conn_add(hdev, LE_LINK, dst);
+ if (!conn)
+ return ERR_PTR(-ENOMEM);
+ conn->dst_type = bdaddr_to_le(dst_type);
+ conn->state = BT_CONNECT;
+ conn->out = true;
+ conn->link_mode |= HCI_LM_MASTER;
+ conn->sec_level = BT_SECURITY_LOW;
conn->pending_sec_level = sec_level;
conn->auth_type = auth_type;
- hci_conn_hold(conn);
+ err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
+ if (err) {
+ hci_conn_del(conn);
+ return ERR_PTR(err);
+ }
+out:
+ hci_conn_hold(conn);
return conn;
}
--
1.8.4
^ permalink raw reply related
* Re: RE: [PATCH v6 0/4] Bluetooth: btmrvl cal data downloading
From: 이혁주 @ 2013-10-03 22:52 UTC (permalink / raw)
To: Bing Zhao, Marcel Holtmann, 이원준,
정종필
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Amitkumar Karwar
[-- Attachment #1: Type: text/html, Size: 5289 bytes --]
[-- Attachment #2: 201310040752684_BEI0XT4N.gif --]
[-- Type: image/gif, Size: 14036 bytes --]
^ permalink raw reply
* Re: [PATCH] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf
From: Anderson Lizardo @ 2013-10-03 23:26 UTC (permalink / raw)
To: Bing Zhao
Cc: BlueZ development, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, linux-wireless@vger.kernel.org, Amitkumar Karwar
In-Reply-To: <477F20668A386D41ADCC57781B1F70430F451FB266@SC-VEXCH1.marvell.com>
Hi Bing,
On Thu, Oct 3, 2013 at 5:06 PM, Bing Zhao <bzhao@marvell.com> wrote:
>> > hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
>> > - hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
>> > + hdr->opcode = cpu_to_le16(opcode);
>>
>> Are you sure the callers of btmrvl_send_sync_cmd() do not need to be
>> changed to pass an opcode instead of just the OCF?
>
> Previously we pass the cmd_no which is the OCF bits to the function, and the function packs it to opcode.
>
> Now I changed the macros of the cmd_no from OCF to opcode as shown below, so no change to the callers.
>
> -/* Bluetooth commands */
> -#define BT_CMD_AUTO_SLEEP_MODE 0x23
> -#define BT_CMD_HOST_SLEEP_CONFIG 0x59
> -#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
> -#define BT_CMD_MODULE_CFG_REQ 0x5B
> -#define BT_CMD_LOAD_CONFIG_DATA 0x61
> +/* Vendor specific Bluetooth commands */
> +#define BT_CMD_AUTO_SLEEP_MODE 0xFC23
> +#define BT_CMD_HOST_SLEEP_CONFIG 0xFC59
> +#define BT_CMD_HOST_SLEEP_ENABLE 0xFC5A
> +#define BT_CMD_MODULE_CFG_REQ 0xFC5B
> +#define BT_CMD_LOAD_CONFIG_DATA 0xFC61
Now I got it :)
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH v2 1/4] Bluetooth: Rename hci_conn variable in hci_connect_le()
From: Marcel Holtmann @ 2013-10-04 6:08 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380835547-3640-2-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch simply rename the hci_conn variable "le" to "conn"
> since it is a better name.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2 2/4] Bluetooth: Initialize hci_conn fields in hci_connect_le
From: Marcel Holtmann @ 2013-10-04 6:09 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380835547-3640-3-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch moves some hci_conn fields initialization from hci_le_
> create_connection() to hci_connect_le(). It makes more sense to
> initialize these fields within the function that creates the hci_
> conn object.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2 3/4] Bluetooth: Use HCI request for LE connection
From: Marcel Holtmann @ 2013-10-04 6:25 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380835547-3640-4-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch introduces a new helper, which uses the HCI request
> framework, for creating LE connectons. All the handling is now
> done by this function so we can remove the hci_cs_le_create_conn()
> event handler.
>
> This patch also removes the old hci_le_create_connection() since
> it is not used anymore.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_conn.c | 26 ++++++-----------------
> net/bluetooth/hci_core.c | 46 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c | 31 ---------------------------
> 4 files changed, 54 insertions(+), 51 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e09c305..0835cf9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1217,6 +1217,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
>
> u8 bdaddr_to_le(u8 bdaddr_type);
>
> +int hci_create_le_conn(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
> +
> #define SCO_AIRMODE_MASK 0x0003
> #define SCO_AIRMODE_CVSD 0x0000
> #define SCO_AIRMODE_TRANSP 0x0003
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 08e601c..cb0e5d7 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
> { EDR_ESCO_MASK | ESCO_EV3, 0x0008 }, /* T1 */
> };
>
> -static void hci_le_create_connection(struct hci_conn *conn)
> -{
> - struct hci_dev *hdev = conn->hdev;
> - struct hci_cp_le_create_conn cp;
> -
> - memset(&cp, 0, sizeof(cp));
> - cp.scan_interval = __constant_cpu_to_le16(0x0060);
> - cp.scan_window = __constant_cpu_to_le16(0x0030);
> - bacpy(&cp.peer_addr, &conn->dst);
> - cp.peer_addr_type = conn->dst_type;
> - cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> - cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> - cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> - cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> - cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> -
> - hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> -}
> -
> static void hci_le_create_connection_cancel(struct hci_conn *conn)
> {
> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
> @@ -545,6 +526,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> u8 dst_type, u8 sec_level, u8 auth_type)
> {
> struct hci_conn *conn;
> + int err;
>
> if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> return ERR_PTR(-ENOTSUPP);
> @@ -565,7 +547,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> conn->link_mode |= HCI_LM_MASTER;
> conn->sec_level = BT_SECURITY_LOW;
>
> - hci_le_create_connection(conn);
> + err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
> + if (err) {
> + hci_conn_del(conn);
> + return ERR_PTR(err);
> + }
it is safe to sleep here? Since that is what the new call is doing.
> }
>
> conn->pending_sec_level = sec_level;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 82dbdc6..1002510 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3662,3 +3662,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
> return ADDR_LE_DEV_RANDOM;
> }
> }
> +
> +static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
> +{
> + struct hci_conn *conn;
> +
> + if (status == 0)
> + return;
> +
> + BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
> + status);
This should say "create" instead of "initiate".
> +
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (!conn)
> + return;
> +
We are no longer setting conn->state to BT_CLOSED on purpose here?
> + mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
> + status);
> +
> + hci_proto_connect_cfm(conn, status);
> +
> + hci_dev_lock(hdev);
> + hci_conn_del(conn);
> + hci_dev_unlock(hdev);
> +}
You are changing the locking behavior without any explanation. Previous code has it around the whole section. I am not a huge fan of changes that get sneaked in this way. That should be a separate patch to explain why it is correct. This patch should not change the current behavior.
> +
> +int hci_create_le_conn(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
> +{
> + struct hci_cp_le_create_conn cp;
> + struct hci_request req;
> +
> + hci_req_init(&req, hdev);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.scan_interval = __constant_cpu_to_le16(0x0060);
> + cp.scan_window = __constant_cpu_to_le16(0x0030);
> + bacpy(&cp.peer_addr, addr);
> + cp.peer_addr_type = type;
> + cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> + cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> + cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> + cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> + cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> + hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> +
> + return hci_req_run(&req, create_le_conn_complete);
> +}
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2 4/4] Bluetooth: Refactor hci_connect_le
From: Marcel Holtmann @ 2013-10-04 6:27 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380835547-3640-5-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch does some code refactoring in hci_connect_le() by moving
> the exception code into if statements and letting the main flow in
> first level of function scope. It also adds extra comments to improve
> the code readability.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 55 ++++++++++++++++++++++++++++++------------------
> 1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index cb0e5d7..ff15841 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -531,34 +531,49 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> return ERR_PTR(-ENOTSUPP);
>
> + /* XXX: Some devices send ATT messages as soon as the physical link
no idea what the XXX wants to tell me here.
> + * is established. To be able to handle these ATT messages, the user-
> + * space first establishes the connection and then starts the pairing
> + * process.
> + *
> + * So if a hci_conn object already exists for the following connection
> + * attempt, we simply update pending_sec_level and auth_type fields
> + * and return the object found.
> + */
> conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> - if (!conn) {
> - conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> - if (conn)
> - return ERR_PTR(-EBUSY);
> -
> - conn = hci_conn_add(hdev, LE_LINK, dst);
> - if (!conn)
> - return ERR_PTR(-ENOMEM);
> + if (conn) {
> + conn->pending_sec_level = sec_level;
> + conn->auth_type = auth_type;
> + goto out;
> + }
Call such a label "done". I never really liked the out part.
If anybody wants to fix the few users of "out" as label, that would be welcome.
>
> - conn->dst_type = bdaddr_to_le(dst_type);
> - conn->state = BT_CONNECT;
> - conn->out = true;
> - conn->link_mode |= HCI_LM_MASTER;
> - conn->sec_level = BT_SECURITY_LOW;
> + /* Since the controller supports only one LE connection attempt at a
> + * time, we return -EBUSY if there is any connection attempt running.
> + */
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (conn)
> + return ERR_PTR(-EBUSY);
>
> - err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
> - if (err) {
> - hci_conn_del(conn);
> - return ERR_PTR(err);
> - }
> - }
> + conn = hci_conn_add(hdev, LE_LINK, dst);
> + if (!conn)
> + return ERR_PTR(-ENOMEM);
>
> + conn->dst_type = bdaddr_to_le(dst_type);
> + conn->state = BT_CONNECT;
> + conn->out = true;
> + conn->link_mode |= HCI_LM_MASTER;
> + conn->sec_level = BT_SECURITY_LOW;
> conn->pending_sec_level = sec_level;
> conn->auth_type = auth_type;
>
> - hci_conn_hold(conn);
> + err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
> + if (err) {
> + hci_conn_del(conn);
> + return ERR_PTR(err);
> + }
>
> +out:
> + hci_conn_hold(conn);
> return conn;
> }
Regards
Marcel
^ permalink raw reply
* [PATCH] obexd/MAP: Fix invalid snprintf format string in map_msg_get()
From: Szymon Janc @ 2013-10-04 7:01 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
obexd/client/map.c: In function ‘map_msg_get’:
obexd/client/map.c:446:2: warning: format ‘%u’ expects argument of type
‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat]
obexd/client/map.c:446:2: warning: format ‘%u’ expects argument of type
‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat]
---
obexd/client/map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/obexd/client/map.c b/obexd/client/map.c
index 72c4de4..fe15bab 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -443,7 +443,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
return g_dbus_create_error(message,
ERROR_INTERFACE ".InvalidArguments", NULL);
- if (snprintf(handle, sizeof(handle), "%u" PRIu64, msg->handle) < 0)
+ if (snprintf(handle, sizeof(handle), "%" PRIu64, msg->handle) < 0)
goto fail;
transfer = obc_transfer_get("x-bt/message", handle, target_file, &err);
--
1.8.4
^ permalink raw reply related
* Re: [RFCv2 06/14] android: Create HAL API header skeleton
From: Andrei Emeltchenko @ 2013-10-04 7:16 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth
In-Reply-To: <4501039.OZxtA2WV21@uw000953>
Hi All,
On Thu, Oct 03, 2013 at 04:07:08PM +0200, Szymon Janc wrote:
> > +
> > +/* Core Service */
> > +
> > +struct hal_msg_rsp {
> > + struct hal_msg_hdr hdr;
> > + uint8_t status;
> > +} __packed;
>
> Non-error response to request has no status field, just header.
I have removed the structure in the following patches but I still have
doubts that this is a good approach.
So in a case of error we get:
struct error_rsp {
struct hal_msg_hdr hdr;
uint8_t status;
}
and in a case of success we get:
struct rsp {
struct hal_msg_hdr hdr;
}
Does it make sense to omit status? We could read error_rsp and check
status for error while in the current design we have to check for size
received. Are we gonna save uint8_t?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH] obexd/MAP: Fix invalid snprintf format string in map_msg_get()
From: Luiz Augusto von Dentz @ 2013-10-04 7:35 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1380870086-3663-1-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
On Fri, Oct 4, 2013 at 10:01 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> obexd/client/map.c: In function ‘map_msg_get’:
> obexd/client/map.c:446:2: warning: format ‘%u’ expects argument of type
> ‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat]
> obexd/client/map.c:446:2: warning: format ‘%u’ expects argument of type
> ‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat]
> ---
> obexd/client/map.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 72c4de4..fe15bab 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -443,7 +443,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
> return g_dbus_create_error(message,
> ERROR_INTERFACE ".InvalidArguments", NULL);
>
> - if (snprintf(handle, sizeof(handle), "%u" PRIu64, msg->handle) < 0)
> + if (snprintf(handle, sizeof(handle), "%" PRIu64, msg->handle) < 0)
> goto fail;
>
> transfer = obc_transfer_get("x-bt/message", handle, target_file, &err);
> --
> 1.8.4
Applied, thanks
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH v4 5/7] android: Android version of log.c
From: Andrei Emeltchenko @ 2013-10-04 8:16 UTC (permalink / raw)
To: Frederic Danis; +Cc: linux-bluetooth
In-Reply-To: <1380632319-7701-5-git-send-email-frederic.danis@linux.intel.com>
On Tue, Oct 01, 2013 at 02:58:37PM +0200, Frederic Danis wrote:
> ---
> android/log.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> android/main.c | 34 +++++++++++
> 2 files changed, 206 insertions(+)
> create mode 100644 android/log.c
What is the status with review. Do I understand correctly that it needs to
be changed something like:
void info(const char *format, ...)
{
va_list ap;
va_start(ap, format);
- android_log(LOG_INFO, format, ap);
+ vfprintf(stderr, format, ap);
+ fprintf(stderr, "\n");
va_end(ap);
}
If so I can change this myself and resend patch series.
Best regards
Andrei Emeltchenko
^ 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