2011-08-12 12:56:28

by Peter Hurley

[permalink] [raw]
Subject: [PATCH BlueZ] Increase timeout before initiating AVDTP connection

QVZEVFBfQ09OTkVDVF9USU1FT1VUIGNvbnRyb2xzIHRoZSBkZWxheSBiZXR3ZWVuIEhTUC9IRlAN
CmNvbm5lY3Rpb24gZXN0YWJsaXNobWVudCBhbmQgQVZEVFAgc2lnbmFsIGNoYW5uZWwgZXN0YWJs
aXNobWVudC4NClRoZSBvcmlnaW5hbCB2YWx1ZSBvZiAxIHNlYy4gaXMgdG9vIHNob3J0IHRvIGF2
b2lkIHJhY2luZyBmb3IgQVZEVFANCmNvbm5lY3Rpb24gZXN0YWJsaXNobWVudCAoZWcuLCBpZiB0
aGUgZGV2aWNlIGNvbnRpbnVlcyB0byBjb25maWd1cmUNCnRoZSBIRlAgc2VydmljZSBsZXZlbCBj
b25uZWN0aW9uIHdpdGggYWRkJ2wgQVQgY21kcykuDQoNClNvbWUgZGV2aWNlcyBoYXZlIGJyb2tl
biBBVkRUUCBpbXBsZW1lbnRhdGlvbnMgdGhhdCBqdXN0IGNhbm5vdA0KaGFuZGxlIHRoZSByYWNl
IGNvbmRpdGlvbnMgdGhhdCBhcmlzZSBpZiBib3RoIGRldmljZXMgYXJlIGF0dGVtcHRpbmcNCnN0
cmVhbSBlc3RhYmxpc2htZW50IGF0IHRoZSBzYW1lIHRpbWUuIEhvd2V2ZXIsIHRoZXNlIGNvbmRp
dGlvbnMgYXJpc2UNCm9ubHkgd2hlbiB0aGUgcmVtb3RlIGRldmljZSBpcyB0aGUgQUNMIGluaXRp
YXRvciAoYW5kIGluIHByYWN0aWNlLCB0aGUNClJGQ09NTSBpbml0aWF0b3IgYXMgd2VsbCkuICBU
aGlzIGZpeCBidW1wcyBvdXQgdGhlIHRpbWVvdXQgdmFsdWUgb25seQ0Kd2hlbiB0aGUgaGVhZHNl
dCBoYXMgaW5pdGlhdGVkIHRoZSBsaW5rLg0KLS0tDQogYXVkaW8vZGV2aWNlLmMgIHwgICAgOSAr
KysrKysrKy0NCiBhdWRpby9oZWFkc2V0LmMgfCAgIDE2ICsrKysrKysrKysrKysrKysNCiBhdWRp
by9oZWFkc2V0LmggfCAgICAzICsrKw0KIGF1ZGlvL21hbmFnZXIuYyB8ICAgIDEgKw0KIDQgZmls
ZXMgY2hhbmdlZCwgMjggaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdp
dCBhL2F1ZGlvL2RldmljZS5jIGIvYXVkaW8vZGV2aWNlLmMNCmluZGV4IDE4ZTg3M2UuLjBiZDE0
NDMgMTAwNjQ0DQotLS0gYS9hdWRpby9kZXZpY2UuYw0KKysrIGIvYXVkaW8vZGV2aWNlLmMNCkBA
IC02MSw2ICs2MSw3IEBADQogDQogI2RlZmluZSBDT05UUk9MX0NPTk5FQ1RfVElNRU9VVCAyDQog
I2RlZmluZSBBVkRUUF9DT05ORUNUX1RJTUVPVVQgMQ0KKyNkZWZpbmUgQVZEVFBfQ09OTkVDVF9U
SU1FT1VUX0JPT1NUIDENCiAjZGVmaW5lIEhFQURTRVRfQ09OTkVDVF9USU1FT1VUIDENCiANCiB0
eXBlZGVmIGVudW0gew0KQEAgLTMwNSw2ICszMDYsNyBAQCBzdGF0aWMgZ2Jvb2xlYW4gYXZkdHBf
Y29ubmVjdF90aW1lb3V0KGdwb2ludGVyIHVzZXJfZGF0YSkNCiBzdGF0aWMgZ2Jvb2xlYW4gZGV2
aWNlX3NldF9hdmR0cF90aW1lcihzdHJ1Y3QgYXVkaW9fZGV2aWNlICpkZXYpDQogew0KIAlzdHJ1
Y3QgZGV2X3ByaXYgKnByaXYgPSBkZXYtPnByaXY7DQorCWd1aW50IHRpbWVvdXQgPSBBVkRUUF9D
T05ORUNUX1RJTUVPVVQ7DQogDQogCWlmICghZGV2LT5zaW5rKQ0KIAkJcmV0dXJuIEZBTFNFOw0K
QEAgLTMxMiw3ICszMTQsMTIgQEAgc3RhdGljIGdib29sZWFuIGRldmljZV9zZXRfYXZkdHBfdGlt
ZXIoc3RydWN0IGF1ZGlvX2RldmljZSAqZGV2KQ0KIAlpZiAocHJpdi0+YXZkdHBfdGltZXIpDQog
CQlyZXR1cm4gRkFMU0U7DQogDQotCXByaXYtPmF2ZHRwX3RpbWVyID0gZ190aW1lb3V0X2FkZF9z
ZWNvbmRzKEFWRFRQX0NPTk5FQ1RfVElNRU9VVCwNCisJLyogaWYgdGhlIGhlYWRzZXQgaXMgdGhl
IEhTUC9IRlAgUkZDT01NIGluaXRpYXRvciwgZ2l2ZSB0aGUgaGVhZHNldA0KKwkgICB0aW1lIHRv
IGluaXRpYXRlIEFWRFRQIHNpZ25hbGxpbmcgKGFuZCBhdm9pZCBmdXJ0aGVyIHJhY2luZykgKi8N
CisJaWYgKGRldi0+aGVhZHNldCAmJiBnZXRfcmZjb21tX2luaXRpYXRvcihkZXYpKQ0KKwkJdGlt
ZW91dCArPSBBVkRUUF9DT05ORUNUX1RJTUVPVVRfQk9PU1Q7DQorDQorCXByaXYtPmF2ZHRwX3Rp
bWVyID0gZ190aW1lb3V0X2FkZF9zZWNvbmRzKHRpbWVvdXQsDQogCQkJCQkJCWF2ZHRwX2Nvbm5l
Y3RfdGltZW91dCwNCiAJCQkJCQkJZGV2KTsNCiANCmRpZmYgLS1naXQgYS9hdWRpby9oZWFkc2V0
LmMgYi9hdWRpby9oZWFkc2V0LmMNCmluZGV4IDhlNjNhZmMuLjY2YTI4ZjIgMTAwNjQ0DQotLS0g
YS9hdWRpby9oZWFkc2V0LmMNCisrKyBiL2F1ZGlvL2hlYWRzZXQuYw0KQEAgLTE2Nyw2ICsxNjcs
NyBAQCBzdHJ1Y3QgaGVhZHNldCB7DQogDQogCWdib29sZWFuIGhmcF9hY3RpdmU7DQogCWdib29s
ZWFuIHNlYXJjaF9oZnA7DQorCWdib29sZWFuIHJmY29tbV9pbml0aWF0b3I7DQogDQogCWhlYWRz
ZXRfc3RhdGVfdCBzdGF0ZTsNCiAJc3RydWN0IHBlbmRpbmdfY29ubmVjdCAqcGVuZGluZzsNCkBA
IC0xNjMzLDYgKzE2MzQsNyBAQCBzdGF0aWMgaW50IHJmY29tbV9jb25uZWN0KHN0cnVjdCBhdWRp
b19kZXZpY2UgKmRldiwgaGVhZHNldF9zdHJlYW1fY2JfdCBjYiwNCiAJfQ0KIA0KIAlocy0+aGZw
X2FjdGl2ZSA9IGhzLT5oZnBfaGFuZGxlICE9IDAgPyBUUlVFIDogRkFMU0U7DQorCWhzLT5yZmNv
bW1faW5pdGlhdG9yID0gRkFMU0U7DQogDQogCWhlYWRzZXRfc2V0X3N0YXRlKGRldiwgSEVBRFNF
VF9TVEFURV9DT05ORUNUSU5HKTsNCiANCkBAIC0yNDQyLDYgKzI0NDQsMjAgQEAgdm9pZCBzZXRf
aGZwX2FjdGl2ZShzdHJ1Y3QgYXVkaW9fZGV2aWNlICpkZXYsIGdib29sZWFuIGFjdGl2ZSkNCiAJ
aHMtPmhmcF9hY3RpdmUgPSBhY3RpdmU7DQogfQ0KIA0KK2dib29sZWFuIGdldF9yZmNvbW1faW5p
dGlhdG9yKHN0cnVjdCBhdWRpb19kZXZpY2UgKmRldikNCit7DQorCXN0cnVjdCBoZWFkc2V0ICpo
cyA9IGRldi0+aGVhZHNldDsNCisNCisJcmV0dXJuIGhzLT5yZmNvbW1faW5pdGlhdG9yOw0KK30N
CisNCit2b2lkIHNldF9yZmNvbW1faW5pdGlhdG9yKHN0cnVjdCBhdWRpb19kZXZpY2UgKmRldiwg
Z2Jvb2xlYW4gaW5pdGlhdG9yKQ0KK3sNCisJc3RydWN0IGhlYWRzZXQgKmhzID0gZGV2LT5oZWFk
c2V0Ow0KKw0KKwlocy0+cmZjb21tX2luaXRpYXRvciA9IGluaXRpYXRvcjsNCit9DQorDQogR0lP
Q2hhbm5lbCAqaGVhZHNldF9nZXRfcmZjb21tKHN0cnVjdCBhdWRpb19kZXZpY2UgKmRldikNCiB7
DQogCXN0cnVjdCBoZWFkc2V0ICpocyA9IGRldi0+aGVhZHNldDsNCmRpZmYgLS1naXQgYS9hdWRp
by9oZWFkc2V0LmggYi9hdWRpby9oZWFkc2V0LmgNCmluZGV4IDdjZTg4YzguLmYyNzVlMDMgMTAw
NjQ0DQotLS0gYS9hdWRpby9oZWFkc2V0LmgNCisrKyBiL2F1ZGlvL2hlYWRzZXQuaA0KQEAgLTgy
LDYgKzgyLDkgQEAgZ2Jvb2xlYW4gaGVhZHNldF9jYW5jZWxfc3RyZWFtKHN0cnVjdCBhdWRpb19k
ZXZpY2UgKmRldiwgdW5zaWduZWQgaW50IGlkKTsNCiBnYm9vbGVhbiBnZXRfaGZwX2FjdGl2ZShz
dHJ1Y3QgYXVkaW9fZGV2aWNlICpkZXYpOw0KIHZvaWQgc2V0X2hmcF9hY3RpdmUoc3RydWN0IGF1
ZGlvX2RldmljZSAqZGV2LCBnYm9vbGVhbiBhY3RpdmUpOw0KIA0KK2dib29sZWFuIGdldF9yZmNv
bW1faW5pdGlhdG9yKHN0cnVjdCBhdWRpb19kZXZpY2UgKmRldik7DQordm9pZCBzZXRfcmZjb21t
X2luaXRpYXRvcihzdHJ1Y3QgYXVkaW9fZGV2aWNlICpkZXYsIGdib29sZWFuIGluaXRpYXRvcik7
DQorDQogdm9pZCBoZWFkc2V0X3NldF9hdXRob3JpemVkKHN0cnVjdCBhdWRpb19kZXZpY2UgKmRl
dik7DQogaW50IGhlYWRzZXRfY29ubmVjdF9yZmNvbW0oc3RydWN0IGF1ZGlvX2RldmljZSAqZGV2
LCBHSU9DaGFubmVsICpjaGFuKTsNCiBpbnQgaGVhZHNldF9jb25uZWN0X3NjbyhzdHJ1Y3QgYXVk
aW9fZGV2aWNlICpkZXYsIEdJT0NoYW5uZWwgKmlvKTsNCmRpZmYgLS1naXQgYS9hdWRpby9tYW5h
Z2VyLmMgYi9hdWRpby9tYW5hZ2VyLmMNCmluZGV4IDA1Y2E2NTQuLjQwMGExYWMgMTAwNjQ0DQot
LS0gYS9hdWRpby9tYW5hZ2VyLmMNCisrKyBiL2F1ZGlvL21hbmFnZXIuYw0KQEAgLTUwOCw2ICs1
MDgsNyBAQCBzdGF0aWMgdm9pZCBhZ19jb25maXJtKEdJT0NoYW5uZWwgKmNoYW4sIGdwb2ludGVy
IGRhdGEpDQogCX0NCiANCiAJc2V0X2hmcF9hY3RpdmUoZGV2aWNlLCBoZnBfYWN0aXZlKTsNCisJ
c2V0X3JmY29tbV9pbml0aWF0b3IoZGV2aWNlLCBUUlVFKTsNCiANCiAJaWYgKGhlYWRzZXRfY29u
bmVjdF9yZmNvbW0oZGV2aWNlLCBjaGFuKSA8IDApIHsNCiAJCWVycm9yKCJoZWFkc2V0X2Nvbm5l
Y3RfcmZjb21tIGZhaWxlZCIpOw0KLS0gDQoxLjcuNC4xDQoNCg==


