When device is removed and paired again, hdp_device is destroyed
but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
This patch ensures that such MCL is destroyed.
There is probably a better solution to this e.g. changing cache
strategy for accepted connections. A loop can be removed if 1:1
relationship between hdp_device and MCL is guaranteed.
---
health/hdp.c | 1 +
health/mcap.c | 23 ++++++++++++++++++++++-
health/mcap_lib.h | 2 ++
3 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index b141fe7..44ebe75 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
}
device_unref_mcl(device);
+ mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
g_free(device);
}
diff --git a/health/mcap.c b/health/mcap.c
index cb7b74c..34ccdaa 100644
--- a/health/mcap.c
+++ b/health/mcap.c
@@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
return TRUE;
}
+static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
+{
+ const struct mcap_mcl *mcl = a;
+ gconstpointer user_data = b;
+
+ if (mcl->cb)
+ if (mcl->cb->user_data == user_data)
+ return 0;
+
+ return 1;
+}
+
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
+{
+ GSList *l;
+
+ while ((l = g_slist_find_custom(mi->cached, user_data,
+ cmp_mcl_user_data)))
+ mcap_close_mcl(l->data, FALSE);
+}
+
void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
{
bacpy(addr, &mcl->addr);
@@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
BT_IO_OPT_INVALID);
-}
\ No newline at end of file
+}
diff --git a/health/mcap_lib.h b/health/mcap_lib.h
index 7740623..cc10c17 100644
--- a/health/mcap_lib.h
+++ b/health/mcap_lib.h
@@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
void mcap_mcl_unref(struct mcap_mcl *mcl);
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
+
/* CSP operations */
void mcap_enable_csp(struct mcap_instance *ms);
--
1.7.0.4
Hi,
On Wed, Nov 10, 2010, Jose Antonio Santos Cadenas wrote:
> ---
> health/hdp.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index 1eba8e1..d361b27 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
> if (!hdp_device->mcl)
> return;
>
> + mcap_close_mcl(hdp_device->mcl, FALSE);
> mcap_mcl_unref(hdp_device->mcl);
> hdp_device->mcl = NULL;
> hdp_device->mcl_conn = FALSE;
Pushed upstream. Thanks.
Johan
Hi Elvis,
2010/11/10 Elvis Pf?tzenreuter <[email protected]>:
>> Hi Elvis,
>>
>> 2010/11/10 Elvis Pf?tzenreuter <[email protected]>:
>>>>> ---
>>>>> ?health/hdp.c | ? ?1 +
>>>>> ?1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/health/hdp.c b/health/hdp.c
>>>>> index 1eba8e1..d361b27 100644
>>>>> --- a/health/hdp.c
>>>>> +++ b/health/hdp.c
>>>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>>>> ? ? ? ?if (!hdp_device->mcl)
>>>>> ? ? ? ? ? ? ? ?return;
>>>>>
>>>>> + ? ? ? mcap_close_mcl(hdp_device->mcl, FALSE);
>>>>> ? ? ? ?mcap_mcl_unref(hdp_device->mcl);
>>>>> ? ? ? ?hdp_device->mcl = NULL;
>>>>> ? ? ? ?hdp_device->mcl_conn = FALSE;
>>>>> --
>>>>> 1.7.1
>>>>>
>>>>>
>>>>
>>>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>>>
>>> Yes, it seems to have fixed the problem. And far cleaner :)
>>>
>>> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
>>
>> This function doesn't disable the caching in general, it just closes
>> this MCL without catching it, but caching is still active for all the
>> other MCL's. Additionally, nothing happens if mcap_close_mcl is called
>> more than once, so this patch takes care of this too.
>
> Indeed, MCL disconnection and hdp_device destruction are different moments. Still fulfilling the caffeine quota for this morning :P
Of course they are, but when a device is removed, is completely normal
that you (HDP) would like to delete the MCAP cache because you (HDP)
are forgetting this device so you won't have any structures for the
data channels nor other structs in HDP needed to manage the
reconnections of the data channels that could be cached. The way to
uncache an mcl in MCAP is to close it indicating that you don't want
to cache it. This is just what this patch to.
>
>>>
>>> So, perhaps it would be better to get rid of caching code in mcap.c?
>>
>> I can't understand this, can you explain this more?. As I see, MCAP
>> should do the catching because it holds all the information about the
>> channels that are connected and can cache it, HDP could not do this
>> without MCAP. More over, if in the future more profiles use MCAP they
>> may want to cache too.
>
> I do have the opinion that caching in mcap.c is more complicated than it could be.
My opinion is that caching in MCAP is the correct way because MCAP is
dealing with sending and receiving commands and need to retain certain
structures that are necessary for it, this structures do not concern
to HDP, only concerns to MCAP and should be MCAP who stores (caches)
them.
> I'd have used a single list with a CLOSED flag. Also, there is some confusion in nomenclature (mcap_uncache_mcl() recovers from the cache, while mcl_uncached_cb callback means that MCL has been invalidated). This is source of bugs like this one.
This is a different issue. Probably the nomenclature is not as clear
as it should be but I think that this is not the point here,
nevertheless we can change it too.
>
> At the very least, cached MCLs should have mcl->cb zeroed. if MCAP level caches MCLs, very well, but HDP level should be asked to set callbacks and user data again, as if it were a new MCL. (Actually, mcl_reconnected in hdp.c seems to do that; not sure why it did not avoid the particular bug that is under discussion.)
It is supposed that if HDP (or any other profile using MCAP) wants to
use cache, it should keep its variables all the time that it is
interested in caching them. If the profile using MCAP don't want to
keep its state should tell MCAP that it is not interested in caching
(as the patch in this thread does) because it is deleting its own
state.
Regards.
Jose
> Hi Elvis,
>
> 2010/11/10 Elvis Pf?tzenreuter <[email protected]>:
>>>> ---
>>>> health/hdp.c | 1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/health/hdp.c b/health/hdp.c
>>>> index 1eba8e1..d361b27 100644
>>>> --- a/health/hdp.c
>>>> +++ b/health/hdp.c
>>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>>> if (!hdp_device->mcl)
>>>> return;
>>>>
>>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>>> mcap_mcl_unref(hdp_device->mcl);
>>>> hdp_device->mcl = NULL;
>>>> hdp_device->mcl_conn = FALSE;
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>>
>> Yes, it seems to have fixed the problem. And far cleaner :)
>>
>> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
>
> This function doesn't disable the caching in general, it just closes
> this MCL without catching it, but caching is still active for all the
> other MCL's. Additionally, nothing happens if mcap_close_mcl is called
> more than once, so this patch takes care of this too.
Indeed, MCL disconnection and hdp_device destruction are different moments. Still fulfilling the caffeine quota for this morning :P
>>
>> So, perhaps it would be better to get rid of caching code in mcap.c?
>
> I can't understand this, can you explain this more?. As I see, MCAP
> should do the catching because it holds all the information about the
> channels that are connected and can cache it, HDP could not do this
> without MCAP. More over, if in the future more profiles use MCAP they
> may want to cache too.
I do have the opinion that caching in mcap.c is more complicated than it could be. I'd have used a single list with a CLOSED flag. Also, there is some confusion in nomenclature (mcap_uncache_mcl() recovers from the cache, while mcl_uncached_cb callback means that MCL has been invalidated). This is source of bugs like this one.
At the very least, cached MCLs should have mcl->cb zeroed. if MCAP level caches MCLs, very well, but HDP level should be asked to set callbacks and user data again, as if it were a new MCL. (Actually, mcl_reconnected in hdp.c seems to do that; not sure why it did not avoid the particular bug that is under discussion.)
Hi Elvis,
2010/11/10 Elvis Pf?tzenreuter <[email protected]>:
>>> ---
>>> ?health/hdp.c | ? ?1 +
>>> ?1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/health/hdp.c b/health/hdp.c
>>> index 1eba8e1..d361b27 100644
>>> --- a/health/hdp.c
>>> +++ b/health/hdp.c
>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>> ? ? ? ?if (!hdp_device->mcl)
>>> ? ? ? ? ? ? ? ?return;
>>>
>>> + ? ? ? mcap_close_mcl(hdp_device->mcl, FALSE);
>>> ? ? ? ?mcap_mcl_unref(hdp_device->mcl);
>>> ? ? ? ?hdp_device->mcl = NULL;
>>> ? ? ? ?hdp_device->mcl_conn = FALSE;
>>> --
>>> 1.7.1
>>>
>>>
>>
>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>
> Yes, it seems to have fixed the problem. And far cleaner :)
>
> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
This function doesn't disable the caching in general, it just closes
this MCL without catching it, but caching is still active for all the
other MCL's. Additionally, nothing happens if mcap_close_mcl is called
more than once, so this patch takes care of this too.
>
> The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
This is because HDP wants to cache as long as possible.
>
> So, perhaps it would be better to get rid of caching code in mcap.c?
I can't understand this, can you explain this more?. As I see, MCAP
should do the catching because it holds all the information about the
channels that are connected and can cache it, HDP could not do this
without MCAP. More over, if in the future more profiles use MCAP they
may want to cache too.
Regards
>> ---
>> health/hdp.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/health/hdp.c b/health/hdp.c
>> index 1eba8e1..d361b27 100644
>> --- a/health/hdp.c
>> +++ b/health/hdp.c
>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>> if (!hdp_device->mcl)
>> return;
>>
>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>> mcap_mcl_unref(hdp_device->mcl);
>> hdp_device->mcl = NULL;
>> hdp_device->mcl_conn = FALSE;
>> --
>> 1.7.1
>>
>>
>
> Please Elvis, try this solution and tell us if it fix the segfault problem.
Yes, it seems to have fixed the problem. And far cleaner :)
I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
So, perhaps it would be better to get rid of caching code in mcap.c?
2010/11/10 Jose Antonio Santos Cadenas <[email protected]>:
> ---
> ?health/hdp.c | ? ?1 +
> ?1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index 1eba8e1..d361b27 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
> ? ? ? ?if (!hdp_device->mcl)
> ? ? ? ? ? ? ? ?return;
>
> + ? ? ? mcap_close_mcl(hdp_device->mcl, FALSE);
> ? ? ? ?mcap_mcl_unref(hdp_device->mcl);
> ? ? ? ?hdp_device->mcl = NULL;
> ? ? ? ?hdp_device->mcl_conn = FALSE;
> --
> 1.7.1
>
>
Please Elvis, try this solution and tell us if it fix the segfault problem.
Regards
---
health/hdp.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index 1eba8e1..d361b27 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
if (!hdp_device->mcl)
return;
+ mcap_close_mcl(hdp_device->mcl, FALSE);
mcap_mcl_unref(hdp_device->mcl);
hdp_device->mcl = NULL;
hdp_device->mcl_conn = FALSE;
--
1.7.1
Hi Elvis,
2010/11/10 Elvis Pf?tzenreuter <[email protected]>:
> When device is removed and paired again, hdp_device is destroyed
> but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
> This patch ensures that such MCL is destroyed.
>
> There is probably a better solution to this e.g. changing cache
> strategy for accepted connections. A loop can be removed if 1:1
> relationship between hdp_device and MCL is guaranteed.
> ---
> ?health/hdp.c ? ? ?| ? ?1 +
> ?health/mcap.c ? ? | ? 23 ++++++++++++++++++++++-
> ?health/mcap_lib.h | ? ?2 ++
> ?3 files changed, 25 insertions(+), 1 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index b141fe7..44ebe75 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
> ? ? ? ?}
>
> ? ? ? ?device_unref_mcl(device);
> + ? ? ? mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
>
> ? ? ? ?g_free(device);
> ?}
> diff --git a/health/mcap.c b/health/mcap.c
> index cb7b74c..34ccdaa 100644
> --- a/health/mcap.c
> +++ b/health/mcap.c
> @@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
> ? ? ? ?return TRUE;
> ?}
>
> +static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
> +{
> + ? ? ? const struct mcap_mcl *mcl = a;
> + ? ? ? gconstpointer user_data = b;
> +
> + ? ? ? if (mcl->cb)
> + ? ? ? ? ? ? ? if (mcl->cb->user_data == user_data)
> + ? ? ? ? ? ? ? ? ? ? ? return 0;
> +
> + ? ? ? return 1;
> +}
> +
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
> +{
> + ? ? ? GSList *l;
> +
> + ? ? ? while ((l = g_slist_find_custom(mi->cached, user_data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmp_mcl_user_data)))
> + ? ? ? ? ? ? ? mcap_close_mcl(l->data, FALSE);
> +}
> +
> ?void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
> ?{
> ? ? ? ?bacpy(addr, &mcl->addr);
> @@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
>
> ? ? ? ?return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?BT_IO_OPT_INVALID);
> -}
> \ No newline at end of file
> +}
> diff --git a/health/mcap_lib.h b/health/mcap_lib.h
> index 7740623..cc10c17 100644
> --- a/health/mcap_lib.h
> +++ b/health/mcap_lib.h
> @@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
> ?struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
> ?void mcap_mcl_unref(struct mcap_mcl *mcl);
>
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
> +
> ?/* CSP operations */
>
> ?void mcap_enable_csp(struct mcap_instance *ms);
I'm not sure if this is the best way to face this issue. It seems that
this solution is a workaround to avoid the real problem. Let me have a
look and search for a better solution.
Regards.