diff mbox

ath10k: Fix return value for btcoex and peer stats debugfs

Message ID 1459870106-2694-1-git-send-email-mohammed@qca.qualcomm.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Mohammed Shafi Shajakhan April 5, 2016, 3:28 p.m. UTC
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>

Return value is incorrect for btcoex and peer stats debugfs
'write' entries if the user provides a value that matches with
the already available debugfs entry, this results in the debugfs
entry getting stuck and the operation has to be terminated manually.
Fix this by returning the appropriate return 'count' as we do it for
other debugfs entries like pktlog etc

Fixes: cc61a1bbbc0e ("ath10k: enable debugfs provision to enable
Peer Stats feature")
Fixes: c28e6f06ff40 ("ath10k: fix sanity check on enabling btcoex via
debugfs")
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/debug.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Joe Perches April 5, 2016, 5:28 p.m. UTC | #1
On Tue, 2016-04-05 at 20:58 +0530, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
> 
> Return value is incorrect for btcoex and peer stats debugfs
> 'write' entries if the user provides a value that matches with
> the already available debugfs entry, this results in the debugfs
> entry getting stuck and the operation has to be terminated manually.
> Fix this by returning the appropriate return 'count' as we do it for
> other debugfs entries like pktlog etc

Not your code, but some of the xor uses are odd at best.

> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
[]
> @@ -2122,7 +2122,7 @@ static ssize_t ath10k_write_btcoex(struct file *file,
>  	struct ath10k *ar = file->private_data;
>  	char buf[32];
>  	size_t buf_size;
> -	int ret = 0;
> +	int ret;
>  	bool val;
>  
>  	buf_size = min(count, (sizeof(buf) - 1));
> @@ -2142,8 +2142,10 @@ static ssize_t ath10k_write_btcoex(struct file *file,
>  		goto exit;
>  	}
>  
> -	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val))
> +	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val)) {

xor on an int and a bool.

> @@ -2189,7 +2191,7 @@ static ssize_t ath10k_write_peer_stats(struct file *file,
>  	struct ath10k *ar = file->private_data;
>  	char buf[32];
>  	size_t buf_size;
> -	int ret = 0;
> +	int ret;
>  	bool val;
>  
>  	buf_size = min(count, (sizeof(buf) - 1));
> @@ -2209,8 +2211,10 @@ static ssize_t ath10k_write_peer_stats(struct file *file,
>  		goto exit;
>  	}
>  
> -	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val))
> +	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val)) {

here too

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mohammed Shafi Shajakhan April 6, 2016, 10:11 a.m. UTC | #2
Hello Joe,

On Tue, Apr 05, 2016 at 10:28:56AM -0700, Joe Perches wrote:
> On Tue, 2016-04-05 at 20:58 +0530, Mohammed Shafi Shajakhan wrote:
> > From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
> > 
> > Return value is incorrect for btcoex and peer stats debugfs
> > 'write' entries if the user provides a value that matches with
> > the already available debugfs entry, this results in the debugfs
> > entry getting stuck and the operation has to be terminated manually.
> > Fix this by returning the appropriate return 'count' as we do it for
> > other debugfs entries like pktlog etc
> 
> Not your code, but some of the xor uses are odd at best.

[shafi] ok.

> 
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> []
> > @@ -2122,7 +2122,7 @@ static ssize_t ath10k_write_btcoex(struct file *file,
> >  	struct ath10k *ar = file->private_data;
> >  	char buf[32];
> >  	size_t buf_size;
> > -	int ret = 0;
> > +	int ret;
> >  	bool val;
> >  
> >  	buf_size = min(count, (sizeof(buf) - 1));
> > @@ -2142,8 +2142,10 @@ static ssize_t ath10k_write_btcoex(struct file *file,
> >  		goto exit;
> >  	}
> >  
> > -	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val))
> > +	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val)) {
> 
> xor on an int and a bool.

[shafi] Should we change the 'val' to int, but this code seems to be working
test_bit seems to do this :
	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));

	do we see a problem, it should work right  (because they are bitwise
	'ANDING' with 1UL)

> 
> > @@ -2189,7 +2191,7 @@ static ssize_t ath10k_write_peer_stats(struct file *file,
> >  	struct ath10k *ar = file->private_data;
> >  	char buf[32];
> >  	size_t buf_size;
> > -	int ret = 0;
> > +	int ret;
> >  	bool val;
> >  
> >  	buf_size = min(count, (sizeof(buf) - 1));
> > @@ -2209,8 +2211,10 @@ static ssize_t ath10k_write_peer_stats(struct file *file,
> >  		goto exit;
> >  	}
> >  
> > -	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val))
> > +	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val)) {
> 
> here too

[shafi] same thing here ? is there a better code , kindly suggest

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kalle Valo April 19, 2016, 3:46 p.m. UTC | #3
Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> writes:

> From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
>
> Return value is incorrect for btcoex and peer stats debugfs
> 'write' entries if the user provides a value that matches with
> the already available debugfs entry, this results in the debugfs
> entry getting stuck and the operation has to be terminated manually.
> Fix this by returning the appropriate return 'count' as we do it for
> other debugfs entries like pktlog etc
>
> Fixes: cc61a1bbbc0e ("ath10k: enable debugfs provision to enable
> Peer Stats feature")
> Fixes: c28e6f06ff40 ("ath10k: fix sanity check on enabling btcoex via
> debugfs")
> Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>

Applied, thanks.

I removed the line wrapping from "Fixes:" lines.
Mohammed Shafi Shajakhan April 19, 2016, 3:57 p.m. UTC | #4
Applied, thanks.

I removed the line wrapping from "Fixes:" lines.

[shafi] thank you Kalle.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 76bbe17..e7d441c 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -2122,7 +2122,7 @@  static ssize_t ath10k_write_btcoex(struct file *file,
 	struct ath10k *ar = file->private_data;
 	char buf[32];
 	size_t buf_size;
-	int ret = 0;
+	int ret;
 	bool val;
 
 	buf_size = min(count, (sizeof(buf) - 1));
@@ -2142,8 +2142,10 @@  static ssize_t ath10k_write_btcoex(struct file *file,
 		goto exit;
 	}
 
-	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val))
+	if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags) ^ val)) {
+		ret = count;
 		goto exit;
+	}
 
 	if (val)
 		set_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags);
@@ -2189,7 +2191,7 @@  static ssize_t ath10k_write_peer_stats(struct file *file,
 	struct ath10k *ar = file->private_data;
 	char buf[32];
 	size_t buf_size;
-	int ret = 0;
+	int ret;
 	bool val;
 
 	buf_size = min(count, (sizeof(buf) - 1));
@@ -2209,8 +2211,10 @@  static ssize_t ath10k_write_peer_stats(struct file *file,
 		goto exit;
 	}
 
-	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val))
+	if (!(test_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags) ^ val)) {
+		ret = count;
 		goto exit;
+	}
 
 	if (val)
 		set_bit(ATH10K_FLAG_PEER_STATS, &ar->dev_flags);