* [PATCH 2/2] bluetooth: add support for controller in MacBookPro6,2
From: Will Woods @ 2010-09-17 20:38 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, kernel, Will Woods
In-Reply-To: <1284755883-10795-1-git-send-email-wwoods@redhat.com>
Once again the device class is ff(vend.) instead of e0(wlcon).
output from 'usb-devices':
T: Bus=01 Lev=03 Prnt=03 Port=02 Cnt=03 Dev#= 8 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=05ac ProdID=8218 Rev=00.22
S: Manufacturer=Apple Inc.
S: Product=Bluetooth USB Host Controller
C: #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=0mA
I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
I: If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=(none)
I: If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)
---
drivers/bluetooth/btusb.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index eac44e4..320e798 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -65,6 +65,9 @@ static struct usb_device_id btusb_table[] = {
/* Apple iMac11,1 */
{ USB_DEVICE(0x05ac, 0x8215) },
+ /* Apple MacBookPro6,2 */
+ { USB_DEVICE(0x05ac, 0x8218) },
+
/* AVM BlueFRITZ! USB v2.0 */
{ USB_DEVICE(0x057c, 0x3800) },
--
1.7.2.3
^ permalink raw reply related
* [PATCH 1/2] bluetooth: add support for controller in MacBookPro7,1
From: Will Woods @ 2010-09-17 20:38 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, kernel, Will Woods
As with iMac11,1 the device class is ff(vend.) instead of e0(wlcon).
output from 'usb-devices':
T: Bus=04 Lev=02 Prnt=04 Port=00 Cnt=01 Dev#= 5 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=05ac ProdID=8213 Rev=01.86
S: Manufacturer=Apple Inc.
S: Product=Bluetooth USB Host Controller
S: SerialNumber=58B0359C28ED
C: #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=0mA
I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
I: If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
I: If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=00 Driver=(none)
---
drivers/bluetooth/btusb.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d22ce3c..eac44e4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -59,6 +59,9 @@ static struct usb_device_id btusb_table[] = {
/* Generic Bluetooth USB device */
{ USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
+ /* Apple MacBookPro7,1 */
+ { USB_DEVICE(0x05ac, 0x8213) },
+
/* Apple iMac11,1 */
{ USB_DEVICE(0x05ac, 0x8215) },
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Replace hard code of fixed channels bit mask
From: Gustavo F. Padovan @ 2010-09-17 18:07 UTC (permalink / raw)
To: Ville Tervo
Cc: haijun liu, linux-bluetooth@vger.kernel.org, Mat Martineau,
dantian.ip
In-Reply-To: <20100917085047.GN14330@null>
Hi Ville,
* Ville Tervo <ville.tervo@nokia.com> [2010-09-17 11:50:47 +0300]:
> Hi Gustavo,
>
> On Thu, Sep 16, 2010 at 04:16:54PM +0200, ext Gustavo F. Padovan wrote:
> > Hi Haijun,
> >
> > * haijun liu <liuhaijun.er@gmail.com> [2010-09-16 15:15:18 +0800]:
> >
> > > This patch add fixed channels bit mask definition for
> > > L2CAP_FIXCHAN_NULLID
> > > L2CAP_FIXCHAN_SIGNAL
> > > L2CAP_FIXCHAN_CONNLESS
> > > L2CAP_FIXCHAN_A2MP
> > > And replace hard code in source file with the macro.
> > >
> > > Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
> > > ---
> > > include/net/bluetooth/l2cap.h | 6 ++++++
> > > net/bluetooth/l2cap.c | 2 +-
> > > 2 files changed, 7 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > > index 6c24144..e4fe2c7 100644
> > > --- a/include/net/bluetooth/l2cap.h
> > > +++ b/include/net/bluetooth/l2cap.h
> > > @@ -97,6 +97,12 @@ struct l2cap_conninfo {
> > > #define L2CAP_FEAT_FCS 0x00000020
> > > #define L2CAP_FEAT_FIXED_CHAN 0x00000080
> > >
> > > +/* L2CAP fixed channel bitmask */
> > > +#define L2CAP_FIXCHAN_NULLID 0x00
> > > +#define L2CAP_FIXCHAN_SIGNAL 0x02
> > > +#define L2CAP_FIXCHAN_CONNLESS 0x04
> > > +#define L2CAP_FIXCHAN_A2MP 0x08
> >
> > That's wrong, signaling channel is 0x01, connless is 0x02 and A2MP is
> > 0x03. And if you haven't noted we already have macros for the signaling
> > and connectionless channels:
>
> But these are bit masks and looks ok to me except L2CAP_FIXCHAN_A2MP which I
> could not find from the list.. Check Vol 3 Part A 4.13.
>
> > #define L2CAP_CID_SIGNALING 0x0001
> > #define L2CAP_CID_CONN_LESS 0x0002
>
> These are channel ids.
Yes, you guys are completely right. I was misunderstanding that piece of
code about fixed channels. Thanks for clarify. ;)
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH] Change the for loop to read blutooth clock in MCAP CSP
From: Santiago Carot-Nemesio @ 2010-09-17 12:37 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: Santiago Carot-Nemesio, linux-bluetooth
In-Reply-To: <F43E63DF-F78C-403B-B960-F302339BF7C3@signove.com>
Hi Elvis,
On 09/17/10 13:56, Elvis Pfützenreuter wrote:
> On Sep 17, 2010, at 4:52 AM, Santiago Carot-Nemesio wrote:
>
>> This is only a cosmetic change. Using a do..while loop
>> we avoid direct manipulation of counter loop variable
>> when error happens reading the BT clock.
>> ---
>> health/mcap_sync.c | 10 +++++-----
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/health/mcap_sync.c b/health/mcap_sync.c
>> index bc1ffcd..78cb163 100644
>> --- a/health/mcap_sync.c
>> +++ b/health/mcap_sync.c
>> @@ -406,18 +406,18 @@ static void initialize_caps(struct mcap_mcl *mcl)
>>
>> /* Do clock read a number of times and measure latency */
>> avg = 0;
>> - for (i = 0; i< 20; ++i) {
>> + i = 0;
>> + do {
>> clock_gettime(CLK,&t1);
>> - if (!read_btclock(mcl,&btclock,&btaccuracy)) {
>> - --i;
>> + if (!read_btclock(mcl,&btclock,&btaccuracy))
>> continue;
>> - }
>> clock_gettime(CLK,&t2);
>>
>> latency = time_us(&t2) - time_us(&t1);
>> latencies[i] = latency;
>> avg += latency;
>> - }
>> + i++;
>> + } while (i< 20);
>> avg /= 20;
>>
>> /* Calculate deviation */
>> --
>> 1.7.2.3
>
> Looking again now, this (the logic, not the patch) may lead to an infinite loop, if for some reason the clock simply can't be read. One more thing to be fixed.--
Yes, you are rigth, I saw it too but I was waiting to speak with you. It
may be better set a maximum number of attempts and return a CSP error
code if it is not possible synchronize with remote device.
For now I think that this patch looks better than use a for loop here :P
^ permalink raw reply
* Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections
From: Matthew Garrett @ 2010-09-17 12:24 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-bluetooth, linux-usb, marcel
In-Reply-To: <201009171039.54110.oneukum@suse.de>
On Fri, Sep 17, 2010 at 10:39:54AM +0200, Oliver Neukum wrote:
> Am Donnerstag, 16. September 2010, 19:58:13 schrieb Matthew Garrett:
> > Bluetooth runtime PM interacts badly with input devices - the connection
> > will be dropped if the device becomes idle, resulting in noticable lag when
> > the user interacts with the input device again. Bump the pm runtime count
> > when the device is associated and release it when it's disassociated in
> > order to avoid this.
>
> What is the effect on battery live of the external devices?
That's a reasonable question. I'll see what I can measure.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH] Bluetooth: Check for SCO type before setting retr. effort
From: Ville Tervo @ 2010-09-17 12:04 UTC (permalink / raw)
To: ext Emeltchenko Andrei; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1284724018-26731-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
On Fri, Sep 17, 2010 at 01:46:58PM +0200, ext Emeltchenko Andrei wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> Check eSCO / SCO type before setting retransmission effort flag
> in Setup SCO command. We found that 0x01 is better for power
> consupmtion but we cannot use it always since controller tries
> to setup eSCO even for old devices.
>
> Might be controller-specific.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Revieved-by: Ville Tervo <ville.tervo@nokia.com>
> ---
> net/bluetooth/hci_conn.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 0b1e460..145993f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -150,7 +150,13 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
> cp.rx_bandwidth = cpu_to_le32(0x00001f40);
> cp.max_latency = cpu_to_le16(0xffff);
> cp.voice_setting = cpu_to_le16(hdev->voice_setting);
> - cp.retrans_effort = 0xff;
> +
> + /* If remote device supports eSCO use optimization for power
> + retransmission effort, otherwise use standard flag do-not-care */
> + if (conn->link->features[3] & LMP_ESCO)
> + cp.retrans_effort = 0x01;
> + else
> + cp.retrans_effort = 0xff;
>
> hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
> }
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Change the for loop to read blutooth clock in MCAP CSP
From: Elvis Pfützenreuter @ 2010-09-17 11:56 UTC (permalink / raw)
To: Santiago Carot-Nemesio; +Cc: linux-bluetooth
In-Reply-To: <1284709950-5302-1-git-send-email-sancane@gmail.com>
On Sep 17, 2010, at 4:52 AM, Santiago Carot-Nemesio wrote:
> This is only a cosmetic change. Using a do..while loop
> we avoid direct manipulation of counter loop variable
> when error happens reading the BT clock.
> ---
> health/mcap_sync.c | 10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/health/mcap_sync.c b/health/mcap_sync.c
> index bc1ffcd..78cb163 100644
> --- a/health/mcap_sync.c
> +++ b/health/mcap_sync.c
> @@ -406,18 +406,18 @@ static void initialize_caps(struct mcap_mcl *mcl)
>
> /* Do clock read a number of times and measure latency */
> avg = 0;
> - for (i = 0; i < 20; ++i) {
> + i = 0;
> + do {
> clock_gettime(CLK, &t1);
> - if (!read_btclock(mcl, &btclock, &btaccuracy)) {
> - --i;
> + if (!read_btclock(mcl, &btclock, &btaccuracy))
> continue;
> - }
> clock_gettime(CLK, &t2);
>
> latency = time_us(&t2) - time_us(&t1);
> latencies[i] = latency;
> avg += latency;
> - }
> + i++;
> + } while (i < 20);
> avg /= 20;
>
> /* Calculate deviation */
> --
> 1.7.2.3
Looking again now, this (the logic, not the patch) may lead to an infinite loop, if for some reason the clock simply can't be read. One more thing to be fixed.
^ permalink raw reply
* [PATCH] Bluetooth: Check for SCO type before setting retr. effort
From: Emeltchenko Andrei @ 2010-09-17 11:46 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Check eSCO / SCO type before setting retransmission effort flag
in Setup SCO command. We found that 0x01 is better for power
consupmtion but we cannot use it always since controller tries
to setup eSCO even for old devices.
Might be controller-specific.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
net/bluetooth/hci_conn.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 0b1e460..145993f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -150,7 +150,13 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
cp.rx_bandwidth = cpu_to_le32(0x00001f40);
cp.max_latency = cpu_to_le16(0xffff);
cp.voice_setting = cpu_to_le16(hdev->voice_setting);
- cp.retrans_effort = 0xff;
+
+ /* If remote device supports eSCO use optimization for power
+ retransmission effort, otherwise use standard flag do-not-care */
+ if (conn->link->features[3] & LMP_ESCO)
+ cp.retrans_effort = 0x01;
+ else
+ cp.retrans_effort = 0xff;
hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
}
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Replace hard code of fixed channels bit mask
From: haijun liu @ 2010-09-17 11:39 UTC (permalink / raw)
To: Ville Tervo
Cc: ext Gustavo F. Padovan, linux-bluetooth@vger.kernel.org,
Mat Martineau, dantian.ip
In-Reply-To: <20100917085047.GN14330@null>
Hi Ville,
On Fri, Sep 17, 2010 at 4:50 PM, Ville Tervo <ville.tervo@nokia.com> wrote:
> Hi Gustavo,
>
> On Thu, Sep 16, 2010 at 04:16:54PM +0200, ext Gustavo F. Padovan wrote:
>> Hi Haijun,
>>
>> * haijun liu <liuhaijun.er@gmail.com> [2010-09-16 15:15:18 +0800]:
>>
>> > This patch add fixed channels bit mask definition for
>> > L2CAP_FIXCHAN_NULLID
>> > L2CAP_FIXCHAN_SIGNAL
>> > L2CAP_FIXCHAN_CONNLESS
>> > L2CAP_FIXCHAN_A2MP
>> > And replace hard code in source file with the macro.
>> >
>> > Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
>> > ---
>> > include/net/bluetooth/l2cap.h | 6 ++++++
>> > net/bluetooth/l2cap.c | 2 +-
>> > 2 files changed, 7 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> > index 6c24144..e4fe2c7 100644
>> > --- a/include/net/bluetooth/l2cap.h
>> > +++ b/include/net/bluetooth/l2cap.h
>> > @@ -97,6 +97,12 @@ struct l2cap_conninfo {
>> > #define L2CAP_FEAT_FCS 0x00000020
>> > #define L2CAP_FEAT_FIXED_CHAN 0x00000080
>> >
>> > +/* L2CAP fixed channel bitmask */
>> > +#define L2CAP_FIXCHAN_NULLID 0x00
>> > +#define L2CAP_FIXCHAN_SIGNAL 0x02
>> > +#define L2CAP_FIXCHAN_CONNLESS 0x04
>> > +#define L2CAP_FIXCHAN_A2MP 0x08
>>
>> That's wrong, signaling channel is 0x01, connless is 0x02 and A2MP is
>> 0x03. And if you haven't noted we already have macros for the signaling
>> and connectionless channels:
>
> But these are bit masks and looks ok to me except L2CAP_FIXCHAN_A2MP which I
> could not find from the list.. Check Vol 3 Part A 4.13.
>
>> #define L2CAP_CID_SIGNALING 0x0001
>> #define L2CAP_CID_CONN_LESS 0x0002
>
> These are channel ids.
>
>
> --
> Ville
>
A2MP means AMP Manager Protocol, it's from <<Core_V3.0 + HS.pdf>>
--
Haijun Liu
^ permalink raw reply
* RE: [RFC] BlueZ D-Bus Sim Access Profile Server API description
From: Nicolas GUILBAUD @ 2010-09-17 11:01 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Suraj Sumangala, Suraj Sumangala, linux-bluetooth,
Jothikumar Mothilal
In-Reply-To: <1284678016.2405.145.camel@localhost.localdomain>
Hi Marcel,
-----Message d'origine-----
De=A0: linux-bluetooth-owner@vger.kernel.org =
[mailto:linux-bluetooth-owner@vger.kernel.org] De la part de Marcel =
Holtmann
Envoy=E9=A0: vendredi 17 septembre 2010 01:00
=C0=A0: Nicolas GUILBAUD
Cc=A0: Suraj Sumangala; Suraj Sumangala; =
linux-bluetooth@vger.kernel.org; Jothikumar Mothilal
Objet=A0: RE: [RFC] BlueZ D-Bus Sim Access Profile Server API =
description
>>that is just because we haven't gotten there yet. Of course if SAP =
takes
over we have to shutdown oFono's handling of the modem. This could be
done similar to the support for Lockdown we have in the oFono TODO list.
Do you have information about the release date of this feature? And =
about the interface to lockdown, is it a D-BUS message?=20
>There is a framework for card services in general, but I think that is
just pure overhead for this.
>And to be honest, my current thinking is that even for pure SIM card
readers with APDU mode, we might wanna support them via oFono anyway.
Having to add SAP support in multiple levels is a bad idea.
Yes but an APDU interface is needed in the SIM driver of oFono, I don't =
see any interface of this kind in the source code.
Maybe it's not the good place to speak about that.
>However in the end this all depends on the hardware and right now I =
have
not had access to SAP capable hardware. Or maybe I did and I didn't know
it yet. Do we have any target hardware in mind?
Yes we have, it's Renesas Modem solution.
Regards
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe =
linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages
From: Oliver Neukum @ 2010-09-17 8:59 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Matthew Garrett, linux-bluetooth, linux-usb, marcel
In-Reply-To: <AANLkTi=NBrb70ZvafQxZMnLZZcmuGEPeon4Vqq4vkpce@mail.gmail.com>
Am Freitag, 17. September 2010, 10:33:30 schrieb Andrei Emeltchenko:
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d22ce3c..3ace025 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
> > usb_anchor_urb(urb, &data->intr_anchor);
> >
> > err = usb_submit_urb(urb, GFP_ATOMIC);
> > - if (err < 0) {
> > - BT_ERR("%s urb %p failed to resubmit (%d)",
> > - hdev->name, urb, -err);
> > + if (err < 0)
> > usb_unanchor_urb(urb);
> > - }
> > }
> >
>
> I do not get the point here. Cannot you just undef BT_ERR if you
> enabled debug? I believe this is standard behavior.
You can undefine BT_ERR, but then you'll miss error reports.
This message may indicate an error, but usually doesn't if
you use runtime pm.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] Bluetooth: Replace hard code of fixed channels bit mask
From: Ville Tervo @ 2010-09-17 8:50 UTC (permalink / raw)
To: ext Gustavo F. Padovan
Cc: haijun liu, linux-bluetooth@vger.kernel.org, Mat Martineau,
dantian.ip
In-Reply-To: <20100916141654.GB15000@vigoh>
Hi Gustavo,
On Thu, Sep 16, 2010 at 04:16:54PM +0200, ext Gustavo F. Padovan wrote:
> Hi Haijun,
>
> * haijun liu <liuhaijun.er@gmail.com> [2010-09-16 15:15:18 +0800]:
>
> > This patch add fixed channels bit mask definition for
> > L2CAP_FIXCHAN_NULLID
> > L2CAP_FIXCHAN_SIGNAL
> > L2CAP_FIXCHAN_CONNLESS
> > L2CAP_FIXCHAN_A2MP
> > And replace hard code in source file with the macro.
> >
> > Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
> > ---
> > include/net/bluetooth/l2cap.h | 6 ++++++
> > net/bluetooth/l2cap.c | 2 +-
> > 2 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 6c24144..e4fe2c7 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -97,6 +97,12 @@ struct l2cap_conninfo {
> > #define L2CAP_FEAT_FCS 0x00000020
> > #define L2CAP_FEAT_FIXED_CHAN 0x00000080
> >
> > +/* L2CAP fixed channel bitmask */
> > +#define L2CAP_FIXCHAN_NULLID 0x00
> > +#define L2CAP_FIXCHAN_SIGNAL 0x02
> > +#define L2CAP_FIXCHAN_CONNLESS 0x04
> > +#define L2CAP_FIXCHAN_A2MP 0x08
>
> That's wrong, signaling channel is 0x01, connless is 0x02 and A2MP is
> 0x03. And if you haven't noted we already have macros for the signaling
> and connectionless channels:
But these are bit masks and looks ok to me except L2CAP_FIXCHAN_A2MP which I
could not find from the list.. Check Vol 3 Part A 4.13.
> #define L2CAP_CID_SIGNALING 0x0001
> #define L2CAP_CID_CONN_LESS 0x0002
These are channel ids.
--
Ville
^ permalink raw reply
* Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections
From: Oliver Neukum @ 2010-09-17 8:39 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linux-bluetooth, linux-usb, marcel
In-Reply-To: <1284659895-27984-1-git-send-email-mjg@redhat.com>
Am Donnerstag, 16. September 2010, 19:58:13 schrieb Matthew Garrett:
> Bluetooth runtime PM interacts badly with input devices - the connection
> will be dropped if the device becomes idle, resulting in noticable lag when
> the user interacts with the input device again. Bump the pm runtime count
> when the device is associated and release it when it's disassociated in
> order to avoid this.
What is the effect on battery live of the external devices?
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages
From: Andrei Emeltchenko @ 2010-09-17 8:33 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linux-bluetooth, linux-usb, marcel
In-Reply-To: <1284659895-27984-2-git-send-email-mjg@redhat.com>
Hi Matthew,
On Thu, Sep 16, 2010 at 8:58 PM, Matthew Garrett <mjg@redhat.com> wrote:
> The main reason for these urbs to error out on submission is that runtime
> pm has kicked in, which is unnecessary noise. Let's just drop them.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
> =A0drivers/bluetooth/btusb.c | =A0 15 +++------------
> =A01 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d22ce3c..3ace025 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_anchor_urb(urb, &data->intr_anchor);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>
I do not get the point here. Cannot you just undef BT_ERR if you
enabled debug? I believe this is standard behavior.
IMO NACK
Regards,
Andrei
> =A0static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags=
)
> @@ -313,11 +310,8 @@ static void btusb_bulk_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_mark_last_busy(data->udev);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>
> =A0static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags=
)
> @@ -402,11 +396,8 @@ static void btusb_isoc_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_anchor_urb(urb, &data->isoc_anchor);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>
> =A0static void inline __fill_isoc_descriptor(struct urb *urb, int len, in=
t mtu)
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH] Change the for loop to read blutooth clock in MCAP CSP
From: Santiago Carot-Nemesio @ 2010-09-17 7:52 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Santiago Carot-Nemesio
This is only a cosmetic change. Using a do..while loop
we avoid direct manipulation of counter loop variable
when error happens reading the BT clock.
---
health/mcap_sync.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/health/mcap_sync.c b/health/mcap_sync.c
index bc1ffcd..78cb163 100644
--- a/health/mcap_sync.c
+++ b/health/mcap_sync.c
@@ -406,18 +406,18 @@ static void initialize_caps(struct mcap_mcl *mcl)
/* Do clock read a number of times and measure latency */
avg = 0;
- for (i = 0; i < 20; ++i) {
+ i = 0;
+ do {
clock_gettime(CLK, &t1);
- if (!read_btclock(mcl, &btclock, &btaccuracy)) {
- --i;
+ if (!read_btclock(mcl, &btclock, &btaccuracy))
continue;
- }
clock_gettime(CLK, &t2);
latency = time_us(&t2) - time_us(&t1);
latencies[i] = latency;
avg += latency;
- }
+ i++;
+ } while (i < 20);
avg /= 20;
/* Calculate deviation */
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Fix unaligned access to l2cap conf data
From: real mz @ 2010-09-17 3:13 UTC (permalink / raw)
To: linux-bluetooth; +Cc: linux-kernel-commits, realmz
In-Reply-To: <1283149112-22164-1-git-send-email-realmz6@gmail.com>
ping
On Mon, Aug 30, 2010 at 2:18 PM, steven.miao <realmz6@gmail.com> wrote:
> From: realmz <realmz6@gmail.com>
>
> In function l2cap_get_conf_opt() and l2cap_add_conf_opt() the address of
> opt->val sometimes is not at the edge of 2-bytes/4-bytes, so 2-bytes/4 bytes
> access will cause data misalignment exeception. Use get_unaligned_le16/32
> and put_unaligned_le16/32 function to avoid data misalignment execption.
> ---
> net/bluetooth/l2cap.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index fadf26b..a07dad8 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -2406,11 +2406,11 @@ static inline int l2cap_get_conf_opt(void **ptr, int *type, int *olen, unsigned
> break;
>
> case 2:
> - *val = __le16_to_cpu(*((__le16 *) opt->val));
> + *val = get_unaligned_le16(opt->val);
> break;
>
> case 4:
> - *val = __le32_to_cpu(*((__le32 *) opt->val));
> + *val = get_unaligned_le32(opt->val);
> break;
>
> default:
> @@ -2437,11 +2437,11 @@ static void l2cap_add_conf_opt(void **ptr, u8 type, u8 len, unsigned long val)
> break;
>
> case 2:
> - *((__le16 *) opt->val) = cpu_to_le16(val);
> + put_unaligned_le16(cpu_to_le16(val), opt->val);
> break;
>
> case 4:
> - *((__le32 *) opt->val) = cpu_to_le32(val);
> + put_unaligned_le32(cpu_to_le32(val), opt->val);
> break;
>
> default:
> --
> 1.5.6.5
>
>
^ permalink raw reply
* Fwd: [PATCH] Bluetooth: Replace hard code of fixed channels bit mask
From: haijun liu @ 2010-09-17 1:08 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <AANLkTik244A2zNFL-yL7CvBKdf3UZDDKq1Ap7HR5+aCR@mail.gmail.com>
Hi Gustavo,
On Thu, Sep 16, 2010 at 10:16 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Haijun,
>
> * haijun liu <liuhaijun.er@gmail.com> [2010-09-16 15:15:18 +0800]:
>
>> This patch add fixed channels bit mask definition for
>> L2CAP_FIXCHAN_NULLID
>> L2CAP_FIXCHAN_SIGNAL
>> L2CAP_FIXCHAN_CONNLESS
>> L2CAP_FIXCHAN_A2MP
>> And replace hard code in source file with the macro.
>>
>> Signed-off-by: Haijun.Liu <Haijun.Liu@Atheros.com>
>> ---
>> include/net/bluetooth/l2cap.h | 6 ++++++
>> net/bluetooth/l2cap.c | 2 +-
>> 2 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 6c24144..e4fe2c7 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -97,6 +97,12 @@ struct l2cap_conninfo {
>> #define L2CAP_FEAT_FCS 0x00000020
>> #define L2CAP_FEAT_FIXED_CHAN 0x00000080
>>
>> +/* L2CAP fixed channel bitmask */
>> +#define L2CAP_FIXCHAN_NULLID 0x00
>> +#define L2CAP_FIXCHAN_SIGNAL 0x02
>> +#define L2CAP_FIXCHAN_CONNLESS 0x04
>> +#define L2CAP_FIXCHAN_A2MP 0x08
>
> That's wrong, signaling channel is 0x01, connless is 0x02 and A2MP is
> 0x03. And if you haven't noted we already have macros for the signaling
> and connectionless channels:
>
> #define L2CAP_CID_SIGNALING 0x0001
> #define L2CAP_CID_CONN_LESS 0x0002
>
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
>
These are not channel id definition, these are feature bit mask values.
And they are used in information exchange process to present FIXED
CHANNELS SUPPORTED.
--
Haijun Liu
^ permalink raw reply
* RE: [RFC] BlueZ D-Bus Sim Access Profile Server API description
From: Marcel Holtmann @ 2010-09-16 23:04 UTC (permalink / raw)
To: Waldemar.Rymarkiewicz
Cc: suraj, Suraj.Sumangala, linux-bluetooth, Jothikumar.Mothilal
In-Reply-To: <99B09243E1A5DA4898CDD8B700111448097968EDE3@EXMB04.eu.tieto.com>
Hi Waldemar,
> >if we talk about the SAP server role found in a mobile phone,
> >then that support clearly needs to interact with the telephony
> >stack. Since when SAP is active the telephony stack needs to
> >be suspended and all SIM transaction being forwarded.
>
> I agree, but a telephony stack needs to care about its state when SAP is active. Not all linux based mobile platforms use ofono.
I think they should use oFono of course since that is the only way to be
modem hardware agnostic in the long run.
> >Currently I would be thinking that the SAP implementation
> >should be done inside oFono actually. Since then you have
> >direct access to the hardware. The D-Bus approach just doesn't
> >sound correct to me. I could be of course wrong, but I can't
> >wrap my mind around on how you can make this work.
>
> I assume that ofono should handle sap transactions (as it's kind of hardware abstraction for bluez) and expose SAP APDU interfaces as i.e Nokia did for sap transactions in spec (http://www.wirelessmodemapi.com/), and not implement sap profile itself. Again, not all use ofono as telephony stack.
>
> Sorry, I don't know ofono well. What's the interface between ofono and bluez?
In this case there will be no direct interface besides oFono registering
the SDP records for SAP.
> >Even with file descriptor passing this doesn't look like the
> >right approach. If we need a hardware abstraction than we
> >either use oFono or we have to create some SAP hardware access
> >abstraction.
>
> I agree with this too. I prefere to have a sap implementation as bluez plugin, but we need to define api for sim operations which could be implemented in ofono or other proprietary stacks. I guess dbus was intended to be kind of hw abstraction api.
> Some time ago Claudio sent proposal implementation of sap server where he defined api for a driver to sim.
I still don't see this as a really good idea. We want modem abstraction
inside oFono and not all over the place.
Regards
Marcel
^ permalink raw reply
* RE: [RFC] BlueZ D-Bus Sim Access Profile Server API description
From: Marcel Holtmann @ 2010-09-16 23:00 UTC (permalink / raw)
To: Nicolas GUILBAUD
Cc: Suraj Sumangala, Suraj Sumangala, linux-bluetooth,
Jothikumar Mothilal
In-Reply-To: <A9AC1023FCA4654D92AC24C2A8F6A09201A2B923@renesas4bis.renesas-rdf.local>
Hi Nicolas,
> >>> I would really appreciate if you can give me any idea about working
> >>> around the above mentioned issues.
> >>
> >> if we talk about the SAP server role found in a mobile phone, then that
> >> support clearly needs to interact with the telephony stack. Since when
> >> SAP is active the telephony stack needs to be suspended and all SIM
> >> transaction being forwarded.
> >>
> >> Currently I would be thinking that the SAP implementation should be done
> >> inside oFono actually. Since then you have direct access to the
> >> hardware. The D-Bus approach just doesn't sound correct to me. I could
> >> be of course wrong, but I can't wrap my mind around on how you can make
> >> this work.
> >>
> >> Even with file descriptor passing this doesn't look like the right
> >> approach. If we need a hardware abstraction than we either use oFono or
> >> we have to create some SAP hardware access abstraction.
> >
> >The advantage I thought d-bus has would be the generic interface it
> >could provide. But, if we have a system with direct access to the Sim
> >access hardware, D-bus could possibly become a bottleneck.
> >
> >I would really appreciate if others who have worked with Sim Access and
> >OFono can give their comments.
> >
> >Also, please share some information on regarding the SIM reader
> >implementation in linux based systems.
> >
> >>
> >> The SAP client role found a carkit is obviously a different story.
>
> I agree with Marcel, because in standard Mobile phone implementation, the SIM card is directly connected to the Modem. To access the SIM card, AT command (or proprietary commands) are needed, oFono implements both of them to access SIM card in Modem side, but in my understanding there is no possibilities to disconnect modem (in oFono) due to SIM SAP.
that is just because we haven't gotten there yet. Of course if SAP takes
over we have to shutdown oFono's handling of the modem. This could be
done similar to the support for Lockdown we have in the oFono TODO list.
> In the case where SIM card is connected to the Application processor (Linux side), it's missing the SIM stack (APDU server...). I don't find any SIM implementation in Linux, may be we have to specify it.
There is a framework for card services in general, but I think that is
just pure overhead for this.
And to be honest, my current thinking is that even for pure SIM card
readers with APDU mode, we might wanna support them via oFono anyway.
Having to add SAP support in multiple levels is a bad idea.
However in the end this all depends on the hardware and right now I have
not had access to SAP capable hardware. Or maybe I did and I didn't know
it yet. Do we have any target hardware in mind?
Regards
Marcel
^ permalink raw reply
* Re: [RFC] D-Bus API for out of band association model
From: Johan Hedberg @ 2010-09-16 18:11 UTC (permalink / raw)
To: Andrzej Kaczmarek
Cc: jaikumar Ganesh, Claudio Takahasi, linux-bluetooth,
par-gunnar.p.hjalmdahl
In-Reply-To: <4C9212E4.4090204@tieto.com>
Hi Andrzej,
It would be nice if you could use some empty lines in your replies. It
was quite hard to distinguish the quoted text from your own.
On Thu, Sep 16, 2010, Andrzej Kaczmarek wrote:
> In case of NFC I guess data will be coming from some application or
> perhaps a daemon which can understand data coming from lower layers,
> which I guess will rather process raw data. So these data will come
> from the same level as bluetoothd resides or above.
Agreed. It might also come from below if the plugin talks to some kernel
driver handling the OOB communication.
> This is something that agent can communicate freely with. Also from
> our point of view OOB data is used in the same way as i.e. passkey and
> this is what agent provides. We don't care how it will receive such
> data.
The same goes for the plugin approach. If we wanted to care about the
actual OOB mechanism the would be no need to abstract this and make it
pluggable.
> Also Jakiumar gave quite good example where NFC application receives
> some data which is identified as BT OOB data so it initiates pairing
> with device specific agent which will provide these data.
Agreed. I do see now that a D-Bus based approach might make more sense
in the end. On the other hand, we could still abstract this inside the
daemon for plugins, and then there'd simply be a special (default)
plugin that'd expose this over D-Bus.
> Do I understand correctly that also plugin or daemon should manage
> received data? Won't it make bluetoothd store unnecessary data, i.e.
> for devices we won't use anyway?
This would be completely implementation specific. It could be within the
daemon or delegated to some external component. The only certain thing
would be an interface for the core daemon to get the OOB data when it
gets an IO capability request.
> Also what in case we want to use several different secure channels?
> Multiple plugins?
Yes, that would be possible.
> In case of agent, we make single call and no need to worry where the
> data come from. Agent is platform dependent anyway so platform vendor
> can put required logic there.
Right, but at the same time it would also require you to multiplex all
potential available OOB methods through a single agent. So in that sense
the agent approach is more restrictive (with plugins we could at least
allow multiple of them).
One thing I'm quite concerned about is that I wouldn't want to add an
extra rountrip to an external process for every single IO capability
request that we get. This extra work should only be done if we know that
there's a non-zero probability for the existence of OOB data.
We could either have the agent tell bluetoothd about the OOB capability
somehow, or we could define some new sort of external "OOB data
provider" D-Bus object. The benefit of using a new object separate from
the agent is that we could allow multiple of them whereas there can only
be one agent right now. And only if there'd be one or more of these new
types of objects registered would bluetoothd make the extra roundtrip
when it gets an IO capability request.
Johan
^ permalink raw reply
* [PATCH 3/3] bluetooth: Enable USB autosuspend by default on btusb
From: Matthew Garrett @ 2010-09-16 17:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: linux-usb, marcel, Matthew Garrett
In-Reply-To: <1284659895-27984-1-git-send-email-mjg@redhat.com>
We've done this for a while in Fedora without any obvious problems other
than some interaction with input devices. Those should be fixed now, so
let's try this in mainline.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
drivers/bluetooth/btusb.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3ace025..03b64e4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1014,6 +1014,8 @@ static int btusb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, data);
+ usb_enable_autosuspend(interface_to_usbdev(intf));
+
return 0;
}
--
1.7.2.3
^ permalink raw reply related
* [PATCH 2/3] bluetooth: Remove some unnecessary error messages
From: Matthew Garrett @ 2010-09-16 17:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: linux-usb, marcel, Matthew Garrett
In-Reply-To: <1284659895-27984-1-git-send-email-mjg@redhat.com>
The main reason for these urbs to error out on submission is that runtime
pm has kicked in, which is unnecessary noise. Let's just drop them.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
drivers/bluetooth/btusb.c | 15 +++------------
1 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d22ce3c..3ace025 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
usb_anchor_urb(urb, &data->intr_anchor);
err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0) {
- BT_ERR("%s urb %p failed to resubmit (%d)",
- hdev->name, urb, -err);
+ if (err < 0)
usb_unanchor_urb(urb);
- }
}
static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
@@ -313,11 +310,8 @@ static void btusb_bulk_complete(struct urb *urb)
usb_mark_last_busy(data->udev);
err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0) {
- BT_ERR("%s urb %p failed to resubmit (%d)",
- hdev->name, urb, -err);
+ if (err < 0)
usb_unanchor_urb(urb);
- }
}
static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags)
@@ -402,11 +396,8 @@ static void btusb_isoc_complete(struct urb *urb)
usb_anchor_urb(urb, &data->isoc_anchor);
err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0) {
- BT_ERR("%s urb %p failed to resubmit (%d)",
- hdev->name, urb, -err);
+ if (err < 0)
usb_unanchor_urb(urb);
- }
}
static void inline __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
--
1.7.2.3
^ permalink raw reply related
* [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections
From: Matthew Garrett @ 2010-09-16 17:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: linux-usb, marcel, Matthew Garrett
Bluetooth runtime PM interacts badly with input devices - the connection
will be dropped if the device becomes idle, resulting in noticable lag when
the user interacts with the input device again. Bump the pm runtime count
when the device is associated and release it when it's disassociated in
order to avoid this.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
net/bluetooth/hidp/core.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index bfe641b..a4489a7 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
#include <linux/file.h>
#include <linux/init.h>
#include <linux/wait.h>
+#include <linux/pm_runtime.h>
#include <net/sock.h>
#include <linux/input.h>
@@ -622,6 +623,14 @@ static int hidp_session(void *arg)
return 0;
}
+static struct hci_dev *hidp_get_hci(struct hidp_session *session)
+{
+ bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
+ bdaddr_t *dst = &bt_sk(session->ctrl_sock->sk)->dst;
+
+ return hci_get_route(dst, src);
+}
+
static struct device *hidp_get_device(struct hidp_session *session)
{
bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
@@ -819,6 +828,7 @@ fault:
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
{
struct hidp_session *session, *s;
+ struct hci_dev *hdev;
int err;
BT_DBG("");
@@ -889,6 +899,10 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
hidp_input_event(session->input, EV_LED, 0, 0);
}
+ hdev = hidp_get_hci(session);
+ pm_runtime_get(hdev->parent);
+ hci_dev_put(hdev);
+
up_write(&hidp_session_sem);
return 0;
@@ -925,6 +939,7 @@ failed:
int hidp_del_connection(struct hidp_conndel_req *req)
{
struct hidp_session *session;
+ struct hci_dev *hdev;
int err = 0;
BT_DBG("");
@@ -952,6 +967,9 @@ int hidp_del_connection(struct hidp_conndel_req *req)
} else
err = -ENOENT;
+ hdev = hidp_get_hci(session);
+ pm_runtime_put(hdev->parent);
+ hci_dev_put(hdev);
up_read(&hidp_session_sem);
return err;
}
--
1.7.2.3
^ permalink raw reply related
* Re: Possible regression with skb_clone() in 2.6.36
From: Mat Martineau @ 2010-09-16 16:40 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth, marcel
In-Reply-To: <20100916001012.GA5656@vigoh>
Gustavo -
Since this is now a only a discussion of blocking behavior with ERTM
(and not a regression with skb_clone()), I've removed linux-kernel and
netdev from the cc list. If you think they still need to see this
thread, please add them back in.
On Wed, 15 Sep 2010, Gustavo F. Padovan wrote:
> * Gustavo F. Padovan <padovan@profusion.mobi> [2010-09-10 16:45:09 -0300]:
>
>> Hi Mat,
>>
>> * Mat Martineau <mathewm@codeaurora.org> [2010-09-10 09:53:31 -0700]:
>>
>>>
>>> Gustavo -
>>>
>>> I'm not sure why the streaming code used to work, but this does not
>>> look like an skb_clone() problem. Your patch to remove the
>>> skb_clone() call in l2cap_streaming_send() addresses the root cause of
>>> this crash.
>>>
>>> On Wed, 8 Sep 2010, Gustavo F. Padovan wrote:
>>>
>>>> I've been experiencing some problems when running the L2CAP Streaming mode in
>>>> 2.6.36. The system quickly runs in an Out Of Memory condition and crash. That
>>>> wasn't happening before, so I think we may have a regression here (I didn't
>>>> find where yet). The crash log is below.
>>>>
>>>> The following patch does not fix the regression, but shows that removing the
>>>> skb_clone() call from l2cap_streaming_send() we workaround the problem. The
>>>> patch is good anyway because it saves memory and time.
>>>>
>>>> By now I have no idea on how to fix this.
>>>>
>>>> <snip>
>>>
>>> This has to do with the sk->sk_wmem_alloc accounting that controls the
>>> amount of write buffer space used on the socket.
>>>
>>> When the L2CAP streaming mode socket segments its data, it allocates
>>> memory using sock_alloc_send_skb() (via bt_skb_send_alloc()). Before
>>> that allocation call returns, skb_set_owner_w() is called on the new
>>> skb. This adds to sk->sk_wmem_alloc and sets skb->destructor so that
>>> sk->sk_wmem_alloc is correctly updated when the skb is freed.
>>>
>>> When that skb is cloned, the clone is not "owned" by the write buffer.
>>> The clone's destructor is set to NULL in __skb_clone(). The version
>>> of l2cap_streaming_send() that runs out of memory is passing the
>>> non-owned skb clone down to the HCI layer. The original skb (the one
>>> that's "owned by w") is immediately freed, which adjusts
>>> sk->sk_wmem_alloc back down - the socket thinks it has unlimited write
>>> buffer space. As a result, bt_skb_send_alloc() never blocks waiting
>>> for buffer space (or returns EAGAIN for nonblocking writes) and the
>>> HCI send queue keeps growing.
>>
>> If the problem is what you are saying, add a skb_set_owner_w(skb, sk) on
>> the cloned skb should solve the problem, but it doesn't. That's exactly
>> what tcp_transmit_skb() does. Also that just appeared in 2.6.36, is was
>> working fine before, i.e, we have a regression here.
>>
>>>
>>> This isn't a problem for the ERTM sends, because the original skbs are
>>> kept in the ERTM tx queue until they are acked. Once they're acked,
>>> the write buffer space is freed and additional skbs can be allocated.
>>
>> It affects ERTM as well, but in that case the kernel doesn't crash
>> because ERTM block on sending trying to allocate memory. Then we are not
>> able to receive any ack (everything stays queued in sk_backlog_queue as
>> the sk is owned by the user) and ERTM stalls.
>
> By reverting
>
> commit 218bb9dfd21472128f86b38ad2eab123205c2991
> Author: Gustavo F. Padovan <padovan@profusion.mobi>
> Date: Mon Jun 21 18:53:22 2010 -0300
>
> Bluetooth: Add backlog queue to ERTM code
>
> backlog queue is the canonical mechanism to avoid race conditions due
> interrupts in bottom half context. After the socket lock is released the
> net core take care of push all skb in its backlog queue.
>
> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>
>
> we can workaround the bug. It's not the real cause of the bug because the
> backlog queue was working before 2.6.36, but removing the backlog queue scheme
> make L2CAP works again and also give us more time to find the real cause of the
> problem.
>
> Before implement the backlog queue L2CAP had his own lock scheme to avoid race
> conditions. The point is that the backlog queue adds too much serialization to
> L2CAP, that was the cause of the ERTM bug. The old scheme (the one we are going
> to use after revert this commit) just serialize access to some places.
>
> By not using the backlog queue, we can receive the acknowledgement frames more
> quickly and then free the acked frames quickly. In fact the old scheme is
> looks better than backlog queue.
>
> I think we should proceed with the revert with this commit in mainline, and at the
> same time try to find the root cause of the problem.
I don't think the backlog patch should be reverted. Although the old
code would appear to get things working again, it's only because locks
were being ignored in some important cases. Maybe it seems ok most of
the time, but the ERTM socket state was not being properly protected.
The backlog is the right way to do it, but also depends on correct
lock handling in the rest of L2CAP.
I think this is one of the locking issues from this message to
linux-bluetooth ("ERTM known bugs/regressions (was Re: [PATCH 0/8]
...)") on August 2:
http://www.spinics.net/lists/linux-bluetooth/msg06734.html
We've both arrived at the root cause already: incoming frames
(including acks) get stuck on the backlog queue while the socket is
locked waiting for memory -- and no memory gets freed until the acks
are processed.
The only time the socket lock is held for a long time is when waiting
for memory, so I think the best solution is to not hold the lock while
allocating. This should be safe, since the only socket state that's
changed during bt_skb_send_alloc() is sk->sk_wmem_alloc, which is an
atomic_t.
Then some changes are needed to these functions:
l2cap_sar_segment_sdu() and l2cap_create_iframe_pdu(): Take some args
on the stack (like remote_mps) instead of accessing l2cap_pinfo.
Don't add to TX_QUEUE(sk) within these functions (let the caller do
that).
l2cap_sock_sendmsg(): Copy necessary data to local variables from
l2cap_pinfo while the lock is held. release_sock(sk), then call
l2cap_sar_segment_sdu() (which will work for the "one PDU" case too)
with the new args for mps, fcs, etc. After l2cap_sar_segment_sdu()
returns, check the socket state again (like the code at the beginning
of l2cap_sock_sendmsg()) and then lock_sock(sk), queue the data, and
send it.
(I know it would be better and clearer to send a patch for this, but
I have some other things to fix right now!)
The other locking problem (with ERTM timers) also deserves some
attention, but maybe in a new email thread!
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply
* Re: CSP implementation for MCAP
From: Johan Hedberg @ 2010-09-16 15:03 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <F9784FEC-442D-4448-9BA1-234AA9963ECE@signove.com>
Hi Elvis,
On Thu, Sep 16, 2010, Elvis Pfützenreuter wrote:
> > On Wed, Sep 15, 2010, Elvis Pfützenreuter wrote:
> >>> This is the repository for the CSP implementation, rebased over the
> >>> recently accepted MCAP, for your appreciation:
> >>>
> >>> git://gitorious.org/bluez-epx/bluez-epx.git csp
> >>>
> >>> or
> >>>
> >>> http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
> >>
> >> And I am happy to announce that this CSP implementation passed on PTS
> >> :) Fixes are on topmost patch of the repository.
> >
> > Good to hear about the progress with the PTS :)
> >
> > Unfortunately you'll need to rebase again since I pushed some cleanups
> > to the MCAP code. Could you also get rid of the unnecessary
> > double-pointers and type casts which aren't needed anymore now that
> > mcap_send_data accepts void *.
>
> It's done, and tested.
Thanks. I still had to do quite a lot of coding style fixes as well as
plug a memory leak, but the patches are now pushed upstream.
Next steps:
- Get rid of the forward declarations of static functions
wherever possible. It looked like you could accomplish that
with a simple reordering of the functions.
- Get rid of raw HCI access (hci_open_dev). bluetoothd shouldn't use
this type of sockets in the future at all. You'll need to add a proper
callback to the adapter_opts and a function to plugins/hciopts.c and
use that instead. Then export a function like btd_adapter_read_clock
for the plugin to use.
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