[PATCH 1/1] forcedeth: Delete a redundant condition branch

[PATCH 1/1] forcedeth: Delete a redundant condition branch

[Zhen Lei]

The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
as the "else" branch. Delete it to simplify code.

No functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.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 8724d6a9ed020a7..5c2c9c5d330b65f 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -3471,9 +3471,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;
-- 
2.26.0.106.g9fadedd

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Zhu Yanjun]

On Mon, May 10, 2021 at 9:57 PM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>
> The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
> as the "else" branch. Delete it to simplify code.
>
> No functional change.

Thanks.
Missing Fixes?

Zhu Yanjun

>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.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 8724d6a9ed020a7..5c2c9c5d330b65f 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -3471,9 +3471,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;
> --
> 2.26.0.106.g9fadedd
>
>

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Jakub Kicinski]

On Mon, 10 May 2021 22:19:34 +0800 Zhu Yanjun wrote:
> On Mon, May 10, 2021 at 9:57 PM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >
> > The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
> > as the "else" branch. Delete it to simplify code.
> >
> > No functional change.  
> 
> Thanks.
> Missing Fixes?

Fixes tag is supposed to be used for functional fixes.

This patch (and the stmmac one) removes a branch based on the fact that
it's the same as the default / catch all case. It's has a net negative
effect on the reability of the code since now not all cases are
explicitly enumerated. But it's at least the 3rd time we got that
stmmac patch so perhaps not worth fighting the bots...

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Andrew Lunn]

> This patch (and the stmmac one) removes a branch based on the fact that
> it's the same as the default / catch all case. It's has a net negative
> effect on the reability of the code since now not all cases are
> explicitly enumerated. But it's at least the 3rd time we got that
> stmmac patch so perhaps not worth fighting the bots...

Hi Jakub

Is it the same bot every time? Or are the masters of the bots learning
what good code actually looks like and fixing their bots? Unless we
push back, the bot masters are not going to get any better at managing
their bots.

    Andrew

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Jakub Kicinski]

On Mon, 10 May 2021 21:52:38 +0200 Andrew Lunn wrote:
> > This patch (and the stmmac one) removes a branch based on the fact that
> > it's the same as the default / catch all case. It's has a net negative
> > effect on the reability of the code since now not all cases are
> > explicitly enumerated. But it's at least the 3rd time we got that
> > stmmac patch so perhaps not worth fighting the bots...  
> 
> Hi Jakub
> 
> Is it the same bot every time? Or are the masters of the bots learning
> what good code actually looks like and fixing their bots? Unless we
> push back, the bot masters are not going to get any better at managing
> their bots.

I think 2 may have been the same bot (Hulk), I found 4 previous
attempts:

https://lore.kernel.org/netdev/1616039414-13288-1-git-send-email-f.fangjian@huawei.com/
https://lore.kernel.org/netdev/1603938832-53705-1-git-send-email-zou_wei@huawei.com/
https://lore.kernel.org/netdev/1576060284-12371-1-git-send-email-vulab@iscas.ac.cn/
https://lore.kernel.org/netdev/20200602104405.28851-1-aishwaryarj100@gmail.com/

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Leon Romanovsky]

On Mon, May 10, 2021 at 04:31:47PM -0700, Jakub Kicinski wrote:
> On Mon, 10 May 2021 21:52:38 +0200 Andrew Lunn wrote:
> > > This patch (and the stmmac one) removes a branch based on the fact that
> > > it's the same as the default / catch all case. It's has a net negative
> > > effect on the reability of the code since now not all cases are
> > > explicitly enumerated. But it's at least the 3rd time we got that
> > > stmmac patch so perhaps not worth fighting the bots...  
> > 
> > Hi Jakub
> > 
> > Is it the same bot every time? Or are the masters of the bots learning
> > what good code actually looks like and fixing their bots? Unless we
> > push back, the bot masters are not going to get any better at managing
> > their bots.
> 
> I think 2 may have been the same bot (Hulk), I found 4 previous
> attempts:
> 
> https://lore.kernel.org/netdev/1616039414-13288-1-git-send-email-f.fangjian@huawei.com/
> https://lore.kernel.org/netdev/1603938832-53705-1-git-send-email-zou_wei@huawei.com/
> https://lore.kernel.org/netdev/1576060284-12371-1-git-send-email-vulab@iscas.ac.cn/
> https://lore.kernel.org/netdev/20200602104405.28851-1-aishwaryarj100@gmail.com/

We (RDMA) got them too and decided that it is not worth to fight against them.
https://lore.kernel.org/linux-rdma/YJkByCnQGcLOIlCz@unreal/T/#m66ec31acf5e0fea5233a4b7265a3a49232492a4a

Thanks

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Andrew Lunn]

On Mon, May 10, 2021 at 09:56:56PM +0800, Zhen Lei wrote:
> The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
> as the "else" branch. Delete it to simplify code.
> 
> No functional change.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Hi Zhen

Could you teach your bot to check lore.kernel.org and see if the same
patch has been submitted before? If it has, there is probably a reason
why it was rejected. You need to check if that reason it still true.

    Andrew

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Leizhen (ThunderTown)]

On 2021/5/11 3:59, Andrew Lunn wrote:
> On Mon, May 10, 2021 at 09:56:56PM +0800, Zhen Lei wrote:
>> The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
>> as the "else" branch. Delete it to simplify code.
>>
>> No functional change.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Hi Zhen
> 
> Could you teach your bot to check lore.kernel.org and see if the same
> patch has been submitted before? If it has, there is probably a reason
> why it was rejected. You need to check if that reason it still true.

This is a tool that comes with the kernel. Now it's all about manual
Google searches to see if someone has posted it. So there could be a
mistake.

Although the compiler can optimize this "if" branch, but I think those
that can optimize directly should try to avoid relying on the machine.
If it must exist, it should be in the form of comments. Otherwise, the
intuition is that there was a mistake in writing this code. That's why
the kernel tool reports it. At least the developers of the tool has the
same point of view as mine.

> 
>     Andrew
> 
> .
> 

Re: [PATCH 1/1] forcedeth: Delete a redundant condition branch

[Andrew Lunn]

> If it must exist, it should be in the form of comments. Otherwise, the
> intuition is that there was a mistake in writing this code.

I agree that it could look like there is an error in the code. But
that is where the bot stops and the human takes over. When you read
the code, and understand what it does, you can see there is no
error. You should also look at the history. This is old code, if it
was broken, it would of been fixed by now, since the hulk bot has been
around for quiet a while.

For me, the optimization argument is not correct. We have a lot of
code, in macros for example, where we assume the compiler will
optimize it. Think about all the

if (IS_ENABLED(CONFIG_FOO))
	{}

code. The code you are suggesting to change is also on the very slow
path. We want to optimize it for human understandability, not code
generation. It is much more important humans understand it, than the
few microsecods it takes the compiler to optimize it.

    Andrew