lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20201029122420.GG933237@lunn.ch>
Date:   Thu, 29 Oct 2020 13:24:20 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Zou Wei <zou_wei@...wei.com>
Cc:     rain.1986.08.12@...il.com, zyjzyj2000@...il.com,
        davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] net: nvidia: forcedeth: remove useless if/else

On Thu, Oct 29, 2020 at 10:30:14AM +0800, Zou Wei wrote:
> Fix the following coccinelle report:
> 
> ./drivers/net/ethernet/nvidia/forcedeth.c:3479:8-10:
> WARNING: possible condition with no effect (if == else)
> 
> Both branches are the same, so remove the else if/else altogether.
> 
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Zou Wei <zou_wei@...wei.com>
> ---
>  drivers/net/ethernet/nvidia/forcedeth.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 2fc10a3..87ed7e1 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -3476,9 +3476,6 @@ static int nv_update_linkspeed(struct net_device *dev)
>  	} else if (adv_lpa & LPA_10FULL) {
>  		newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
>  		newdup = 1;
> -	} else if (adv_lpa & LPA_10HALF) {
> -		newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
> -		newdup = 0;
>  	} else {
>  		newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
>  		newdup = 0;

I think the original code is more readable. The idea is, you look at
what each end of the link can do, and work your way from fastest to
slowest finding one in common. That is what the four if () do. If
there is no speed in common, the link is probably not going to work,
but default to 10Half, because all devices should in theory support
that. That is the last else. The change makes it a lot less clear
about this last past.

How about this instead. It keeps the idea of, we have nothing else
better, do 10Half.

This is not even compile tested.

    Andrew


diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 2fc10a36afa4..f626bd6c0dfc 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -3467,6 +3467,8 @@ static int nv_update_linkspeed(struct net_device *dev)
 
        /* FIXME: handle parallel detection properly */
        adv_lpa = lpa & adv;
+       newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
+       newdup = 0;
        if (adv_lpa & LPA_100FULL) {
                newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_100;
                newdup = 1;
@@ -3479,9 +3481,6 @@ static int nv_update_linkspeed(struct net_device *dev)
        } else if (adv_lpa & LPA_10HALF) {
                newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
                newdup = 0;
-       } else {
-               newls = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
-               newdup = 0;
        }
 
 set_speed:

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