All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] carl9170: Fix error return -EAGAIN if not started
@ 2021-10-08  0:15 Colin King
  2021-10-08  5:58 ` Dan Carpenter
  2021-10-10  8:05 ` Kalle Valo
  0 siblings, 2 replies; 6+ messages in thread
From: Colin King @ 2021-10-08  0:15 UTC (permalink / raw)
  To: Christian Lamparter, Kalle Valo, David S . Miller,
	Jakub Kicinski, John W . Linville, linux-wireless, netdev
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

There is an error return path where the error return is being
assigned to err rather than count and the error exit path does
not return -EAGAIN as expected. Fix this by setting the error
return to variable count as this is the value that is returned
at the end of the function.

Addresses-Coverity: ("Unused value")
Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/wireless/ath/carl9170/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
index bb40889d7c72..f163c6bdac8f 100644
--- a/drivers/net/wireless/ath/carl9170/debug.c
+++ b/drivers/net/wireless/ath/carl9170/debug.c
@@ -628,7 +628,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
 
 	case 'R':
 		if (!IS_STARTED(ar)) {
-			err = -EAGAIN;
+			count = -EAGAIN;
 			goto out;
 		}
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] carl9170: Fix error return -EAGAIN if not started
  2021-10-08  0:15 [PATCH] carl9170: Fix error return -EAGAIN if not started Colin King
@ 2021-10-08  5:58 ` Dan Carpenter
  2021-10-08  7:31   ` Colin Ian King
  2021-10-10  8:05 ` Kalle Valo
  1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-10-08  5:58 UTC (permalink / raw)
  To: Colin King
  Cc: Christian Lamparter, Kalle Valo, David S . Miller,
	Jakub Kicinski, John W . Linville, linux-wireless, netdev,
	kernel-janitors, linux-kernel

On Fri, Oct 08, 2021 at 01:15:58AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There is an error return path where the error return is being
> assigned to err rather than count and the error exit path does
> not return -EAGAIN as expected. Fix this by setting the error
> return to variable count as this is the value that is returned
> at the end of the function.
> 
> Addresses-Coverity: ("Unused value")
> Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/net/wireless/ath/carl9170/debug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
> index bb40889d7c72..f163c6bdac8f 100644
> --- a/drivers/net/wireless/ath/carl9170/debug.c
> +++ b/drivers/net/wireless/ath/carl9170/debug.c
> @@ -628,7 +628,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
>  
>  	case 'R':
>  		if (!IS_STARTED(ar)) {
> -			err = -EAGAIN;
> +			count = -EAGAIN;
>  			goto out;

This is ugly.  The bug wouldn't have happened with a direct return, it's
only the goto out which causes it.  Better to replace all the error
paths with direct returns.  There are two other direct returns so it's
not like a new thing...

Goto out on the success path is fine here, though.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] carl9170: Fix error return -EAGAIN if not started
  2021-10-08  5:58 ` Dan Carpenter
@ 2021-10-08  7:31   ` Colin Ian King
  2021-10-08  7:43     ` Dan Carpenter
  2021-10-08 15:15     ` Christian Lamparter
  0 siblings, 2 replies; 6+ messages in thread
From: Colin Ian King @ 2021-10-08  7:31 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Christian Lamparter, Kalle Valo, David S . Miller,
	Jakub Kicinski, John W . Linville, linux-wireless, netdev,
	kernel-janitors, linux-kernel

On 08/10/2021 06:58, Dan Carpenter wrote:
> On Fri, Oct 08, 2021 at 01:15:58AM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There is an error return path where the error return is being
>> assigned to err rather than count and the error exit path does
>> not return -EAGAIN as expected. Fix this by setting the error
>> return to variable count as this is the value that is returned
>> at the end of the function.
>>
>> Addresses-Coverity: ("Unused value")
>> Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   drivers/net/wireless/ath/carl9170/debug.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
>> index bb40889d7c72..f163c6bdac8f 100644
>> --- a/drivers/net/wireless/ath/carl9170/debug.c
>> +++ b/drivers/net/wireless/ath/carl9170/debug.c
>> @@ -628,7 +628,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
>>   
>>   	case 'R':
>>   		if (!IS_STARTED(ar)) {
>> -			err = -EAGAIN;
>> +			count = -EAGAIN;
>>   			goto out;
> 
> This is ugly.  The bug wouldn't have happened with a direct return, it's
> only the goto out which causes it.  Better to replace all the error
> paths with direct returns.  There are two other direct returns so it's
> not like a new thing...

Yep, I agree it was ugly, I was trying to keep to the coding style and 
reduce the patch delta size. I can do a V2 if the maintainers deem it's 
a cleaner solution.

> 
> Goto out on the success path is fine here, though.

Yep. I believe that a goto to one exit return point (may possibly?) make 
the code smaller rather than a sprinkling of returns in a function, so 
I'm never sure if this is a win or not with these kind of cases.

Colin
> 
> regards,
> dan carpenter
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] carl9170: Fix error return -EAGAIN if not started
  2021-10-08  7:31   ` Colin Ian King
