From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751187AbdALQFB (ORCPT ); Thu, 12 Jan 2017 11:05:01 -0500 Received: from mout.kundenserver.de ([212.227.126.134]:55767 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750914AbdALQFA (ORCPT ); Thu, 12 Jan 2017 11:05:00 -0500 From: Arnd Bergmann To: Or Gerlitz Cc: Saeed Mahameed , Hadar Hen Zion , "David S . Miller" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning Date: Thu, 12 Jan 2017 17:04:43 +0100 Message-ID: <2981112.7jTLlX72ae@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <46a85790-2cfe-a8d9-f764-4f736fbd1af7@mellanox.com> References: <20170111211451.2705705-1-arnd@arndb.de> <46a85790-2cfe-a8d9-f764-4f736fbd1af7@mellanox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:LwqKE3qgUF0QVT0Z43iFY3QXPsLr+JmhF4tr0+wVyviU4Yv2Ala Qdtj8yHFEbgebucAWQtkxwOx3CMhXUeqybv0tWhUxrI84nXGy8JzrAFA7uvvJDnoSNansMm wKXqJmv79JCHMXGueQnPaL4HODD0BDh+T4d77HqXFfwOM8CaE7NblXODLkbrEQiQLJ1h9Dl nrPmEn0qhyi53C0uhAscw== X-UI-Out-Filterresults: notjunk:1;V01:K0:y/RjKDXC6Mo=:HMNx4P5gmXDsgthyt50oXY d6uwftXsD87Ijw81Wka+pb3aP1VOS4ntkKZBr8sbjx4aRbTe3AiRlLmoMgUjzJX4G3T6PCMEy 4VyxlABMyoAxqxXw6tiOisy2vyf804bOha02lbdy8VL0RTPqFgp58oW1p+mgbs6mrTIx877g6 TSaxDicYlJuSt/FKOqwqDz632nuV3krBU2SiK5G2E+5I85qqMfBUffE3zd9rghxEKBhkT0dS6 GXgY7+NOCMPN4hO03U8tnoZMU4KYAe8OwG1T8tiDkYp8VJiLVUOmrVMJ102dWNCHsmX/fpuNz OWfTUTMlxhVMdTCAhiYQkSNJOjiOawihR6T6BHDQYGIomLqF/ahWtfPjcs78U12H5oq/kQ1OV zpxahnfxo9whRPsCK3zM/h8UOTnCTWbUODxbZuBYQsjcNpTt41aMARnH3zE9ueHKK4p7LmJey 8gRZcCWe0f6Z0EA1/0jGYuBrT5U/GANQK5vHXwrAqHU2nPRYAjnTR9QUMAuF8JOPQ5/MR0lFA LPPBJ/JvzE7HnjGkfDVe6ZwQhwBPVyep5F/f3oHqfR5+G7W0eDm73LIYBz4CkeMcbFU6u0aAR amYhYr1Dq7Bq2U8bVMEGEqYEuYln8hovEfIBW0YEdjedX48JFfN71PD103QD2xghVlzSI0v8F BbuhpMElZV+u+T18PcXctpEgQUcTDn7KdRzlDijLWpfCstaQl7H03Nsq1/qFN6qDXGaIH3lhS ORrdZxRSUNQJop9P Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, January 12, 2017 5:21:49 PM CET Or Gerlitz wrote: > On 1/11/2017 11:14 PM, Arnd Bergmann wrote: > > As found by Olof's build bot, today's mainline kernel gained a harmless > > warning about a potential uninitalied variable reference: > > > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'parse_tc_fdb_actions': > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:769:13: warning: 'out_dev' may be used uninitialized in this function [-Wmaybe-uninitialized] > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:811:21: note: 'out_dev' was declared here > > > > This was introduced through the addition of an 'IS_ERR/PTR_ERR' pair that > > gcc is unfortunately unable to completely figure out. Replacing it with > > PTR_ERR_OR_ZERO makes the code more understandable to gcc so it no longer > > warns. > > can you elaborate on this a little further? The problem is static int mlx5e_route_lookup_ipv4(struct net_device **out_dev, ...) { ... if (IS_ERR(rt)) return PTR_ERR(rt); *out_dev = ...; ... } static int mlx5e_create_encap_header_ipv4(...) { ... err = mlx5e_route_lookup_ipv4(..., out_dev, ...); if (err) goto out; e->out_dev = *out_dev; ... } I've seen several examples of this, the problem every time is that gcc cannot tell that if(IS_ERR()) in the first function is equivalent to if(err) in the second, so it assumes that 'out_dev' is used here after the first 'return PTR_ERR(rt)'. The PTR_ERR_OR_ZERO() case by comparison is fairly easy to detect by gcc, so it can't get that wrong here. > > Hadar Hen Zion already attempted to fix the warning earlier by adding > > fake initializations, but that ended up just making the code worse without > > fully addressing all warnings, so I'm reverting it now that it is no longer needed. > > ok, so if your approach eliminates the warning on out_dev and also on > the variables for which Hadar added the faked initializers, I guess we > should be fine with this change (saw your reply on my other comment), Ok. > just another question: > > > In order to avoid pulling a variable declaration into the #ifdef, I'm > > removing it in favor of a more readable 'if()' statement here that has the same effect. > > When I build here without CONFIG_INET in my system, the build goes fine > with this approach. However, we're pretty sure that in the past we got > 0-day report from the kbuild test robot where he was unhappy that we > make the ip_route_output_key call without being wrapped with that #if > IS_ENABLED(CONFIG_INET) -- so, we don't want to go there again... thoughts? I went back and forth between the two versions, either leaving the #if in place, or using the if(IS_ENABLED()) check to be really sure that we can't get compile error here. I did check that ip_route_output_key() is always declared, but now I see that net/route.h might not always be included from en_tc.c if CONFIG_INET is disabled (I don't see how it gets included, but it obviously is when CONFIG_INET is turned on). Adding an explicit include of that file should probably avoid the case you ran into earlier, but for I agree it's safer to not rely on that here for a bugfix, and just leave the #ifdef. Do you want to modify it yourself, or should I spin a new version with that? Arnd