[PATCH 1/3] android/health: Remove mcl instance on mcl_disconnected call back

[PATCH 1/3] android/health: Remove mcl instance on mcl_disconnected call back

[Ravi kumar Veeramally]

MCAP library is maintaining two sets of mcl list. On mcl_disconnected
call mcl instance is removed from regular mcl list and cached in mcl_cache
list. health.c doesn't maintain any cached mcls list. So mcl_conn variable
doesn't make any sense of not being connected on mcl_disconnected call back.
So unref mcl on mcl_disconnected and don't do anything in mcl_unached callback.
---
 android/health.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/android/health.c b/android/health.c
index 665482e..ee6c11b 100644
--- a/android/health.c
+++ b/android/health.c
@@ -85,7 +85,6 @@ struct health_device {
 	uint16_t app_id;
 
 	struct mcap_mcl *mcl;
-	bool mcl_conn;
 
 	struct queue *channels;     /* data channels */
 
@@ -193,7 +192,6 @@ static void unref_mcl(struct health_device *dev)
 	mcap_close_mcl(dev->mcl, FALSE);
 	mcap_mcl_unref(dev->mcl);
 	dev->mcl = NULL;
-	dev->mcl_conn = false;
 }
 
 static void free_health_device(void *data)
@@ -1826,7 +1824,6 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
 	if (!channel->dev->mcl)
 		channel->dev->mcl = mcap_mcl_ref(mcl);
 
-	channel->dev->mcl_conn = true;
 	info("health: MCL connected");
 
 	ret = set_mcl_cb(channel->dev->mcl, channel, &gerr);
@@ -1948,7 +1945,7 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 		}
 	}
 
-	if (!dev->mcl || (dev->mcl && !dev->mcl_conn)) {
+	if (!dev->mcl) {
 		if (connect_mcl(channel) < 0) {
 			error("health: error retrieving HDP SDP record");
 			goto fail;
@@ -2072,8 +2069,6 @@ static void mcl_reconnected(struct mcap_mcl *mcl, gpointer data)
 		error("device data does not exists");
 		return;
 	}
-
-	dev->mcl_conn = true;
 }
 
 static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
@@ -2084,18 +2079,12 @@ static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
 
 	info("health: MCL disconnected");
 	dev = search_dev_by_mcl(mcl);
-	if (dev)
-		dev->mcl_conn = false;
+	unref_mcl(dev);
 }
 
 static void mcl_uncached(struct mcap_mcl *mcl, gpointer data)
 {
-	struct health_device *dev;
-
-	DBG("");
-
-	dev = search_dev_by_mcl(mcl);
-	free_health_device(dev);
+	/* mcap library maintains cache of mcls, not required here */
 }
 
 bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
-- 
1.9.1

[PATCH 2/3] android/health: Call MDL shutdown after mdl_deleted_cb callback

[Ravi kumar Veeramally]

Shutdown_mdl shutdown io channels and unrefs it. After that related
callbacks will shutdown or close and free its data. Due to timing
issue process_md_delete_mdl_req send delete confirmation response to
peer before properly shutting down all opened data channels.
---
 android/health.c   | 25 ++++---------------------
 android/mcap-lib.c |  6 ++----
 2 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/android/health.c b/android/health.c
index ee6c11b..f49110a 100644
--- a/android/health.c
+++ b/android/health.c
@@ -1254,34 +1254,17 @@ static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
 	channel->mdl_conn = false;
 }
 
-static void notify_channel(void *data, void *user_data)
-{
-	struct health_channel *channel = data;
-
-	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
-}
-
 static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
 {
-	struct health_channel *channel = data;
-	struct health_device *dev;
-
-	if (!channel)
-		return;
-
-	dev = channel->dev;
+	struct health_channel *channel;
 
-	DBG("device %p channel %p mdl %p", dev, channel, mdl);
 	info("health: MDL deleted");
 
-	/* mdl == NULL means, delete all mdls */
-	if (!mdl) {
-		queue_foreach(dev->channels, notify_channel, NULL);
-		queue_remove_all(dev->channels, NULL, NULL,
-						free_health_channel);
+	channel = search_channel_by_mdl(mdl);
+	if (!channel)
 		return;
-	}
 
+	DBG("channel %p mdl %p", channel, mdl);
 	destroy_channel(channel);
 }
 
