2011-08-21 15:54:54

by Mohammed Shafi Shajakhan

[permalink] [raw]
Subject: [PATCH] ath9k_htc: Fix few possible memory leaks

From: Mohammed Shafi Shajakhan <[email protected]>

still there are few other memory leaks which will be fixed
very soon

Cc: Rajkumar Manoharan <[email protected]>
Signed-off-by: Mohammed Shafi Shajakhan <[email protected]>
Signed-off-by: Larry Finger <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc_hst.c | 13 ++++++++-----
drivers/net/wireless/ath/ath9k/wmi.c | 3 ++-
2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 1b90ed8..e435c9b 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -169,13 +169,14 @@ static int htc_config_pipe_credits(struct htc_target *target)
time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
if (!time_left) {
dev_err(target->dev, "HTC credit config timeout\n");
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto err;
}

return 0;
err:
kfree_skb(skb);
- return -EINVAL;
+ return ret;
}

static int htc_setup_complete(struct htc_target *target)
@@ -204,14 +205,15 @@ static int htc_setup_complete(struct htc_target *target)
time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
if (!time_left) {
dev_err(target->dev, "HTC start timeout\n");
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto err;
}

return 0;

err:
kfree_skb(skb);
- return -EINVAL;
+ return ret;
}

/* HTC APIs */
@@ -276,7 +278,8 @@ int htc_connect_service(struct htc_target *target,
if (!time_left) {
dev_err(target->dev, "Service connection timeout for: %d\n",
service_connreq->service_id);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto err;
}

*conn_rsp_epid = target->conn_rsp_epid;
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 35422fc..50d901d 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -334,7 +334,8 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
"Timeout waiting for WMI command: %s\n",
wmi_cmd_to_name(cmd_id));
mutex_unlock(&wmi->op_mutex);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto out;
}

mutex_unlock(&wmi->op_mutex);
--
1.7.0.4



Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

On Sun, Aug 21, 2011 at 09:24:45PM +0530, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <[email protected]>
>
> still there are few other memory leaks which will be fixed
> very soon
>
> Cc: Rajkumar Manoharan <[email protected]>
> Signed-off-by: Mohammed Shafi Shajakhan <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/htc_hst.c | 13 ++++++++-----
> drivers/net/wireless/ath/ath9k/wmi.c | 3 ++-
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
> index 1b90ed8..e435c9b 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
> @@ -169,13 +169,14 @@ static int htc_config_pipe_credits(struct htc_target *target)
> time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
> if (!time_left) {
> dev_err(target->dev, "HTC credit config timeout\n");
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto err;

Are you sure we have to free the skb upon timeout?. It looks like
it is already taken care in ath9k_htc_txcompletion_cb(). The same
may apply to your other changes as well.

Vasanth

2011-08-22 16:48:47

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

On 08/22/2011 08:56 AM, mohammed wrote:
>
>
> Vasanth thanks for your review, I haven't and not aware of that code
> flow/callback. I will check into that.
>

This patch has helped a lot, but after about 12 hours of running, I still have
18 leaks found. I will try to sort them out.

Larry

2011-08-24 18:04:17

by Mohammed Shafi

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

On Wed, Aug 24, 2011 at 11:18 PM, John W. Linville
<[email protected]> wrote:
> Is this intended for 3.1? ?Should it be Cc: [email protected] as well?

Hi John,

sorry for the inconvenience, please drop this patch
as discussed in the thread
http://comments.gmane.org/gmane.linux.kernel.wireless.general/75206
thanks!

>
> On Sun, Aug 21, 2011 at 09:24:45PM +0530, Mohammed Shafi Shajakhan wrote:
>> From: Mohammed Shafi Shajakhan <[email protected]>
>>
>> still there are few other memory leaks which will be fixed
>> very soon
>>
>> Cc: Rajkumar Manoharan <[email protected]>
>> Signed-off-by: Mohammed Shafi Shajakhan <[email protected]>
>> Signed-off-by: Larry Finger <[email protected]>
>> ---
>> ?drivers/net/wireless/ath/ath9k/htc_hst.c | ? 13 ++++++++-----
>> ?drivers/net/wireless/ath/ath9k/wmi.c ? ? | ? ?3 ++-
>> ?2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
>> index 1b90ed8..e435c9b 100644
>> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
>> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
>> @@ -169,13 +169,14 @@ static int htc_config_pipe_credits(struct htc_target *target)
>> ? ? ? time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
>> ? ? ? if (!time_left) {
>> ? ? ? ? ? ? ? dev_err(target->dev, "HTC credit config timeout\n");
>> - ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? ret = -ETIMEDOUT;
>> + ? ? ? ? ? ? goto err;
>> ? ? ? }
>>
>> ? ? ? return 0;
>> ?err:
>> ? ? ? kfree_skb(skb);
>> - ? ? return -EINVAL;
>> + ? ? return ret;
>> ?}
>>
>> ?static int htc_setup_complete(struct htc_target *target)
>> @@ -204,14 +205,15 @@ static int htc_setup_complete(struct htc_target *target)
>> ? ? ? time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
>> ? ? ? if (!time_left) {
>> ? ? ? ? ? ? ? dev_err(target->dev, "HTC start timeout\n");
>> - ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? ret = -ETIMEDOUT;
>> + ? ? ? ? ? ? goto err;
>> ? ? ? }
>>
>> ? ? ? return 0;
>>
>> ?err:
>> ? ? ? kfree_skb(skb);
>> - ? ? return -EINVAL;
>> + ? ? return ret;
>> ?}
>>
>> ?/* HTC APIs */
>> @@ -276,7 +278,8 @@ int htc_connect_service(struct htc_target *target,
>> ? ? ? if (!time_left) {
>> ? ? ? ? ? ? ? dev_err(target->dev, "Service connection timeout for: %d\n",
>> ? ? ? ? ? ? ? ? ? ? ? service_connreq->service_id);
>> - ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? ret = -ETIMEDOUT;
>> + ? ? ? ? ? ? goto err;
>> ? ? ? }
>>
>> ? ? ? *conn_rsp_epid = target->conn_rsp_epid;
>> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
>> index 35422fc..50d901d 100644
>> --- a/drivers/net/wireless/ath/ath9k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
>> @@ -334,7 +334,8 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
>> ? ? ? ? ? ? ? ? ? ? ? "Timeout waiting for WMI command: %s\n",
>> ? ? ? ? ? ? ? ? ? ? ? wmi_cmd_to_name(cmd_id));
>> ? ? ? ? ? ? ? mutex_unlock(&wmi->op_mutex);
>> - ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? ret = -ETIMEDOUT;
>> + ? ? ? ? ? ? goto out;
>> ? ? ? }
>>
>> ? ? ? mutex_unlock(&wmi->op_mutex);
>> --
>> 1.7.0.4
>>
>>
>
> --
> John W. Linville ? ? ? ? ? ? ? ?Someday the world will need a hero, and you
> [email protected] ? ? ? ? ? ? ? ? ?might be all we have. ?Be ready.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>



