| 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 |
| 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 |
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 > >
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...
> 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
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
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/
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 > > . >
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
> 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 --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;
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(-)