diff --git a/android/mcap-lib.c b/android/mcap-lib.c
index f208fac..1706d9f 100644
--- a/android/mcap-lib.c
+++ b/android/mcap-lib.c
@@ -1007,10 +1007,10 @@ static void mcap_del_mdl(gpointer elem, gpointer user_data)
 	struct mcap_mdl *mdl = elem;
 	gboolean notify = *(gboolean *) user_data;
 
-	shutdown_mdl(mdl);
 	if (notify)
 		mdl->mcl->cb->mdl_deleted(mdl, mdl->mcl->cb->user_data);
 
+	shutdown_mdl(mdl);
 	mcap_mdl_unref(mdl);
 }
 
@@ -1231,13 +1231,11 @@ static void process_md_delete_mdl_req(struct mcap_mcl *mcl, void *cmd,
 	req = cmd;
 	mdlid = ntohs(req->mdl);
 	if (mdlid == MCAP_ALL_MDLIDS) {
-		notify = FALSE;
+		notify = TRUE;
 		g_slist_foreach(mcl->mdls, mcap_del_mdl, &notify);
 		g_slist_free(mcl->mdls);
 		mcl->mdls = NULL;
 		mcl->state = MCL_CONNECTED;
-		/* NULL mdl means ALL_MDLS */
-		mcl->cb->mdl_deleted(NULL, mcl->cb->user_data);
 		goto resp;
 	}
 
-- 
1.9.1

[PATCH 3/3] android/health: Cache MDL file desciptor

[Ravi kumar Veeramally]

PTS expects to close all data channels before sending delete confirmation
to peer. But file descriptor passed over IPC to hal needs to be closed.
Due to timing issue IPC notification is triggering after sending
confirmation. So cache fd and shutdown on channel free will solve the issue.
---
 android/health.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/android/health.c b/android/health.c
index f49110a..29c1d15 100644
--- a/android/health.c
+++ b/android/health.c
@@ -102,6 +102,7 @@ struct health_channel {
 	struct mcap_mdl *mdl;
 	bool mdl_conn;
 	uint16_t mdl_id; /* MDL ID */
+	int fd;
 
 	uint16_t id; /* channel id */
 };
@@ -168,6 +169,9 @@ static void free_health_channel(void *data)
 	if (!channel)
 		return;
 
+	if (channel->fd >= 0)
+		shutdown(channel->fd, SHUT_RDWR);
+
 	unref_mdl(channel);
 	free(channel);
 }
@@ -1234,6 +1238,7 @@ static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
 	}
 
 	info("health: MDL connected");
+	channel->fd = fd;
 	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
 
 	return;