--
shafi

2011-08-23 05:05:10

by Mohammed Shafi Shajakhan

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

On Monday 22 August 2011 10:18 PM, Larry Finger wrote:
> On 08/22/2011 08:56 AM, mohammed wrote:
>>
>>
>> Vasanth thanks for your review, I haven't and not aware of that code
>> flow/callback. I will check into that.
>>
>
> This patch has helped a lot, but after about 12 hours of running, I
> still have 18 leaks found. I will try to sort them out.

Hi Larry,

thanks for your help, we will also try to find a suitable fix for this.

thanks,
shafi


2011-08-22 13:56:18

by Mohammed Shafi Shajakhan

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

On Monday 22 August 2011 10:59 AM, Vasanthakumar Thiagarajan wrote:
> On Sun, Aug 21, 2011 at 09:24:45PM +0530, Mohammed Shafi Shajakhan wrote:
>> From: Mohammed Shafi Shajakhan<[email protected]>
>>
>> still there are few other memory leaks which will be fixed
>> very soon
>>
>> Cc: Rajkumar Manoharan<[email protected]>
>> Signed-off-by: Mohammed Shafi Shajakhan<[email protected]>
>> Signed-off-by: Larry Finger<[email protected]>
>> ---
>> drivers/net/wireless/ath/ath9k/htc_hst.c | 13 ++++++++-----
>> drivers/net/wireless/ath/ath9k/wmi.c | 3 ++-
>> 2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
>> index 1b90ed8..e435c9b 100644
>> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
>> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
>> @@ -169,13 +169,14 @@ static int htc_config_pipe_credits(struct htc_target *target)
>> time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
>> if (!time_left) {
>> dev_err(target->dev, "HTC credit config timeout\n");
>> - return -ETIMEDOUT;
>> + ret = -ETIMEDOUT;
>> + goto err;
>
> Are you sure we have to free the skb upon timeout?. It looks like
> it is already taken care in ath9k_htc_txcompletion_cb(). The same
> may apply to your other changes as well.


Vasanth thanks for your review, I haven't and not aware of that code
flow/callback. I will check into that.

thanks,
shafi

>
> Vasanth


2011-08-24 18:00:23

by John W. Linville

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: Fix few possible memory leaks

Is this intended for 3.1? Should it be Cc: [email protected] as well?

On Sun, Aug 21, 2011 at 09:24:45PM +0530, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <[email protected]>
>
> still there are few other memory leaks which will be fixed
> very soon
>
> Cc: Rajkumar Manoharan <[email protected]>
> Signed-off-by: Mohammed Shafi Shajakhan <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/htc_hst.c | 13 ++++++++-----
> drivers/net/wireless/ath/ath9k/wmi.c | 3 ++-
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
> index 1b90ed8..e435c9b 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
> @@ -169,13 +169,14 @@ static int htc_config_pipe_credits(struct htc_target *target)
> time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
> if (!time_left) {
> dev_err(target->dev, "HTC credit config timeout\n");
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto err;
> }
>
> return 0;
> err:
> kfree_skb(skb);
> - return -EINVAL;
> + return ret;
> }
>
> static int htc_setup_complete(struct htc_target *target)
> @@ -204,14 +205,15 @@ static int htc_setup_complete(struct htc_target *target)
> time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
> if (!time_left) {
> dev_err(target->dev, "HTC start timeout\n");
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto err;
> }
>
> return 0;
>
> err:
> kfree_skb(skb);
> - return -EINVAL;
> + return ret;
> }
>
> /* HTC APIs */
> @@ -276,7 +278,8 @@ int htc_connect_service(struct htc_target *target,
> if (!time_left) {
> dev_err(target->dev, "Service connection timeout for: %d\n",
> service_connreq->service_id);
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto err;
> }
>
> *conn_rsp_epid = target->conn_rsp_epid;
> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
> index 35422fc..50d901d 100644
> --- a/drivers/net/wireless/ath/ath9k/wmi.c
> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
> @@ -334,7 +334,8 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
> "Timeout waiting for WMI command: %s\n",
> wmi_cmd_to_name(cmd_id));
> mutex_unlock(&wmi->op_mutex);
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto out;
> }
>
> mutex_unlock(&wmi->op_mutex);
> --
> 1.7.0.4
>
>

--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.