* RE: [PATCH v4] Bluetooth: btmrvl: add calibration data download support
From: Amitkumar Karwar @ 2013-09-16 15:51 UTC (permalink / raw)
To: 'Mike Frysinger', Bing Zhao
Cc: linux-bluetooth@vger.kernel.org, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, linux-wireless@vger.kernel.org, Hyuckjoo Lee
In-Reply-To: <CAAbOScnG8RHWbPdjq6=e=EBJPr_f6+R4o36f3h0T=NxGgQt7AQ@mail.gmail.com>
Hi Mike,
Thanks for your comments. We will take care of them in updated version.
>=20
> On Fri, Sep 13, 2013 at 7:32 PM, Bing Zhao wrote:
> > --- a/drivers/bluetooth/btmrvl_main.c
> > +++ b/drivers/bluetooth/btmrvl_main.c
> >
> > +static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32
> dst_size)
>=20
> would be nice if you put a comment above this func explaining the
> expected format. otherwise, we see arbitrary parsing with no idea if
> it's correct.
Sure. We will add below information.
Calibrated input data should contain hex bytes separated by space or
new line character. Here is an example
00 1C 01 37 FF FF FF FF 02 04 7F 01
CE BA 00 00 00 2D C6 C0 00 00 00 00
00 F0 00 00
>=20
> > + while ((s - src) < len) {
> > + if (isspace(*s)) {
> > + s++;
> > + continue;
> > + }
> > +
> > + if (isxdigit(*s)) {
> > + if ((d - dst) >=3D dst_size) {
> > + BT_ERR("calibration data file too
> big!!!");
> > + return -EINVAL;
> > + }
> > +
> > + memcpy(tmp, s, 2);
> > +
> > + ret =3D kstrtou8(tmp, 16, d++);
> > + if (ret < 0)
> > + return ret;
> > +
> > + s +=3D 2;
> > + } else {
> > + s++;
> > + }
> > + }
>=20
> so if it's a space, you skip it. if it's a hexdigit, you parse two
> bytes. if it's anything else, you skip it. i'd imagine the "non
> space and non hexdigit" case should throw a warning if not reject the
> file out right. otherwise, if you want to keep this logic, punt the
> explicit "isspace" check.
You are right. Rejecting the file containing non space, non new line and no=
n hexdigit character makes sense.
>=20
> you might also copy one more byte than you should ? your limit is "(s
> - src) < len", yet the isxdigit code always copies two bytes.
Thanks for pointing this out. We will replace "(s - src) < len" with "(s -s=
rc) <=3D (len - 2)".=20
>=20
> > +static int btmrvl_load_cal_data(struct btmrvl_private *priv,
> > + u8 *config_data)
> > +{
> > + struct sk_buff *skb;
> > + struct btmrvl_cmd *cmd;
> > + int i;
> > +
> > + skb =3D bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
>=20
> maybe i'm unfamiliar with bluetooth and this is common, but why is
> your code so special as to require GFP_ATOMIC allocations ?
GFP_ATOMIC was used to match other usages in bluetooth code.
I just found that as per commit "Remove GFP_ATOMIC usage from l2cap_core.c"=
(commit id: 8bcde1f2ab..), since bluetooth core is now running in process c=
ontext, we don't need to use GFP_ATOMIC.
We will replace it with GFP_KERNEL in updated version.
>=20
> > + for (i =3D 4; i < BT_CMD_DATA_SIZE; i++)
> > + cmd->data[i] =3D config_data[(i/4)*8 - 1 - i];
>=20
> style nit, but there should be spacing around those math operators.
> ignoring the fact that this is some funky funky buffer offsets.
>=20
> i =3D 4
> config_data[(4 / 4) * 8 - 1 - 4] ->
> config_data[8 - 1 - 4] ->
> config_data[3]
>=20
> i =3D 5
> config_data[(5 / 4) * 8 - 1 - 5] ->
> config_data[8 - 1 - 5] ->
> config_data[2]
>=20
> i =3D 6
> config_data[(6 / 4) * 8 - 1 - 6] ->
> config_data[8 - 1 - 6] ->
> config_data[1]
>=20
> i =3D 7
> config_data[(7 / 4) * 8 - 1 - 7] ->
> config_data[8 - 1 - 7] ->
> config_data[0]
>=20
> i =3D 8
> config_data[(8 / 4) * 8 - 1 - 8] ->
> config_data[16 - 1 - 8] ->
> config_data[7]
>=20
> i =3D {4,5,6,7} -> config_data[{3,2,1,0}]
> i =3D {8,9,10,11} -> config_data[{7,6,5,4}]
>=20
> that really could do with a comment explaining the mapping of input
> bytes to output bytes.
Sure. We add a comment here.
Actually each 4 bytes are being swapped. Considering 4 byte SDIO header off=
set, it becomes
input{3, 2, 1, 0} -> output{0+4, 1+4, 2+4, 3+4}
Regards,
Amitkumar Karwar
> -mike
^ permalink raw reply
* Re: [PATCH v2 1/5] Bluetooth: Fix hci_add_sysfs
From: Gustavo Padovan @ 2013-09-16 15:06 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1377288071-3664-1-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
2013-08-23 Andre Guedes <andre.guedes@openbossa.org>:
> The inquiry_cache and auto_accept_delay files should be added to
> debugfs only if controller is BR/EDR capable.
>
> Since in hci_register_dev() hdev has not been initialized yet,
> we are not able to check if the controller is BR/EDR capable.
> Thus, we postpone exporting those two files to just after
> controller's initialization.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 17 ++++++++++++-----
> net/bluetooth/hci_sysfs.c | 16 +++++++++++-----
> 3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3ede820..879bf45 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -775,6 +775,7 @@ int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count);
> void hci_init_sysfs(struct hci_dev *hdev);
> int hci_add_sysfs(struct hci_dev *hdev);
> void hci_del_sysfs(struct hci_dev *hdev);
> +void hci_sysfs_export_info(struct hci_dev *hdev);
> void hci_conn_init_sysfs(struct hci_conn *conn);
> void hci_conn_add_sysfs(struct hci_conn *conn);
> void hci_conn_del_sysfs(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 8d9b87d..e8be6ec 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1184,11 +1184,18 @@ int hci_dev_open(__u16 dev)
> hci_dev_hold(hdev);
> set_bit(HCI_UP, &hdev->flags);
> hci_notify(hdev, HCI_DEV_UP);
> - if (!test_bit(HCI_SETUP, &hdev->dev_flags) &&
> - mgmt_valid_hdev(hdev)) {
> - hci_dev_lock(hdev);
> - mgmt_powered(hdev, 1);
> - hci_dev_unlock(hdev);
> + if (mgmt_valid_hdev(hdev)) {
> + /* If we are in HCI_SETUP phase, meaning the device
> + * has just been registered, we should export the
> + * remaining infos to debugfs.
> + */
> + if (test_bit(HCI_SETUP, &hdev->dev_flags)) {
> + hci_sysfs_export_info(hdev);
> + } else {
> + hci_dev_lock(hdev);
> + mgmt_powered(hdev, 1);
> + hci_dev_unlock(hdev);
> + }
These patches doesn't apply cleanly on bluetooth-next anymore. Please rebase
the set and resend. Thanks.
Gustavo
^ permalink raw reply
* Re: [PATCH 1/1 v4] Bluetooth: Fix ACL alive for long in case of non pariable devices
From: Gustavo Padovan @ 2013-09-16 14:52 UTC (permalink / raw)
To: Syam Sidhardhan; +Cc: linux-bluetooth, Syam Sidhardhan
In-Reply-To: <1375721952-3601-1-git-send-email-syamsidhardh@gmail.com>
Hi Syam,
2013-08-06 Syam Sidhardhan <syamsidhardh@gmail.com>:
> From: Syam Sidhardhan <s.syam@samsung.com>
>
> For certain devices (ex: HID mouse), support for authentication,
> pairing and bonding is optional. For such devices, the ACL alive
> for too long after the L2CAP disconnection.
>
> To avoid the ACL alive for too long after L2CAP disconnection, reset the
> ACL disconnect timeout back to HCI_DISCONN_TIMEOUT during L2CAP connect.
>
> While merging the commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422
> this issue might have introduced.
>
> Hcidump info:
> sh-4.1# /opt/hcidump -Xt
> HCI sniffer - Bluetooth packet analyzer ver 2.4
> device: hci0 snap_len: 1500 filter: 0xffffffff
> 2013-08-05 16:48:47.847053 > HCI Event: Connect Request (0x04) plen 10
> bdaddr 00:12:A1:65:E5:B2 class 0x002580 type ACL
> 2013-08-05 16:48:47.847310 < HCI Command: Accept Connection Request (0x01
> |0x0009) plen 7
> bdaddr 00:12:A1:65:E5:B2 role 0x00
> Role: Master
> 2013-08-05 16:48:47.848436 > HCI Event: Command Status (0x0f) plen 4
> Accept Connection Request (0x01|0x0009) status 0x00 ncmd 1
> 2013-08-05 16:48:48.007120 > HCI Event: Role Change (0x12) plen 8
> status 0x00 bdaddr 00:12:A1:65:E5:B2 role 0x00
> Role: Master
> 2013-08-05 16:48:48.050475 > HCI Event: Connect Complete (0x03) plen 11
> status 0x00 handle 12 bdaddr 00:12:A1:65:E5:B2 type ACL encrypt 0x00
> 2013-08-05 16:48:48.051235 < HCI Command: Read Remote Supported Features
> (0x01|0x001b) plen 2
> handle 12
> 2013-08-05 16:48:48.051691 > HCI Event: Command Status (0x0f) plen 4
> Read Remote Supported Features (0x01|0x001b) status 0x00 ncmd 1
> 2013-08-05 16:48:48.051864 > HCI Event: Read Remote Supported Features
> (0x0b) plen 11
> status 0x00 handle 12
> Features: 0xbc 0x02 0x04 0x38 0x08 0x00 0x00 0x00
> 2013-08-05 16:48:48.051899 < HCI Command: Remote Name Request (0x01|
> 0x0019) plen 10
> bdaddr 00:12:A1:65:E5:B2 mode 2 clkoffset 0x0000
> 2013-08-05 16:48:48.052530 > HCI Event: Command Status (0x0f) plen 4
> Remote Name Request (0x01|0x0019) status 0x00 ncmd 1
> 2013-08-05 16:48:48.056407 > ACL data: handle 12 flags 0x02 dlen 12
> L2CAP(s): Connect req: psm 17 scid 0x0049
> 2013-08-05 16:48:48.056492 < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Connect rsp: dcid 0x0040 scid 0x0049 result 1 status 0
> Connection pending - No futher information available
> 2013-08-05 16:48:48.056510 < ACL data: handle 12 flags 0x00 dlen 10
> L2CAP(s): Info req: type 2
> 2013-08-05 16:48:48.061404 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 12 packets 2
> 2013-08-05 16:48:48.064019 > ACL data: handle 12 flags 0x02 dlen 16
> L2CAP(s): Info rsp: type 2 result 0
> Extended feature mask 0x0004
> Bi-directional QoS
> 2013-08-05 16:48:48.064059 < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Connect rsp: dcid 0x0040 scid 0x0049 result 0 status 0
> Connection successful
> 2013-08-05 16:48:48.064071 < ACL data: handle 12 flags 0x00 dlen 12
> L2CAP(s): Config req: dcid 0x0049 flags 0x00 clen 0
> 2013-08-05 16:48:48.067220 > HCI Event: Remote Name Req Complete (0x07)
> plen 255
> status 0x00 bdaddr 00:12:A1:65:E5:B2 name 'Bluetooth Mouse'
> 2013-08-05 16:48:48.067627 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 12 packets 2
> 2013-08-05 16:48:48.070114 > ACL data: handle 12 flags 0x02 dlen 16
> L2CAP(s): Config req: dcid 0x0040 flags 0x00 clen 4
> MTU 185
> 2013-08-05 16:48:48.070148 < ACL data: handle 12 flags 0x00 dlen 18
> L2CAP(s): Config rsp: scid 0x0049 flags 0x00 result 0 clen 4
> MTU 185
> 2013-08-05 16:48:48.072642 > ACL data: handle 12 flags 0x02 dlen 18
> L2CAP(s): Config rsp: scid 0x0040 flags 0x00 result 0 clen 4
> MTU 185
> 2013-08-05 16:48:48.075123 > ACL data: handle 12 flags 0x02 dlen 12
> L2CAP(s): Connect req: psm 19 scid 0x004a
> 2013-08-05 16:48:48.075196 < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Connect rsp: dcid 0x0041 scid 0x004a result 1 status 2
> Connection pending - Authorization pending
> 2013-08-05 16:48:48.075364 < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Connect rsp: dcid 0x0041 scid 0x004a result 0 status 0
> Connection successful
> 2013-08-05 16:48:48.075403 < ACL data: handle 12 flags 0x00 dlen 12
> L2CAP(s): Config req: dcid 0x004a flags 0x00 clen 0
> 2013-08-05 16:48:48.077633 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 12 packets 2
> 2013-08-05 16:48:48.080127 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 12 packets 2
> 2013-08-05 16:48:48.081355 > ACL data: handle 12 flags 0x02 dlen 16
> L2CAP(s): Config req: dcid 0x0041 flags 0x00 clen 4
> MTU 185
> 2013-08-05 16:48:48.081402 < ACL data: handle 12 flags 0x00 dlen 18
> L2CAP(s): Config rsp: scid 0x004a flags 0x00 result 0 clen 4
> MTU 185
> 2013-08-05 16:48:48.082633 > ACL data: handle 12 flags 0x02 dlen 18
> L2CAP(s): Config rsp: scid 0x0041 flags 0x00 result 0 clen 4
> MTU 185
> 2013-08-05 16:48:48.084838 < ACL data: handle 12 flags 0x00 dlen 12
> L2CAP(d): cid 0x004a len 8 [psm 19]
> HIDP: Data: Output report
> 0000: 02 00 00 00 00 00 00 .......
> ...
> ...
>
> 2013-08-05 16:49:00.894129 < ACL data: handle 12 flags 0x00 dlen 12
> L2CAP(s): Disconn req: dcid 0x004a scid 0x0041
> 2013-08-05 16:49:00.894195 < HCI Command: Exit Sniff Mode (0x02|0x0004)
> plen 2
> handle 12
> 2013-08-05 16:49:00.894269 < ACL data: handle 12 flags 0x00 dlen 12
> L2CAP(s): Disconn req: dcid 0x0049 scid 0x0040
> 2013-08-05 16:49:00.895645 > HCI Event: Command Status (0x0f) plen 4
> Exit Sniff Mode (0x02|0x0004) status 0x00 ncmd 1
> 2013-08-05 16:49:00.934391 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 12 mode 0x00 interval 0
> Mode: Active
> 2013-08-05 16:49:00.936592 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 12 packets 2
> 2013-08-05 16:49:00.951577 > ACL data: handle 12 flags 0x02 dlen 12
> L2CAP(s): Disconn rsp: dcid 0x004a scid 0x0041
> 2013-08-05 16:49:00.952820 > ACL data: handle 12 flags 0x02 dlen 12
> L2CAP(s): Disconn rsp: dcid 0x0049 scid 0x0040
> 2013-08-05 16:49:00.969165 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 12 mode 0x02 interval 50
> Mode: Sniff
>
> 2013-08-05 16:49:48.175533 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 12 mode 0x00 interval 0
> Mode: Active
> 2013-08-05 16:49:48.219045 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 12 mode 0x02 interval 108
> Mode: Sniff
>
> 2013-08-05 16:51:00.968209 < HCI Command: Disconnect (0x01|0x0006) plen 3
> handle 12 reason 0x13
> Reason: Remote User Terminated Connection
> 2013-08-05 16:51:00.969056 > HCI Event: Command Status (0x0f) plen 4
> Disconnect (0x01|0x0006) status 0x00 ncmd 1
> 2013-08-05 16:51:01.013495 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 12 mode 0x00 interval 0
> Mode: Active
> 2013-08-05 16:51:01.073777 > HCI Event: Disconn Complete (0x05) plen 4
> status 0x00 handle 12 reason 0x16
> Reason: Connection Terminated by Local Host
>
> ========================= Conn status =================================
> sh-4.1# date; hcitool con
> Mon Aug 5 16:49:16 KST 2013
> Connections:
> > ACL 00:12:A1:65:E5:B2 handle 12 state 1 lm MASTER
> sh-4.1# date; hcitool con
> Mon Aug 5 16:50:32 KST 2013
> Connections:
> > ACL 00:12:A1:65:E5:B2 handle 12 state 1 lm MASTER
> sh-4.1# date; hcitool con
> Mon Aug 5 16:50:59 KST 2013
> Connections:
> > ACL 00:12:A1:65:E5:B2 handle 12 state 1 lm MASTER
> sh-4.1# date; hcitool con
> Mon Aug 5 16:51:01 KST 2013
> Connections:
> sh-4.1#
>
> ============================ After fix ================================
>
> 2013-08-05 16:57:35.986648 < ACL data: handle 11 flags 0x00 dlen 12
> L2CAP(s): Disconn req: dcid 0x004c scid 0x0041
> 2013-08-05 16:57:35.986713 < HCI Command: Exit Sniff Mode (0x02|0x0004)
> plen 2
> handle 11
> 2013-08-05 16:57:35.986785 < ACL data: handle 11 flags 0x00 dlen 12
> L2CAP(s): Disconn req: dcid 0x004b scid 0x0040
> 2013-08-05 16:57:35.988110 > HCI Event: Command Status (0x0f) plen 4
> Exit Sniff Mode (0x02|0x0004) status 0x00 ncmd 1
> 2013-08-05 16:57:36.030714 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 11 mode 0x00 interval 0
> Mode: Active
> 2013-08-05 16:57:36.032950 > HCI Event: Number of Completed Packets
> (0x13) plen 5
> handle 11 packets 2
> 2013-08-05 16:57:36.047926 > ACL data: handle 11 flags 0x02 dlen 12
> L2CAP(s): Disconn rsp: dcid 0x004c scid 0x0041
> 2013-08-05 16:57:36.049200 > ACL data: handle 11 flags 0x02 dlen 12
> L2CAP(s): Disconn rsp: dcid 0x004b scid 0x0040
> 2013-08-05 16:57:36.065509 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 11 mode 0x02 interval 50
> Mode: Sniff
>
> 2013-08-05 16:57:40.052006 < HCI Command: Disconnect (0x01|0x0006) plen 3
> handle 11 reason 0x13
> Reason: Remote User Terminated Connection
> 2013-08-05 16:57:40.052869 > HCI Event: Command Status (0x0f) plen 4
> Disconnect (0x01|0x0006) status 0x00 ncmd 1
> 2013-08-05 16:57:40.104731 > HCI Event: Mode Change (0x14) plen 6
> status 0x00 handle 11 mode 0x00 interval 0
> Mode: Active
> 2013-08-05 16:57:40.146935 > HCI Event: Disconn Complete (0x05) plen 4
> status 0x00 handle 11 reason 0x16
> Reason: Connection Terminated by Local Host
>
> Signed-off-by: Sang-Ki Park <sangki79.park@samsung.com>
> Signed-off-by: Chan-yeol Park <chanyeol.park@samsung.com>
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
> ---
>
> v1 -> Modified the code as per the latest code.
> v2 -> Add descriptive comment as per Marcel request.
> v3 -> Moved from l2cap_conn_ready() to l2cap_connect() inorder to fix remote
> non ssp pairing timeout. Generated the patch based on bluetooth.git tree.
> v4 -> Include hcidump logs showing the conn status before and after fix.
>
> This patch is generated the patch based on bluetooth.git tree.
>
> net/bluetooth/l2cap_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
Patch has been applied to bluetooth.git. Thanks.
Gustavo
^ permalink raw reply
* Re: [RFC SBC] build: Add configure option --disable-high-precision
From: Luiz Augusto von Dentz @ 2013-09-16 14:43 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <ACAC771F-74CD-40BF-BFB7-4CC92B588415@holtmann.org>
Hi Marcel,
On Mon, Sep 16, 2013 at 5:19 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> This enables high precision using 64 bits accumulators by default which
>> can be disabled with --disable-high-precision.
>> ---
>> configure.ac | 8 ++++++++
>> sbc/sbc_tables.h | 2 --
>> 2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index c052616..5f994d1 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -58,4 +58,12 @@ if (test "${enable_tester}" != "no"); then
>> fi
>> AM_CONDITIONAL(TESTER, test "${enable_tester}" != "no")
>>
>> +AC_ARG_ENABLE(high-precision, AC_HELP_STRING([--disable-high-precision],
>> + [disable high precision support]),
>> + [enable_high_precision=${enableval}])
>> +if (test "${enable_high_precision}" != "no"); then
>> + AC_DEFINE(SBC_HIGH_PRECISION, 1,
>> + [Define to 1 to enable high precision build of SBC encoder])
>> +fi
>> +
>> AC_OUTPUT(Makefile sbc/sbc.pc)
>> diff --git a/sbc/sbc_tables.h b/sbc/sbc_tables.h
>> index 25e24e6..3fd80e1 100644
>> --- a/sbc/sbc_tables.h
>> +++ b/sbc/sbc_tables.h
>> @@ -136,8 +136,6 @@ static const int32_t synmatrix8[16][8] = {
>> SN8(0xfb8e3130), SN8(0xf8275a10), SN8(0xfe70747c), SN8(0x06a6d988) }
>> };
>>
>> -/* Uncomment the following line to enable high precision build of SBC encoder */
>> -
>> /* #define SBC_HIGH_PRECISION */
>
> the comment with the define should also be removed.
>
> Is there any impact on using 64-bit integers compared to 32-bit integers. When would you use one or the other. I think this needs to be documented in README as well.
The use of 64-bit apparently makes the encoder 'better' in term of
dynamic range, but I will probably check with the tools that Siarhei
uses for testing the exact amount of 'better', the drawback is that
for 32-bits systems it might have an impact in performance so it is
good to keep --disable-high-precision and document it in the README as
you suggested.
Maybe this will be useful to kick start some unit tests for sbc as well.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [RFC SBC] build: Add configure option --disable-high-precision
From: Marcel Holtmann @ 2013-09-16 14:19 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1379323375-21540-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
> This enables high precision using 64 bits accumulators by default which
> can be disabled with --disable-high-precision.
> ---
> configure.ac | 8 ++++++++
> sbc/sbc_tables.h | 2 --
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index c052616..5f994d1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -58,4 +58,12 @@ if (test "${enable_tester}" != "no"); then
> fi
> AM_CONDITIONAL(TESTER, test "${enable_tester}" != "no")
>
> +AC_ARG_ENABLE(high-precision, AC_HELP_STRING([--disable-high-precision],
> + [disable high precision support]),
> + [enable_high_precision=${enableval}])
> +if (test "${enable_high_precision}" != "no"); then
> + AC_DEFINE(SBC_HIGH_PRECISION, 1,
> + [Define to 1 to enable high precision build of SBC encoder])
> +fi
> +
> AC_OUTPUT(Makefile sbc/sbc.pc)
> diff --git a/sbc/sbc_tables.h b/sbc/sbc_tables.h
> index 25e24e6..3fd80e1 100644
> --- a/sbc/sbc_tables.h
> +++ b/sbc/sbc_tables.h
> @@ -136,8 +136,6 @@ static const int32_t synmatrix8[16][8] = {
> SN8(0xfb8e3130), SN8(0xf8275a10), SN8(0xfe70747c), SN8(0x06a6d988) }
> };
>
> -/* Uncomment the following line to enable high precision build of SBC encoder */
> -
> /* #define SBC_HIGH_PRECISION */
the comment with the define should also be removed.
Is there any impact on using 64-bit integers compared to 32-bit integers. When would you use one or the other. I think this needs to be documented in README as well.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Marcel Holtmann @ 2013-09-16 14:14 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20130916081926.GA13593@x220.p-661hnu-f1>
Hi Johan,
>>> @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
>>> }
>>> EXPORT_SYMBOL(bt_sock_wait_state);
>>>
>>
>> add a small comment here that this needs to be called with sk lock held.
>
> I suppose I should do that to the old bt_sock_wait_state function too?
> (which btw my wait_ready function is essentially a copy of, with the
> exception of which condition it looks for)
good idea.
>>> +int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
>>> +{
>>> + DECLARE_WAITQUEUE(wait, current);
>>> + unsigned long timeo;
>>> + int err = 0;
>>> +
>>> + BT_DBG("sk %p", sk);
>>> +
>>> + timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
>>> +
>>> + add_wait_queue(sk_sleep(sk), &wait);
>>> + set_current_state(TASK_INTERRUPTIBLE);
>>> + while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
>>> + if (!timeo) {
>>> + err = -EAGAIN;
>>> + break;
>>> + }
>>> +
>>> + if (signal_pending(current)) {
>>> + err = sock_intr_errno(timeo);
>>> + break;
>>> + }
>>> +
>>> + release_sock(sk);
>>> + timeo = schedule_timeout(timeo);
>>> + lock_sock(sk);
>>> + set_current_state(TASK_INTERRUPTIBLE);
>>> +
>>> + err = sock_error(sk);
>>> + if (err)
>>> + break;
>>> + }
>>> + __set_current_state(TASK_RUNNING);
>>> + remove_wait_queue(sk_sleep(sk), &wait);
>>> +
>>> + return err;
>>> +}
>>> +EXPORT_SYMBOL(bt_sock_wait_ready);
>>> +
>>> #ifdef CONFIG_PROC_FS
>>> struct bt_seq_state {
>>> struct bt_sock_list *l;
>>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>>> index 0098af8..ad95b42 100644
>>> --- a/net/bluetooth/l2cap_sock.c
>>> +++ b/net/bluetooth/l2cap_sock.c
>>> @@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
>>> if (sk->sk_state != BT_CONNECTED)
>>> return -ENOTCONN;
>>>
>>> + lock_sock(sk);
>>> + err = bt_sock_wait_ready(sk, msg->msg_flags);
>>> + release_sock(sk);
>>> + if (err)
>>> + return err;
>>> +
>>
>> After starting to look into this now, I am not sure we have proper
>> locking in that function in the first place. Can you check that it is
>> actually fine not holding the socket lock for the send operation
>> itself.
>
> What procedure did you have in mind for checking what's fine and what's
> not? Some quick googling and trying to look at the code of other address
> families didn't help me getting much wiser about this.
>
> This is the way that RFCOMM and L2CAP sockets seem to always have done
> locking and we haven't seen any obvious breakage because of it. The
> RFCOMM side does indeed use lock_sock for the send operation and L2CAP
> doesn't. However, I think the reason it works is that l2cap_chan_lock
> (which is used before calling l2cap_chan_send) serves a similar purpose.
Then lets leave it as is.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Marcel Holtmann @ 2013-09-16 14:13 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379325919-19003-9-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> In the case of blocking sockets we should not proceed with sendmsg() if
> the socket has the BT_SK_SUSPEND flag set. So far the code was only
> ensuring that POLLOUT doesn't get set for non-blocking sockets using
> poll() but there was no code in place to ensure that blocking sockets do
> the right thing when writing to them.
>
> This patch adds a new bt_sock_wait_ready helper function to sleep in the
> sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon as it
> is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks to take
> advantage of this new helper function.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/bluetooth.h | 1 +
> net/bluetooth/af_bluetooth.c | 40 +++++++++++++++++++++++++++++++++++++++
> net/bluetooth/l2cap_sock.c | 6 ++++++
> net/bluetooth/rfcomm/sock.c | 7 ++++++-
> 4 files changed, 53 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 3/8] Bluetooth: Fix L2CAP error return used for failed channel lookups
From: Marcel Holtmann @ 2013-09-16 14:12 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379325919-19003-4-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> The EFAULT error should only be used for memory address related errors
> and ENOENT might be needed for other purposes than invalid CID errors.
> This patch fixes the l2cap_config_req, l2cap_connect_create_rsp and
> l2cap_create_channel_req handlers to use the unique EBADSLT error to
> indicate failed lookups on a given CID.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 5/8] Bluetooth: Fix L2CAP command reject reason
From: Marcel Holtmann @ 2013-09-16 14:11 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379325919-19003-6-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> There are several possible reason codes that can be sent in the command
> reject L2CAP packet. Before this patch the code has used a hard-coded
> single response code ("command not understood"). This patch adds a
> helper function to map the return value of an L2CAP handler function to
> the correct command reject reason.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: Marcel Holtmann @ 2013-09-16 14:10 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379325919-19003-5-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> If we receive an L2CAP Disconnect Request for an unknown CID we should
> not just silently drop it but reply with a proper Command Reject
> response. This patch fixes this by ensuring that the disconnect handler
> returns a proper error instead of 0 and will cause the function caller
> to send the right response.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/8] sdp: Decouple Device ID profile implementation
From: Luiz Augusto von Dentz @ 2013-09-16 12:04 UTC (permalink / raw)
To: Szymon Janc; +Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org
In-Reply-To: <2389837.qqrqTJ68od@uw000953>
Hi Szymon,
On Mon, Sep 16, 2013 at 2:10 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Marcel,
>
>> > Make DeviceID profile similar to other profiles implementations. Use
>> > btd_profile for handling DeviceID profile while adding/removing
>> > adapters. The nice drawback is that SDP code no longer depends on
>> > main_opts.
>> > ---
>> > Makefile.plugins | 3 +
>> > profiles/deviceid/deviceid.c | 181 +++++++++++++++++++++++++++++++++++++++++++
>> > src/sdpd-server.c | 4 -
>> > src/sdpd-service.c | 58 --------------
>> > src/sdpd.h | 2 -
>> > 5 files changed, 184 insertions(+), 64 deletions(-)
>> > create mode 100644 profiles/deviceid/deviceid.c
>> >
>> > diff --git a/Makefile.plugins b/Makefile.plugins
>> > index 7c5f71d..df5d2a1 100644
>> > --- a/Makefile.plugins
>> > +++ b/Makefile.plugins
>> > @@ -82,6 +82,9 @@ builtin_sources += profiles/scanparam/scan.c
>> > builtin_modules += deviceinfo
>> > builtin_sources += profiles/deviceinfo/deviceinfo.c
>> >
>> > +builtin_modules += deviceid
>> > +builtin_sources += profiles/deviceid/deviceid.c
>> > +
>>
>> this is changing the semantics a little bit. Device Id record is guaranteed
>> to have 0x10000 handle. I would prefer if we can keep it that way.
>
> Why? There is no such requirement in spec (is it?), and this is just another
> profile so not sure why would you like to treat it in special way. (Also if one
> disable deviceid other profiles might get 0x10000 handle.)
>
> If that is really needed we could still have a plugin, just with high prio so
> that it init is called first and then use insert instead of add record when
> probing adapters.
>
>> Also we need to find a way to make this work with the extended inquiry
>> response that can also included device id information.
>
> Currently deviceid info is in main_opts and is handled from many places.
> We could remove this from main_opts and put all the logic (config reading,
> default values etc) to deviceid plugin and let it set proper values for
> adapters.
I would prefer to stick with core managing the main.conf and then have
an API to which the plugins can access it, iirc that how connman deals
with plugins accessing information in the conf file.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 1/6] obexd: Add request struct to MAP
From: Luiz Augusto von Dentz @ 2013-09-16 12:00 UTC (permalink / raw)
To: Christian Fetzer; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1379086116-22617-1-git-send-email-christian.fetzer@oss.bmw-carit.de>
Hi Christian,
On Fri, Sep 13, 2013 at 6:28 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>
> This adds a pending_request struct in order to store the D-Bus request
> data.
>
> The current version stores the received D-Bus message in the MAP session
> struct. The stored message is overridden by intermediate D-Bus method
> calls which can lead into a crash.
>
> Trace:
> arguments to dbus_message_unref() were incorrect,
> assertion "!message->in_cache" failed in file dbus-message.c line 1618.
>
> 0 0x00007ffff6a6a1c9 in raise () from /usr/lib/libc.so.6
> 1 0x00007ffff6a6b5c8 in abort () from /usr/lib/libc.so.6
> 2 0x00007ffff7313de5 in ?? () from /usr/lib/libdbus-1.so.3
> 3 0x00007ffff730ab91 in ?? () from /usr/lib/libdbus-1.so.3
> 4 0x000000000043721c in message_listing_cb (session=0x6a7d30,
> transfer=0x6a9450, err=0x0, user_data=0x6a9950) at obexd/client/map.c:1166
> 5 0x000000000042f7af in session_terminate_transfer (session=0x6a7d30,
> transfer=0x6a9450, gerr=0x0) at obexd/client/session.c:830
> 6 0x000000000042f83d in session_notify_complete (session=0x6a7d30,
> transfer=0x6a9450) at obexd/client/session.c:845
> 7 0x000000000042f8dc in transfer_complete (transfer=0x6a9450, err=0x0,
> user_data=0x6a7d30) at obexd/client/session.c:865
> 8 0x0000000000439ee7 in xfer_complete (obex=0x677250, err=0x0,
> user_data=0x6a9450) at obexd/client/transfer.c:577
> 9 0x000000000043a05f in get_xfer_progress_first (obex=0x677250, err=0x0,
> rsp=0x678730, user_data=0x6a9450) at obexd/client/transfer.c:621
> 10 0x0000000000413f08 in handle_response (obex=0x677250, err=0x0,
> rsp=0x678730) at gobex/gobex.c:949
> 11 0x00000000004147db in incoming_data (io=0x6a8a00, cond=G_IO_IN,
> user_data=0x677250) at gobex/gobex.c:1192
> 12 0x00007ffff702dda6 in g_main_context_dispatch ()
> from /usr/lib/libglib-2.0.so.0
> 13 0x00007ffff702e0f8 in ?? () from /usr/lib/libglib-2.0.so.0
> 14 0x00007ffff702e4fa in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
> 15 0x0000000000427ce8 in main (argc=1, argv=0x7fffffffdd48)
> at obexd/src/main.c:319
> ---
> obexd/client/map.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 95f0334..f969aad 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -101,6 +101,11 @@ struct map_data {
> uint8_t supported_message_types;
> };
>
> +struct pending_request {
> + struct map_data *map;
> + DBusMessage *msg;
> +};
> +
> #define MAP_MSG_FLAG_PRIORITY 0x01
> #define MAP_MSG_FLAG_READ 0x02
> #define MAP_MSG_FLAG_SENT 0x04
> @@ -134,6 +139,25 @@ struct map_parser {
>
> static DBusConnection *conn = NULL;
>
> +static struct pending_request *pending_request_new(struct map_data *map,
> + DBusMessage *message)
> +{
> + struct pending_request *p;
> +
> + p = g_new0(struct pending_request, 1);
> + p->map = map;
> + p->msg = dbus_message_ref(message);
> +
> + return p;
> +}
> +
> +static void pending_request_free(struct pending_request *p)
> +{
> + dbus_message_unref(p->msg);
> +
> + g_free(p);
> +}
> +
> static void simple_cb(struct obc_session *session,
> struct obc_transfer *transfer,
> GError *err, void *user_data)
> --
> 1.8.3.4
All patches applied, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 4/8] sdp: Decouple Device ID profile implementation
From: Szymon Janc @ 2013-09-16 11:10 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <B531A8F3-8ABA-4CD1-A013-AB0AFF40302C@holtmann.org>
Hi Marcel,
> > Make DeviceID profile similar to other profiles implementations. Use
> > btd_profile for handling DeviceID profile while adding/removing
> > adapters. The nice drawback is that SDP code no longer depends on
> > main_opts.
> > ---
> > Makefile.plugins | 3 +
> > profiles/deviceid/deviceid.c | 181 +++++++++++++++++++++++++++++++++++++++++++
> > src/sdpd-server.c | 4 -
> > src/sdpd-service.c | 58 --------------
> > src/sdpd.h | 2 -
> > 5 files changed, 184 insertions(+), 64 deletions(-)
> > create mode 100644 profiles/deviceid/deviceid.c
> >
> > diff --git a/Makefile.plugins b/Makefile.plugins
> > index 7c5f71d..df5d2a1 100644
> > --- a/Makefile.plugins
> > +++ b/Makefile.plugins
> > @@ -82,6 +82,9 @@ builtin_sources += profiles/scanparam/scan.c
> > builtin_modules += deviceinfo
> > builtin_sources += profiles/deviceinfo/deviceinfo.c
> >
> > +builtin_modules += deviceid
> > +builtin_sources += profiles/deviceid/deviceid.c
> > +
>
> this is changing the semantics a little bit. Device Id record is guaranteed
> to have 0x10000 handle. I would prefer if we can keep it that way.
Why? There is no such requirement in spec (is it?), and this is just another
profile so not sure why would you like to treat it in special way. (Also if one
disable deviceid other profiles might get 0x10000 handle.)
If that is really needed we could still have a plugin, just with high prio so
that it init is called first and then use insert instead of add record when
probing adapters.
> Also we need to find a way to make this work with the extended inquiry
> response that can also included device id information.
Currently deviceid info is in main_opts and is handled from many places.
We could remove this from main_opts and put all the logic (config reading,
default values etc) to deviceid plugin and let it set proper values for
adapters.
--
BR
Szymon Janc
^ permalink raw reply
* [PATCH v5 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
In the case of blocking sockets we should not proceed with sendmsg() if
the socket has the BT_SK_SUSPEND flag set. So far the code was only
ensuring that POLLOUT doesn't get set for non-blocking sockets using
poll() but there was no code in place to ensure that blocking sockets do
the right thing when writing to them.
This patch adds a new bt_sock_wait_ready helper function to sleep in the
sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon as it
is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks to take
advantage of this new helper function.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/bluetooth.h | 1 +
net/bluetooth/af_bluetooth.c | 40 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/l2cap_sock.c | 6 ++++++
net/bluetooth/rfcomm/sock.c | 7 ++++++-
4 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10d43d8..afbc711 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -249,6 +249,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
+int bt_sock_wait_ready(struct sock *sk, unsigned long flags);
void bt_accept_enqueue(struct sock *parent, struct sock *sk);
void bt_accept_unlink(struct sock *sk);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9096137..c600631 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -525,6 +525,46 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
}
EXPORT_SYMBOL(bt_sock_wait_state);
+/* This function expects the sk lock to be held when called */
+int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
+{
+ DECLARE_WAITQUEUE(wait, current);
+ unsigned long timeo;
+ int err = 0;
+
+ BT_DBG("sk %p", sk);
+
+ timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ set_current_state(TASK_INTERRUPTIBLE);
+ while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
+ if (!timeo) {
+ err = -EAGAIN;
+ break;
+ }
+
+ if (signal_pending(current)) {
+ err = sock_intr_errno(timeo);
+ break;
+ }
+
+ release_sock(sk);
+ timeo = schedule_timeout(timeo);
+ lock_sock(sk);
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ err = sock_error(sk);
+ if (err)
+ break;
+ }
+ __set_current_state(TASK_RUNNING);
+ remove_wait_queue(sk_sleep(sk), &wait);
+
+ return err;
+}
+EXPORT_SYMBOL(bt_sock_wait_ready);
+
#ifdef CONFIG_PROC_FS
struct bt_seq_state {
struct bt_sock_list *l;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0098af8..ad95b42 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
if (sk->sk_state != BT_CONNECTED)
return -ENOTCONN;
+ lock_sock(sk);
+ err = bt_sock_wait_ready(sk, msg->msg_flags);
+ release_sock(sk);
+ if (err)
+ return err;
+
l2cap_chan_lock(chan);
err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
l2cap_chan_unlock(chan);
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 30b3721..072938d 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
struct sock *sk = sock->sk;
struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
struct sk_buff *skb;
- int sent = 0;
+ int sent;
if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
return -ENOTCONN;
@@ -559,6 +559,10 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
lock_sock(sk);
+ sent = bt_sock_wait_ready(sk, msg->msg_flags);
+ if (sent)
+ goto done;
+
while (len) {
size_t size = min_t(size_t, len, d->mtu);
int err;
@@ -594,6 +598,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
len -= size;
}
+done:
release_sock(sk);
return sent;
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 7/8] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When we have an LE link we should not respond to any data on the BR/EDR
L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
This patch fixes this issue by checking for a valid link type and
ignores data if it is wrong.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b0947ae..636a3b4 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static __le16 l2cap_err_to_reason(int err)
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
+ struct hci_conn *hcon = conn->hcon;
u8 *data = skb->data;
int len = skb->len;
struct l2cap_cmd_hdr cmd;
@@ -5318,6 +5319,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
l2cap_raw_recv(conn, skb);
+ if (hcon->type != LE_LINK)
+ return;
+
while (len >= L2CAP_CMD_HDR_SIZE) {
u16 cmd_len;
memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
@@ -5355,6 +5359,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
static inline void l2cap_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
+ struct hci_conn *hcon = conn->hcon;
u8 *data = skb->data;
int len = skb->len;
struct l2cap_cmd_hdr cmd;
@@ -5362,6 +5367,9 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
l2cap_raw_recv(conn, skb);
+ if (hcon->type != ACL_LINK)
+ return;
+
while (len >= L2CAP_CMD_HDR_SIZE) {
u16 cmd_len;
memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 6/8] Bluetooth: Fix sending responses to identified L2CAP response packets
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When L2CAP packets return a non-zero error and the value is passed
onwards by l2cap_bredr_sig_cmd this will trigger a command reject packet
to be sent. However, the core specification (page 1416 in core 4.0) says
the following: "Command Reject packets should not be sent in response to
an identified Response packet.".
This patch ensures that a command reject packet is not sent for any
identified response packet by ignoring the error return value from the
response handler functions.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2ff35a2..b0947ae 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5212,7 +5212,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
case L2CAP_CONN_RSP:
case L2CAP_CREATE_CHAN_RSP:
- err = l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
+ l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CONF_REQ:
@@ -5220,7 +5220,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_CONF_RSP:
- err = l2cap_config_rsp(conn, cmd, cmd_len, data);
+ l2cap_config_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_DISCONN_REQ:
@@ -5228,7 +5228,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_DISCONN_RSP:
- err = l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
+ l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_ECHO_REQ:
@@ -5243,7 +5243,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_INFO_RSP:
- err = l2cap_information_rsp(conn, cmd, cmd_len, data);
+ l2cap_information_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CREATE_CHAN_REQ:
@@ -5255,7 +5255,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_RSP:
- err = l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_MOVE_CHAN_CFM:
@@ -5263,7 +5263,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_CFM_RSP:
- err = l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
break;
default:
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 5/8] Bluetooth: Fix L2CAP command reject reason
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
There are several possible reason codes that can be sent in the command
reject L2CAP packet. Before this patch the code has used a hard-coded
single response code ("command not understood"). This patch adds a
helper function to map the return value of an L2CAP handler function to
the correct command reject reason.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 31bf812..2ff35a2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5294,6 +5294,20 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
}
}
+static __le16 l2cap_err_to_reason(int err)
+{
+ switch (err) {
+ case -EBADSLT:
+ return __constant_cpu_to_le16(L2CAP_REJ_INVALID_CID);
+ case -EMSGSIZE:
+ return __constant_cpu_to_le16(L2CAP_REJ_MTU_EXCEEDED);
+ case -EINVAL:
+ case -EPROTO:
+ default:
+ return __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ }
+}
+
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
@@ -5326,8 +5340,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
@@ -5371,8 +5384,7 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
If we receive an L2CAP Disconnect Request for an unknown CID we should
not just silently drop it but reply with a proper Command Reject
response. This patch fixes this by ensuring that the disconnect handler
returns a proper error instead of 0 and will cause the function caller
to send the right response.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index eb60cf7..31bf812 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
chan = __l2cap_get_chan_by_scid(conn, dcid);
if (!chan) {
mutex_unlock(&conn->chan_lock);
- return 0;
+ return -EBADSLT;
}
l2cap_chan_lock(chan);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 3/8] Bluetooth: Fix L2CAP error return used for failed channel lookups
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The EFAULT error should only be used for memory address related errors
and ENOENT might be needed for other purposes than invalid CID errors.
This patch fixes the l2cap_config_req, l2cap_connect_create_rsp and
l2cap_create_channel_req handlers to use the unique EBADSLT error to
indicate failed lookups on a given CID.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b8a270..eb60cf7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3884,13 +3884,13 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
if (scid) {
chan = __l2cap_get_chan_by_scid(conn, scid);
if (!chan) {
- err = -EFAULT;
+ err = -EBADSLT;
goto unlock;
}
} else {
chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
if (!chan) {
- err = -EFAULT;
+ err = -EBADSLT;
goto unlock;
}
}
@@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
chan = l2cap_get_chan_by_scid(conn, dcid);
if (!chan)
- return -ENOENT;
+ return -EBADSLT;
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
@@ -4438,7 +4438,7 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
hs_hcon = hci_conn_hash_lookup_ba(hdev, AMP_LINK, conn->dst);
if (!hs_hcon) {
hci_dev_put(hdev);
- return -EFAULT;
+ return -EBADSLT;
}
BT_DBG("mgr %p bredr_chan %p hs_hcon %p", mgr, chan, hs_hcon);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When an L2CAP request handler returns non-zero the calling code will
send a command reject response. The l2cap_create_chan_req function will
in some cases send its own response but then still return a -EFAULT
error which would cause two responses to be sent. This patch fixes this
by making the function return 0 after sending its own response.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b3bb7bc..0b8a270 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4462,7 +4462,7 @@ error:
l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
sizeof(rsp), &rsp);
- return -EFAULT;
+ return 0;
}
static void l2cap_send_move_chan_req(struct l2cap_chan *chan, u8 dest_amp_id)
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 1/8] Bluetooth: Remove unused event mask struct
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The struct for HCI_Set_Event_Mask is never used. Instead a local 8-byte
array is used for sending this command. Therefore, remove the
unnecessary struct definition.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 57aa408..7ede266 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -695,9 +695,6 @@ struct hci_cp_sniff_subrate {
} __packed;
#define HCI_OP_SET_EVENT_MASK 0x0c01
-struct hci_cp_set_event_mask {
- __u8 mask[8];
-} __packed;
#define HCI_OP_RESET 0x0c03
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 0/8] Bluetooth: Various L2CAP fixes
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
Hi,
Here's a fifth revision with all feedback taken into account (as far as
these 8 patches are concerned). There's still the issue of verifying
sk locking in l2cap_sock_sendmsg as well as a potentially missing
comment for the wait_state function, but even if those fixes are needed
they'll end up being in separate patches. So, I'm hoping this patch set
could start reaching its conclusion.
Johan
----------------------------------------------------------------
Johan Hedberg (8):
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
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/hci.h | 3 ---
net/bluetooth/af_bluetooth.c | 40 ++++++++++++++++++++++++++++
net/bluetooth/l2cap_core.c | 52 +++++++++++++++++++++++++------------
net/bluetooth/l2cap_sock.c | 6 +++++
net/bluetooth/rfcomm/sock.c | 7 ++++-
6 files changed, 89 insertions(+), 20 deletions(-)
^ permalink raw reply
* Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: Johan Hedberg @ 2013-09-16 9:54 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <B44973E0-9AF5-4AE2-8C81-E49EE46C6E31@holtmann.org>
Hi Marcel,
On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> I have nothing against ENOENT. I however am a bit concerned about that
> we accidentally use ENOENT in some other case and end up overloading
> its meaning.
>
> So just for the sake of making this unique, I would consider going
> with EBADSLT. I am not married to this at all. However I wanted to
> have this talked through so that we understand the whole picture. If I
> would know the perfect answer, I would have just forced you to use it
> ;)
I really don't have a strong opinion one way or another, both seem good
enough to me, but since ENOENT has you feeling a bit uneasy I'll change
this to EBADSLT for the next revision (which will hopefully be the last
one for this error code issue).
Johan
^ permalink raw reply
* [RFC SBC] build: Add configure option --disable-high-precision
From: Luiz Augusto von Dentz @ 2013-09-16 9:22 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This enables high precision using 64 bits accumulators by default which
can be disabled with --disable-high-precision.
---
configure.ac | 8 ++++++++
sbc/sbc_tables.h | 2 --
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index c052616..5f994d1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -58,4 +58,12 @@ if (test "${enable_tester}" != "no"); then
fi
AM_CONDITIONAL(TESTER, test "${enable_tester}" != "no")
+AC_ARG_ENABLE(high-precision, AC_HELP_STRING([--disable-high-precision],
+ [disable high precision support]),
+ [enable_high_precision=${enableval}])
+if (test "${enable_high_precision}" != "no"); then
+ AC_DEFINE(SBC_HIGH_PRECISION, 1,
+ [Define to 1 to enable high precision build of SBC encoder])
+fi
+
AC_OUTPUT(Makefile sbc/sbc.pc)
diff --git a/sbc/sbc_tables.h b/sbc/sbc_tables.h
index 25e24e6..3fd80e1 100644
--- a/sbc/sbc_tables.h
+++ b/sbc/sbc_tables.h
@@ -136,8 +136,6 @@ static const int32_t synmatrix8[16][8] = {
SN8(0xfb8e3130), SN8(0xf8275a10), SN8(0xfe70747c), SN8(0x06a6d988) }
};
-/* Uncomment the following line to enable high precision build of SBC encoder */
-
/* #define SBC_HIGH_PRECISION */
#ifdef SBC_HIGH_PRECISION
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Johan Hedberg @ 2013-09-16 8:19 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <63CB3872-A353-4997-9A00-E746DA99E6C6@holtmann.org>
Hi Marcel,
On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> > @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> > }
> > EXPORT_SYMBOL(bt_sock_wait_state);
> >
>
> add a small comment here that this needs to be called with sk lock held.
I suppose I should do that to the old bt_sock_wait_state function too?
(which btw my wait_ready function is essentially a copy of, with the
exception of which condition it looks for)
> > +int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
> > +{
> > + DECLARE_WAITQUEUE(wait, current);
> > + unsigned long timeo;
> > + int err = 0;
> > +
> > + BT_DBG("sk %p", sk);
> > +
> > + timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> > +
> > + add_wait_queue(sk_sleep(sk), &wait);
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
> > + if (!timeo) {
> > + err = -EAGAIN;
> > + break;
> > + }
> > +
> > + if (signal_pending(current)) {
> > + err = sock_intr_errno(timeo);
> > + break;
> > + }
> > +
> > + release_sock(sk);
> > + timeo = schedule_timeout(timeo);
> > + lock_sock(sk);
> > + set_current_state(TASK_INTERRUPTIBLE);
> > +
> > + err = sock_error(sk);
> > + if (err)
> > + break;
> > + }
> > + __set_current_state(TASK_RUNNING);
> > + remove_wait_queue(sk_sleep(sk), &wait);
> > +
> > + return err;
> > +}
> > +EXPORT_SYMBOL(bt_sock_wait_ready);
> > +
> > #ifdef CONFIG_PROC_FS
> > struct bt_seq_state {
> > struct bt_sock_list *l;
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 0098af8..ad95b42 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> > if (sk->sk_state != BT_CONNECTED)
> > return -ENOTCONN;
> >
> > + lock_sock(sk);
> > + err = bt_sock_wait_ready(sk, msg->msg_flags);
> > + release_sock(sk);
> > + if (err)
> > + return err;
> > +
>
> After starting to look into this now, I am not sure we have proper
> locking in that function in the first place. Can you check that it is
> actually fine not holding the socket lock for the send operation
> itself.
What procedure did you have in mind for checking what's fine and what's
not? Some quick googling and trying to look at the code of other address
families didn't help me getting much wiser about this.
This is the way that RFCOMM and L2CAP sockets seem to always have done
locking and we haven't seen any obvious breakage because of it. The
RFCOMM side does indeed use lock_sock for the send operation and L2CAP
doesn't. However, I think the reason it works is that l2cap_chan_lock
(which is used before calling l2cap_chan_send) serves a similar purpose.
> > --- a/net/bluetooth/rfcomm/sock.c
> > +++ b/net/bluetooth/rfcomm/sock.c
> > @@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> > struct sock *sk = sock->sk;
> > struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
> > struct sk_buff *skb;
> > - int sent = 0;
> > + int err, sent = 0;
> >
> > if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
> > return -ENOTCONN;
> > @@ -559,9 +559,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> >
> > lock_sock(sk);
> >
> > + err = bt_sock_wait_ready(sk, msg->msg_flags);
> > + if (err) {
> > + release_sock(sk);
> > + return err;
> > + }
> > +
>
> I would be fine with using the sent variable here. And then of course
> create a label for the failure case. Assuming bt_sock_wait_ready()
> returning 0 on success you could use it to initialize sent here.
>
> However at minimum, I want a label here to jump at the end of the
> function in error case. I like to keep the number of lock_sock +
> release_sock balanced.
Makes sense. I'll do these improvements to the next version.
> Which is something that needs to be fixed for l2cap_sock_recvmsg()
> actually.
I'll see if I can roll up a separate patch for that.
Johan
^ 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