@ 2021-10-08  7:43     ` Dan Carpenter
  2021-10-08 15:15     ` Christian Lamparter
  1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-10-08  7:43 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Christian Lamparter, Kalle Valo, David S . Miller,
	Jakub Kicinski, John W . Linville, linux-wireless, netdev,
	kernel-janitors, linux-kernel

On Fri, Oct 08, 2021 at 08:31:29AM +0100, Colin Ian King wrote:
> On 08/10/2021 06:58, Dan Carpenter wrote:
> > On Fri, Oct 08, 2021 at 01:15:58AM +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > There is an error return path where the error return is being
> > > assigned to err rather than count and the error exit path does
> > > not return -EAGAIN as expected. Fix this by setting the error
> > > return to variable count as this is the value that is returned
> > > at the end of the function.
> > > 
> > > Addresses-Coverity: ("Unused value")
> > > Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >   drivers/net/wireless/ath/carl9170/debug.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
> > > index bb40889d7c72..f163c6bdac8f 100644
> > > --- a/drivers/net/wireless/ath/carl9170/debug.c
> > > +++ b/drivers/net/wireless/ath/carl9170/debug.c
> > > @@ -628,7 +628,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
> > >   	case 'R':
> > >   		if (!IS_STARTED(ar)) {
> > > -			err = -EAGAIN;
> > > +			count = -EAGAIN;
> > >   			goto out;
> > 
> > This is ugly.  The bug wouldn't have happened with a direct return, it's
> > only the goto out which causes it.  Better to replace all the error
> > paths with direct returns.  There are two other direct returns so it's
> > not like a new thing...
> 
> Yep, I agree it was ugly, I was trying to keep to the coding style and
> reduce the patch delta size. I can do a V2 if the maintainers deem it's a
> cleaner solution.
> 
> > 
> > Goto out on the success path is fine here, though.
> 
> Yep. I believe that a goto to one exit return point (may possibly?) make the
> code smaller rather than a sprinkling of returns in a function, so I'm never
> sure if this is a win or not with these kind of cases.
> 

My default position is to hate goto out labels, but today I'm thinking
maybe they kind of make sense for read/write "return count;" functions...

This is debugfs code and debugfs code is always the worst code in any
driver...

The other style anti-patten here is to have two variables which store
error codes.  Maybe it would be better to do something like below.  Or
your patch is also find...  Let's just go with that.  It's fine.

regards,
dan carpenter

diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
index bb40889d7c72..8ccd0d3bea4c 100644
--- a/drivers/net/wireless/ath/carl9170/debug.c
+++ b/drivers/net/wireless/ath/carl9170/debug.c
@@ -616,7 +616,7 @@ DEBUGFS_DECLARE_RW_FILE(hw_ioread32, CARL9170_DEBUG_RING_SIZE * 40);
 static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
 					  size_t count)
 {
-	int err;
+	int err = 0;
 
 	if (count < 1)
 		return -EINVAL;
@@ -638,7 +638,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
 	case 'M':
 		err = carl9170_mac_reset(ar);
 		if (err < 0)
-			count = err;
+			goto out;
 
 		goto out;
 
@@ -646,7 +646,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
 		err = carl9170_set_channel(ar, ar->hw->conf.chandef.chan,
 			cfg80211_get_chandef_type(&ar->hw->conf.chandef));
 		if (err < 0)
-			count = err;
+			goto out;
 
 		goto out;
 
@@ -657,6 +657,8 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
 	carl9170_restart(ar, CARL9170_RR_USER_REQUEST);
 
 out:
+	if (err < 0)
+		return err;
 	return count;
 }
 


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] carl9170: Fix error return -EAGAIN if not started
  2021-10-08  7:31   ` Colin Ian King
  2021-10-08  7:43     ` Dan Carpenter
