diff mbox series

[net-next,4/7] net: ipa: ipa_stop() does not return an error

Message ID 20210409180722.1176868-5-elder@linaro.org
State New
Headers show
Series net: ipa: a few small fixes | expand

Commit Message

Alex Elder April 9, 2021, 6:07 p.m. UTC 
In ipa_modem_stop(), if the modem netdev pointer is non-null we call
ipa_stop().  We check for an error and if one is returned we handle
it.  But ipa_stop() never returns an error, so this extra handling
is unnecessary.  Simplify the code in ipa_modem_stop() based on the
knowledge no error handling is needed at this spot.

Signed-off-by: Alex Elder <elder@linaro.org>

---
 drivers/net/ipa/ipa_modem.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

-- 
2.27.0

Comments

Leon Romanovsky April 11, 2021, 6:34 a.m. UTC | #1 
On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:
> In ipa_modem_stop(), if the modem netdev pointer is non-null we call

> ipa_stop().  We check for an error and if one is returned we handle

> it.  But ipa_stop() never returns an error, so this extra handling

> is unnecessary.  Simplify the code in ipa_modem_stop() based on the

> knowledge no error handling is needed at this spot.

> 

> Signed-off-by: Alex Elder <elder@linaro.org>

> ---

>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------

>  1 file changed, 4 insertions(+), 14 deletions(-)


<...>

> +	/* Stop the queue and disable the endpoints if it's open */