2011-08-12 13:30:37

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Increase timeout before initiating AVDTP connection

Hi Peter,

On Fri, Aug 12, 2011 at 3:56 PM, Peter Hurley <[email protected]> wrote:
> AVDTP_CONNECT_TIMEOUT controls the delay between HSP/HFP
> connection establishment and AVDTP signal channel establishment.
> The original value of 1 sec. is too short to avoid racing for AVDTP
> connection establishment (eg., if the device continues to configure
> the HFP service level connection with add'l AT cmds).
>
> Some devices have broken AVDTP implementations that just cannot
> handle the race conditions that arise if both devices are attempting
> stream establishment at the same time. However, these conditions arise
> only when the remote device is the ACL initiator (and in practice, the
> RFCOMM initiator as well). ?This fix bumps out the timeout value only
> when the headset has initiated the link.
> ---
> ?audio/device.c ?| ? ?9 ++++++++-
> ?audio/headset.c | ? 16 ++++++++++++++++
> ?audio/headset.h | ? ?3 +++
> ?audio/manager.c | ? ?1 +
> ?4 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/audio/device.c b/audio/device.c
> index 18e873e..0bd1443 100644
> --- a/audio/device.c
> +++ b/audio/device.c
> @@ -61,6 +61,7 @@
>
> ?#define CONTROL_CONNECT_TIMEOUT 2
> ?#define AVDTP_CONNECT_TIMEOUT 1
> +#define AVDTP_CONNECT_TIMEOUT_BOOST 1
> ?#define HEADSET_CONNECT_TIMEOUT 1
>
> ?typedef enum {
> @@ -305,6 +306,7 @@ static gboolean avdtp_connect_timeout(gpointer user_data)
> ?static gboolean device_set_avdtp_timer(struct audio_device *dev)
> ?{
> ? ? ? ?struct dev_priv *priv = dev->priv;
> + ? ? ? guint timeout = AVDTP_CONNECT_TIMEOUT;
>
> ? ? ? ?if (!dev->sink)
> ? ? ? ? ? ? ? ?return FALSE;
> @@ -312,7 +314,12 @@ static gboolean device_set_avdtp_timer(struct audio_device *dev)
> ? ? ? ?if (priv->avdtp_timer)
> ? ? ? ? ? ? ? ?return FALSE;
>
> - ? ? ? priv->avdtp_timer = g_timeout_add_seconds(AVDTP_CONNECT_TIMEOUT,
> + ? ? ? /* if the headset is the HSP/HFP RFCOMM initiator, give the headset
> + ? ? ? ? ?time to initiate AVDTP signalling (and avoid further racing) */
> + ? ? ? if (dev->headset && get_rfcomm_initiator(dev))
> + ? ? ? ? ? ? ? timeout += AVDTP_CONNECT_TIMEOUT_BOOST;
> +
> + ? ? ? priv->avdtp_timer = g_timeout_add_seconds(timeout,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?avdtp_connect_timeout,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dev);
>
> diff --git a/audio/headset.c b/audio/headset.c
> index 8e63afc..66a28f2 100644
> --- a/audio/headset.c
> +++ b/audio/headset.c
> @@ -167,6 +167,7 @@ struct headset {
>
> ? ? ? ?gboolean hfp_active;
> ? ? ? ?gboolean search_hfp;
> + ? ? ? gboolean rfcomm_initiator;
>
> ? ? ? ?headset_state_t state;
> ? ? ? ?struct pending_connect *pending;
> @@ -1633,6 +1634,7 @@ static int rfcomm_connect(struct audio_device *dev, headset_stream_cb_t cb,
> ? ? ? ?}
>
> ? ? ? ?hs->hfp_active = hs->hfp_handle != 0 ? TRUE : FALSE;
> + ? ? ? hs->rfcomm_initiator = FALSE;
>
> ? ? ? ?headset_set_state(dev, HEADSET_STATE_CONNECTING);
>
> @@ -2442,6 +2444,20 @@ void set_hfp_active(struct audio_device *dev, gboolean active)
> ? ? ? ?hs->hfp_active = active;
> ?}
>
> +gboolean get_rfcomm_initiator(struct audio_device *dev)
> +{
> + ? ? ? struct headset *hs = dev->headset;
> +
> + ? ? ? return hs->rfcomm_initiator;
> +}
> +
> +void set_rfcomm_initiator(struct audio_device *dev, gboolean initiator)
> +{
> + ? ? ? struct headset *hs = dev->headset;
> +
> + ? ? ? hs->rfcomm_initiator = initiator;
> +}
> +

I would suggest you to follow the name convention e.g.
headset_get_rfcomm_initiator...

> ?GIOChannel *headset_get_rfcomm(struct audio_device *dev)
> ?{
> ? ? ? ?struct headset *hs = dev->headset;
> diff --git a/audio/headset.h b/audio/headset.h
> index 7ce88c8..f275e03 100644
> --- a/audio/headset.h
> +++ b/audio/headset.h
> @@ -82,6 +82,9 @@ gboolean headset_cancel_stream(struct audio_device *dev, unsigned int id);
> ?gboolean get_hfp_active(struct audio_device *dev);
> ?void set_hfp_active(struct audio_device *dev, gboolean active);
>
> +gboolean get_rfcomm_initiator(struct audio_device *dev);
> +void set_rfcomm_initiator(struct audio_device *dev, gboolean initiator);
> +

Here too.

--
Luiz Augusto von Dentz