[PATCH net-next] net/rose: Remove unnecessary if check in rose_dev_first()

[PATCH net-next] net/rose: Remove unnecessary if check in rose_dev_first()

[Thorsten Blum]

dev_hold() already checks if its argument is NULL.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 net/rose/rose_route.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
index b72bf8a08d48..35e21a2bec9c 100644
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -608,8 +608,7 @@ struct net_device *rose_dev_first(void)
 			if (first == NULL || strncmp(dev->name, first->name, 3) < 0)
 				first = dev;
 	}
-	if (first)
-		dev_hold(first);
+	dev_hold(first);
 	rcu_read_unlock();
 
 	return first;
-- 
2.50.0

Re: [PATCH net-next] net/rose: Remove unnecessary if check in rose_dev_first()

[Simon Horman]

On Fri, Jul 04, 2025 at 10:33:08AM +0200, Thorsten Blum wrote:
> dev_hold() already checks if its argument is NULL.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Hi Thorsten,

I agree that this is correct. But I think that cleanup like this
needs to be in the context of other changes to make it worthwhile.

Quoting documentation:

  Clean-up patches
  ~~~~~~~~~~~~~~~~

  Netdev discourages patches which perform simple clean-ups, which are not in
  the context of other work. For example:

  * Addressing ``checkpatch.pl`` warnings
  * Addressing :ref:`Local variable ordering<rcs>` issues
  * Conversions to device-managed APIs (``devm_`` helpers)

  This is because it is felt that the churn that such changes produce comes
  at a greater cost than the value of such clean-ups.

  Conversely, spelling and grammar fixes are not discouraged.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#clean-up-patches

--
pw-bot: cr

Re: [PATCH net-next] net/rose: Remove unnecessary if check in rose_dev_first()

[Dan Carpenter]

On Fri, Jul 04, 2025 at 10:33:08AM +0200, Thorsten Blum wrote:
> dev_hold() already checks if its argument is NULL.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  net/rose/rose_route.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
> index b72bf8a08d48..35e21a2bec9c 100644
> --- a/net/rose/rose_route.c
> +++ b/net/rose/rose_route.c
> @@ -608,8 +608,7 @@ struct net_device *rose_dev_first(void)
>  			if (first == NULL || strncmp(dev->name, first->name, 3) < 0)
>  				first = dev;
>  	}
> -	if (first)
> -		dev_hold(first);
> +	dev_hold(first);

I'm not a fan of these sorts of "remove the NULL check" patches in
general.  Sure it removes a line of code, but does it really improve
readability?  I feel like someone reading this code might think a NULL
check was required.

I guess there is also an argument that this is a tiny speedup.  That
could be a valid argument especially if we had benchmarking data to back
it up.

Of course, if you're planning to take over this code and be the
maintainer of it, then you get to do whatever you feel is best.  So if
this change were part of a larger change where you were taking over then
that's fine.

regards,
dan carpenter