@@ -1357,6 +1362,7 @@ static struct health_channel *create_channel(struct health_app *app,
 	channel->type = mdep->channel_type;
 	channel->id = channel_id++;
 	channel->dev = dev;
+	channel->fd = -1;
 
 	if (!queue_push_tail(dev->channels, channel)) {
 		free_health_channel(channel);
@@ -1547,6 +1553,7 @@ static void connect_mdl_cb(struct mcap_mdl *mdl, GError *gerr, gpointer data)
 		if (channel->type != CHANNEL_TYPE_RELIABLE)
 			goto fail;
 
+	channel->fd = fd;
 	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
 
 	return;
-- 
1.9.1

Re: [PATCH 1/3] android/health: Remove mcl instance on mcl_disconnected call back

[Szymon Janc]

Hi Ravi,

On Friday 22 of August 2014 12:10:05 Ravi kumar Veeramally wrote:
> MCAP library is maintaining two sets of mcl list. On mcl_disconnected
> call mcl instance is removed from regular mcl list and cached in mcl_cache
> list. health.c doesn't maintain any cached mcls list. So mcl_conn variable
> doesn't make any sense of not being connected on mcl_disconnected call back.
> So unref mcl on mcl_disconnected and don't do anything in mcl_unached callback.
> ---
>  android/health.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index 665482e..ee6c11b 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -85,7 +85,6 @@ struct health_device {
>  	uint16_t app_id;
>  
>  	struct mcap_mcl *mcl;
> -	bool mcl_conn;
>  
>  	struct queue *channels;     /* data channels */
>  
> @@ -193,7 +192,6 @@ static void unref_mcl(struct health_device *dev)
>  	mcap_close_mcl(dev->mcl, FALSE);
>  	mcap_mcl_unref(dev->mcl);
>  	dev->mcl = NULL;
> -	dev->mcl_conn = false;
>  }
>  
>  static void free_health_device(void *data)
> @@ -1826,7 +1824,6 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
>  	if (!channel->dev->mcl)
>  		channel->dev->mcl = mcap_mcl_ref(mcl);
>  
> -	channel->dev->mcl_conn = true;
>  	info("health: MCL connected");
>  
>  	ret = set_mcl_cb(channel->dev->mcl, channel, &gerr);
> @@ -1948,7 +1945,7 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
>  		}
>  	}
>  
> -	if (!dev->mcl || (dev->mcl && !dev->mcl_conn)) {
> +	if (!dev->mcl) {
>  		if (connect_mcl(channel) < 0) {
>  			error("health: error retrieving HDP SDP record");
>  			goto fail;
> @@ -2072,8 +2069,6 @@ static void mcl_reconnected(struct mcap_mcl *mcl, gpointer data)
>  		error("device data does not exists");
>  		return;
>  	}
> -
> -	dev->mcl_conn = true;
>  }
>  
>  static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
> @@ -2084,18 +2079,12 @@ static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
>  
>  	info("health: MCL disconnected");
>  	dev = search_dev_by_mcl(mcl);
> -	if (dev)
> -		dev->mcl_conn = false;
> +	unref_mcl(dev);
>  }
>  
>  static void mcl_uncached(struct mcap_mcl *mcl, gpointer data)
>  {
> -	struct health_device *dev;
> -
> -	DBG("");
> -
> -	dev = search_dev_by_mcl(mcl);
> -	free_health_device(dev);
> +	/* mcap library maintains cache of mcls, not required here */
>  }
>  
>  bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
> 

This patch is now applied, thanks.

I had to fix commit message line length limit, so please keep it 72 characters
in future.

-- 
Best regards, 
Szymon Janc

Re: [PATCH 2/3] android/health: Call MDL shutdown after mdl_deleted_cb callback

[Szymon Janc]

Hi Ravi,