@ 2021-10-08 15:15     ` Christian Lamparter
  1 sibling, 0 replies; 6+ messages in thread
From: Christian Lamparter @ 2021-10-08 15:15 UTC (permalink / raw)
  To: Colin Ian King, Dan Carpenter
  Cc: Christian Lamparter, Kalle Valo, David S . Miller,
	Jakub Kicinski, John W . Linville, linux-wireless, netdev,
	kernel-janitors, linux-kernel

Hello,

On 08/10/2021 09:31, Colin Ian King wrote:
> On 08/10/2021 06:58, Dan Carpenter wrote:
>> On Fri, Oct 08, 2021 at 01:15:58AM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> There is an error return path where the error return is being
>>> assigned to err rather than count and the error exit path does
>>> not return -EAGAIN as expected. Fix this by setting the error
>>> return to variable count as this is the value that is returned
>>> at the end of the function.
>>>
>>> Addresses-Coverity: ("Unused value")
>>> Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>   drivers/net/wireless/ath/carl9170/debug.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/carl9170/debug.c b/drivers/net/wireless/ath/carl9170/debug.c
>>> index bb40889d7c72..f163c6bdac8f 100644
>>> --- a/drivers/net/wireless/ath/carl9170/debug.c
>>> +++ b/drivers/net/wireless/ath/carl9170/debug.c
>>> @@ -628,7 +628,7 @@ static ssize_t carl9170_debugfs_bug_write(struct ar9170 *ar, const char *buf,
>>>       case 'R':
>>>           if (!IS_STARTED(ar)) {
>>> -            err = -EAGAIN;
>>> +            count = -EAGAIN;
>>>               goto out;
>>
>> This is ugly.  The bug wouldn't have happened with a direct return, it's
>> only the goto out which causes it.  Better to replace all the error
>> paths with direct returns.  There are two other direct returns so it's
>> not like a new thing...
> 
> Yep, I agree it was ugly, I was trying to keep to the coding style and reduce the patch delta size. I can do a V2 if the maintainers deem it's a cleaner solution.

Hm? I don't think there's any need to stick to a particular
coding style. This file hasn't been touched a lot since 2010.
Things moved on and replacing the gotos with straight return
is totally fine.

(It has to pass the build checkers of course. However I don't
think this will be a problem here...)

Cheers,
Christian

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] carl9170: Fix error return -EAGAIN if not started
  2021-10-08  0:15 [PATCH] carl9170: Fix error return -EAGAIN if not started Colin King
  2021-10-08  5:58 ` Dan Carpenter
@ 2021-10-10  8:05 ` Kalle Valo
  1 sibling, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2021-10-10  8:05 UTC (permalink / raw)
  To: Colin King
  Cc: Christian Lamparter, David S . Miller, Jakub Kicinski,
	John W . Linville, linux-wireless, netdev, kernel-janitors,
	linux-kernel

Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
> 
> There is an error return path where the error return is being
> assigned to err rather than count and the error exit path does
> not return -EAGAIN as expected. Fix this by setting the error
> return to variable count as this is the value that is returned
> at the end of the function.
> 
> Addresses-Coverity: ("Unused value")
> Fixes: 00c4da27a421 ("carl9170: firmware parser and debugfs code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

I assume there will be v2 so dropping this version.

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20211008001558.32416-1-colin.king@canonical.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-10  8:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08  0:15 [PATCH] carl9170: Fix error return -EAGAIN if not started Colin King
2021-10-08  5:58 ` Dan Carpenter
2021-10-08  7:31   ` Colin Ian King
2021-10-08  7:43     ` Dan Carpenter
2021-10-08 15:15     ` Christian Lamparter
2021-10-10  8:05 ` Kalle Valo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.