>  	if (netdev) {

> -		/* Stop the queue and disable the endpoints if it's open */

> -		ret = ipa_stop(netdev);

> -		if (ret)

> -			goto out_set_state;

> -

> +		(void)ipa_stop(netdev);


This void casting is not needed here and in more general case sometimes
even be seen as a mistake, for example if the returned attribute declared
as __must_check.

Thanks
Alex Elder April 11, 2021, 1:09 p.m. UTC | #2 
On 4/11/21 1:34 AM, Leon Romanovsky wrote:
> On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:

>> In ipa_modem_stop(), if the modem netdev pointer is non-null we call

>> ipa_stop().  We check for an error and if one is returned we handle

>> it.  But ipa_stop() never returns an error, so this extra handling

>> is unnecessary.  Simplify the code in ipa_modem_stop() based on the

>> knowledge no error handling is needed at this spot.

>>

>> Signed-off-by: Alex Elder <elder@linaro.org>

>> ---

>>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------

>>  1 file changed, 4 insertions(+), 14 deletions(-)

> 

> <...>

> 

>> +	/* Stop the queue and disable the endpoints if it's open */

>>  	if (netdev) {

>> -		/* Stop the queue and disable the endpoints if it's open */

>> -		ret = ipa_stop(netdev);

>> -		if (ret)

>> -			goto out_set_state;

>> -

>> +		(void)ipa_stop(netdev);

> 

> This void casting is not needed here and in more general case sometimes

> even be seen as a mistake, for example if the returned attribute declared

> as __must_check.


I accept your point but I feel like it's sort of a 50/50 thing.

I think *not* checking an available return value is questionable
practice.  I'd really rather have a build option for a
"__need_not_check" tag and have "must_check" be the default.

The void cast here says "I know this returns a result, but I am
intentionally not checking it."  If it had been __must_check I
would certainly have checked it.  

That being said, I don't really care that much, so I'll plan
to post version 2, which will drop this cast (I'll probably
add a comment though).

Thanks.

					-Alex

> 

> Thanks

>
Leon Romanovsky April 11, 2021, 1:28 p.m. UTC | #3 
On Sun, Apr 11, 2021 at 08:09:55AM -0500, Alex Elder wrote:
> On 4/11/21 1:34 AM, Leon Romanovsky wrote:

> > On Fri, Apr 09, 2021 at 01:07:19PM -0500, Alex Elder wrote:

> >> In ipa_modem_stop(), if the modem netdev pointer is non-null we call

> >> ipa_stop().  We check for an error and if one is returned we handle

> >> it.  But ipa_stop() never returns an error, so this extra handling

> >> is unnecessary.  Simplify the code in ipa_modem_stop() based on the

> >> knowledge no error handling is needed at this spot.

> >>

> >> Signed-off-by: Alex Elder <elder@linaro.org>

> >> ---

> >>  drivers/net/ipa/ipa_modem.c | 18 ++++--------------

> >>  1 file changed, 4 insertions(+), 14 deletions(-)

> > 

> > <...>

> > 

> >> +	/* Stop the queue and disable the endpoints if it's open */

> >>  	if (netdev) {

> >> -		/* Stop the queue and disable the endpoints if it's open */

> >> -		ret = ipa_stop(netdev);

> >> -		if (ret)

> >> -			goto out_set_state;

> >> -

> >> +		(void)ipa_stop(netdev);

> > 

> > This void casting is not needed here and in more general case sometimes

> > even be seen as a mistake, for example if the returned attribute declared

> > as __must_check.

> 

> I accept your point but I feel like it's sort of a 50/50 thing.

> 

> I think *not* checking an available return value is questionable

> practice.  I'd really rather have a build option for a

> "__need_not_check" tag and have "must_check" be the default.


__need_not_check == void ???

> 

> The void cast here says "I know this returns a result, but I am

> intentionally not checking it."  If it had been __must_check I

> would certainly have checked it.  

> 

> That being said, I don't really care that much, so I'll plan

> to post version 2, which will drop this cast (I'll probably

> add a comment though).


Thanks

> 

> Thanks.

> 

> 					-Alex

> 

> > 

> > Thanks

> > 

>
Alex Elder April 11, 2021, 1:42 p.m. UTC | #4 
On 4/11/21 8:28 AM, Leon Romanovsky wrote:
>> I think *not* checking an available return value is questionable

>> practice.  I'd really rather have a build option for a

>> "__need_not_check" tag and have "must_check" be the default.

> __need_not_check == void ???


I'm not sure I understand your statement here, but...

My point is, I'd rather have things like printk() and
strscpy() be marked with (an imaginary) __need_not_check,
than the way things are, with only certain functions being
marked __must_check.

In my view, if a function returns a value, all callers
of that function ought to be checking it.  If the return
value is not necessary it should be a void function if
possible.

I don't expect the world to change, but I just think the
default should be "must check" rather than "check optional".

					-Alex
Leon Romanovsky April 12, 2021, 7:26 a.m. UTC | #5 
On Sun, Apr 11, 2021 at 08:42:15AM -0500, Alex Elder wrote:
> On 4/11/21 8:28 AM, Leon Romanovsky wrote:

> >> I think *not* checking an available return value is questionable

> >> practice.  I'd really rather have a build option for a

> >> "__need_not_check" tag and have "must_check" be the default.

> > __need_not_check == void ???

> 

> I'm not sure I understand your statement here, but...


We are talking about the same thing. My point was that __need_not_check
is actually void. The API author was supposed to declare that by
declaring that function doesn't return anything.

Thanks
Alex Elder April 12, 2021, 7:45 a.m. UTC | #6 
On 4/12/21 2:26 AM, Leon Romanovsky wrote:
> On Sun, Apr 11, 2021 at 08:42:15AM -0500, Alex Elder wrote:

>> On 4/11/21 8:28 AM, Leon Romanovsky wrote:

>>>> I think *not* checking an available return value is questionable

>>>> practice.  I'd really rather have a build option for a

>>>> "__need_not_check" tag and have "must_check" be the default.

>>> __need_not_check == void ???

>>

>> I'm not sure I understand your statement here, but...

> 

> We are talking about the same thing. My point was that __need_not_check

> is actually void. The API author was supposed to declare that by

> declaring that function doesn't return anything.


No, we are not.

Functions like strcpy() return a value, but that value is almost
never checked.  The returned value isn't an error, so there is
no real need to check that return value.  This is the kind of
thing I'm talking about that might be tagged __need_not_check.

A function that returns a value for no reason should be void,
I agree with that.

In the ipa_stop() case, the value *must* be returned because
it serves as an ->ndo_stop() function and has to adhere to
that function prototype.  The point of the current patch
was to simplify the code (defined privately in the current
source file), given knowledge that it never returns an error.

The compiler could ensure all calls to functions that return
a value actually check the return value.  And because I think
that's the best practice, I'd like to be able to run such a
check in my code.  But there are always exceptions, and that
would be the purpose of a __need_not_check tag.

I don't think this is worthy of any more discussion.

					-Alex
diff mbox series

Patch

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 8a6ccebde2889..af9aedbde717a 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -240,7 +240,6 @@  int ipa_modem_stop(struct ipa *ipa)
 {
 	struct net_device *netdev = ipa->modem_netdev;
 	enum ipa_modem_state state;
-	int ret;
 
 	/* Only attempt to stop the modem if it's running */
 	state = atomic_cmpxchg(&ipa->modem_state, IPA_MODEM_STATE_RUNNING,
@@ -257,29 +256,20 @@  int ipa_modem_stop(struct ipa *ipa)
 	/* Prevent the modem from triggering a call to ipa_setup() */
 	ipa_smp2p_disable(ipa);
 
+	/* Stop the queue and disable the endpoints if it's open */
 	if (netdev) {
-		/* Stop the queue and disable the endpoints if it's open */
-		ret = ipa_stop(netdev);
-		if (ret)
-			goto out_set_state;
-
+		(void)ipa_stop(netdev);
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		ipa->modem_netdev = NULL;
 		unregister_netdev(netdev);
 		free_netdev(netdev);
-	} else {
-		ret = 0;
 	}
 
-out_set_state:
-	if (ret)
-		atomic_set(&ipa->modem_state, IPA_MODEM_STATE_RUNNING);
-	else
-		atomic_set(&ipa->modem_state, IPA_MODEM_STATE_STOPPED);
+	atomic_set(&ipa->modem_state, IPA_MODEM_STATE_STOPPED);
 	smp_mb__after_atomic();
 
-	return ret;
+	return 0;
 }
 
 /* Treat a "clean" modem stop the same as a crash */