diff mbox series

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

Message ID 20210510135656.3960-1-thunder.leizhen@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [1/1] forcedeth: Delete a redundant condition branch | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link

Commit Message

Leizhen (ThunderTown) May 10, 2021, 1:56 p.m. UTC
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(-)

Comments

Zhu Yanjun May 10, 2021, 2:19 p.m. UTC | #1
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
>
>
Jakub Kicinski May 10, 2021, 6:31 p.m. UTC | #2
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...
Andrew Lunn May 10, 2021, 7:52 p.m. UTC | #3
> 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
Andrew Lunn May 10, 2021, 7:59 p.m. UTC | #4
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
Jakub Kicinski May 10, 2021, 11:31 p.m. UTC | #5
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/
Leizhen (ThunderTown) May 11, 2021, 1:43 a.m. UTC | #6
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
> 
> .
>
Leon Romanovsky May 11, 2021, 5:24 a.m. UTC | #7
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
Andrew Lunn May 11, 2021, 12:20 p.m. UTC | #8
> 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
diff mbox series

Patch

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;