On Friday 22 of August 2014 12:10:06 Ravi kumar Veeramally wrote:
> Shutdown_mdl shutdown io channels and unrefs it. After that related
> callbacks will shutdown or close and free its data. Due to timing
> issue process_md_delete_mdl_req send delete confirmation response to
> peer before properly shutting down all opened data channels.
> ---
>  android/health.c   | 25 ++++---------------------
>  android/mcap-lib.c |  6 ++----
>  2 files changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index ee6c11b..f49110a 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -1254,34 +1254,17 @@ static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
>  	channel->mdl_conn = false;
>  }
>  
> -static void notify_channel(void *data, void *user_data)
> -{
> -	struct health_channel *channel = data;
> -
> -	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
> -}
> -
>  static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
>  {
> -	struct health_channel *channel = data;
> -	struct health_device *dev;
> -
> -	if (!channel)
> -		return;
> -
> -	dev = channel->dev;
> +	struct health_channel *channel;
>  
> -	DBG("device %p channel %p mdl %p", dev, channel, mdl);
>  	info("health: MDL deleted");
>  
> -	/* mdl == NULL means, delete all mdls */
> -	if (!mdl) {
> -		queue_foreach(dev->channels, notify_channel, NULL);
> -		queue_remove_all(dev->channels, NULL, NULL,
> -						free_health_channel);
> +	channel = search_channel_by_mdl(mdl);
> +	if (!channel)
>  		return;
> -	}
>  
> +	DBG("channel %p mdl %p", channel, mdl);
>  	destroy_channel(channel);
>  }
>  
> diff --git a/android/mcap-lib.c b/android/mcap-lib.c
> index f208fac..1706d9f 100644
> --- a/android/mcap-lib.c
> +++ b/android/mcap-lib.c
> @@ -1007,10 +1007,10 @@ static void mcap_del_mdl(gpointer elem, gpointer user_data)
>  	struct mcap_mdl *mdl = elem;
>  	gboolean notify = *(gboolean *) user_data;
>  
> -	shutdown_mdl(mdl);
>  	if (notify)
>  		mdl->mcl->cb->mdl_deleted(mdl, mdl->mcl->cb->user_data);
>  
> +	shutdown_mdl(mdl);
>  	mcap_mdl_unref(mdl);
>  }
>  
> @@ -1231,13 +1231,11 @@ static void process_md_delete_mdl_req(struct mcap_mcl *mcl, void *cmd,
>  	req = cmd;
>  	mdlid = ntohs(req->mdl);
>  	if (mdlid == MCAP_ALL_MDLIDS) {
> -		notify = FALSE;
> +		notify = TRUE;
>  		g_slist_foreach(mcl->mdls, mcap_del_mdl, &notify);
>  		g_slist_free(mcl->mdls);
>  		mcl->mdls = NULL;
>  		mcl->state = MCL_CONNECTED;
> -		/* NULL mdl means ALL_MDLS */
> -		mcl->cb->mdl_deleted(NULL, mcl->cb->user_data);
>  		goto resp;
>  	}
>  
> 

Applied. Thanks.

-- 
Best regards, 
Szymon Janc

Re: [PATCH 3/3] android/health: Cache MDL file desciptor

[Szymon Janc]

Hi Ravi,

On Friday 22 of August 2014 12:10:07 Ravi kumar Veeramally wrote:
> PTS expects to close all data channels before sending delete confirmation
> to peer. But file descriptor passed over IPC to hal needs to be closed.
> Due to timing issue IPC notification is triggering after sending
> confirmation. So cache fd and shutdown on channel free will solve the issue.
> ---
>  android/health.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/android/health.c b/android/health.c
> index f49110a..29c1d15 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -102,6 +102,7 @@ struct health_channel {
>  	struct mcap_mdl *mdl;
>  	bool mdl_conn;
>  	uint16_t mdl_id; /* MDL ID */
> +	int fd;
>  
>  	uint16_t id; /* channel id */
>  };
> @@ -168,6 +169,9 @@ static void free_health_channel(void *data)
>  	if (!channel)
>  		return;
>  
> +	if (channel->fd >= 0)
> +		shutdown(channel->fd, SHUT_RDWR);

Shouldn't channel->fd be closed here as well? (I think in what we have in
git now we also miss closing after sending fd over IPC, but that would not
matter if we cache fd with this patch.)

Also I think fd should be closed for echo scenario ie by setting
close_on_unref flag in glib io?

Thoughts?

> +
>  	unref_mdl(channel);
>  	free(channel);
>  }
> @@ -1234,6 +1238,7 @@ static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
>  	}
>  
>  	info("health: MDL connected");
> +	channel->fd = fd;
>  	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
>  
>  	return;
> @@ -1357,6 +1362,7 @@ static struct health_channel *create_channel(struct health_app *app,
>  	channel->type = mdep->channel_type;
>  	channel->id = channel_id++;
>  	channel->dev = dev;
> +	channel->fd = -1;
>  
>  	if (!queue_push_tail(dev->channels, channel)) {
>  		free_health_channel(channel);
> @@ -1547,6 +1553,7 @@ static void connect_mdl_cb(struct mcap_mdl *mdl, GError *gerr, gpointer data)
>  		if (channel->type != CHANNEL_TYPE_RELIABLE)
>  			goto fail;
>  
> +	channel->fd = fd;
>  	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
>  
>  	return;
> 

-- 
Best regards, 
Szymon Janc