linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
@ 2023-02-17  9:56 Arnd Bergmann
  2023-02-17 16:13 ` Edward Cree
  2023-02-20 10:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2023-02-17  9:56 UTC (permalink / raw)
  To: Edward Cree, Martin Habets, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Alejandro Lucero
  Cc: Arnd Bergmann, Jonathan Cooper, netdev, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

One local variable has become unused after a recent change:

drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
  struct net_device *net_dev = efx->net_dev;
                     ^~~~~~~

The variable is still used in an #ifdef. Replace the #ifdef with
an if(IS_ENABLED()) check that lets the compiler see where it is
used, rather than adding another #ifdef.

This also fixes an uninitialized return value in ef100_probe_netdev_pf()
that gcc did not spot.

Fixes: 7e056e2360d9 ("sfc: obtain device mac address based on firmware handle for ef100")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index becd21c2325d..4dc643b0d2db 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -399,14 +399,14 @@ static int ef100_filter_table_up(struct efx_nic *efx)
 	 * filter insertion will need to take the lock for read.
 	 */
 	up_write(&efx->filter_sem);
-#ifdef CONFIG_SFC_SRIOV
-	rc = efx_tc_insert_rep_filters(efx);
+	if (IS_ENABLED(CONFIG_SFC_SRIOV))
+		rc = efx_tc_insert_rep_filters(efx);
+
 	/* Rep filter failure is nonfatal */
 	if (rc)
 		netif_warn(efx, drv, efx->net_dev,
 			   "Failed to insert representor filters, rc %d\n",
 			   rc);
-#endif
 	return 0;
 
 fail_vlan0:
@@ -419,9 +419,8 @@ static int ef100_filter_table_up(struct efx_nic *efx)
 
 static void ef100_filter_table_down(struct efx_nic *efx)
 {
-#ifdef CONFIG_SFC_SRIOV
-	efx_tc_remove_rep_filters(efx);
-#endif
+	if (IS_ENABLED(CONFIG_SFC_SRIOV))
+		efx_tc_remove_rep_filters(efx);
 	down_write(&efx->filter_sem);
 	efx_mcdi_filter_del_vlan(efx, 0);
 	efx_mcdi_filter_del_vlan(efx, EFX_FILTER_VID_UNSPEC);
@@ -737,7 +736,6 @@ static unsigned int efx_ef100_recycle_ring_size(const struct efx_nic *efx)
 	return 10 * EFX_RECYCLE_RING_SIZE_10G;
 }
 
-#ifdef CONFIG_SFC_SRIOV
 static int efx_ef100_get_base_mport(struct efx_nic *efx)
 {
 	struct ef100_nic_data *nic_data = efx->nic_data;
@@ -773,7 +771,6 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx)
 
 	return 0;
 }
-#endif
 
 static int compare_versions(const char *a, const char *b)
 {
@@ -1155,10 +1152,9 @@ int ef100_probe_netdev_pf(struct efx_nic *efx)
 	struct net_device *net_dev = efx->net_dev;
 	int rc;
 
-	if (!nic_data->grp_mae)
+	if (!IS_ENABLED(CONFIG_SFC_SRIOV) || !nic_data->grp_mae)
 		return 0;
 
-#ifdef CONFIG_SFC_SRIOV
 	rc = efx_init_struct_tc(efx);
 	if (rc)
 		return rc;
@@ -1193,7 +1189,6 @@ int ef100_probe_netdev_pf(struct efx_nic *efx)
 		net_dev->features |= NETIF_F_HW_TC;
 		efx->fixed_features |= NETIF_F_HW_TC;
 	}
-#endif
 	return rc;
 }
 
@@ -1206,12 +1201,11 @@ void ef100_remove(struct efx_nic *efx)
 {
 	struct ef100_nic_data *nic_data = efx->nic_data;
 
-#ifdef CONFIG_SFC_SRIOV
-	if (efx->mae) {
+	if (IS_ENABLED(CONFIG_SFC_SRIOV) && efx->mae) {
 		efx_ef100_fini_reps(efx);
 		efx_fini_mae(efx);
 	}
-#endif
+
 	efx_mcdi_detach(efx);
 	efx_mcdi_fini(efx);
 	if (nic_data)
@@ -1304,9 +1298,8 @@ const struct efx_nic_type ef100_pf_nic_type = {
 	.update_stats = ef100_update_stats,
 	.pull_stats = efx_mcdi_mac_pull_stats,
 	.stop_stats = efx_mcdi_mac_stop_stats,
-#ifdef CONFIG_SFC_SRIOV
-	.sriov_configure = efx_ef100_sriov_configure,
-#endif
+	.sriov_configure = IS_ENABLED(CONFIG_SFC_SRIOV) ?
+		efx_ef100_sriov_configure : NULL,
 
 	/* Per-type bar/size configuration not used on ef100. Location of
 	 * registers is defined by extended capabilities.
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
  2023-02-17  9:56 [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV Arnd Bergmann
@ 2023-02-17 16:13 ` Edward Cree
  2023-02-17 16:19   ` Arnd Bergmann
  2023-02-19  9:31   ` Leon Romanovsky
  2023-02-20 10:50 ` patchwork-bot+netdevbpf
  1 sibling, 2 replies; 6+ messages in thread
From: Edward Cree @ 2023-02-17 16:13 UTC (permalink / raw)
  To: Arnd Bergmann, Martin Habets, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Alejandro Lucero
  Cc: Arnd Bergmann, Jonathan Cooper, netdev, linux-kernel, Leon Romanovsky

On 17/02/2023 09:56, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> One local variable has become unused after a recent change:
> 
> drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
> drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
>   struct net_device *net_dev = efx->net_dev;
>                      ^~~~~~~
> 
> The variable is still used in an #ifdef. Replace the #ifdef with
> an if(IS_ENABLED()) check that lets the compiler see where it is
> used, rather than adding another #ifdef.

So we've had Leon telling us[1] to use __maybe_unused, and you're
 saying to use IS_ENABLED() instead.  Which is right?
(And does it make any difference to build time?  I'm assuming the
 compiler is smart enough that this change doesn't affect text
 size...?)
-ed

[1]: https://lore.kernel.org/netdev/cac3fa89-50a3-6de0-796c-a215400f3710@intel.com/T/#md2ecc82f18c200391dc6581ff68ff08eee9a65cf

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
  2023-02-17 16:13 ` Edward Cree
@ 2023-02-17 16:19   ` Arnd Bergmann
  2023-02-19 13:31     ` Simon Horman
  2023-02-19  9:31   ` Leon Romanovsky
  1 sibling, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2023-02-17 16:19 UTC (permalink / raw)
  To: Edward Cree, Arnd Bergmann, Martin Habets, David S . Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alejandro Lucero
  Cc: Jonathan Cooper, Netdev, linux-kernel, Leon Romanovsky

On Fri, Feb 17, 2023, at 17:13, Edward Cree wrote:
> On 17/02/2023 09:56, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> One local variable has become unused after a recent change:
>> 
>> drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
>> drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
>>   struct net_device *net_dev = efx->net_dev;
>>                      ^~~~~~~
>> 
>> The variable is still used in an #ifdef. Replace the #ifdef with
>> an if(IS_ENABLED()) check that lets the compiler see where it is
>> used, rather than adding another #ifdef.
>
> So we've had Leon telling us[1] to use __maybe_unused, and you're
>  saying to use IS_ENABLED() instead.  Which is right?
> (And does it make any difference to build time?  I'm assuming the
>  compiler is smart enough that this change doesn't affect text
>  size...?)
> -ed

Both are correct, but I prefer the IS_ENABLED() change because it
improves build coverage. The resulting object code should be the
same, as the dead-code-elimination in gcc takes care of removing
it the same way.

If you use the __maybe_uninitialized annotation, you still need
an extra fix to initialize the ef100_probe_netdev_pf() return
code.

      Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
  2023-02-17 16:13 ` Edward Cree
  2023-02-17 16:19   ` Arnd Bergmann
@ 2023-02-19  9:31   ` Leon Romanovsky
  1 sibling, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2023-02-19  9:31 UTC (permalink / raw)
  To: Edward Cree
  Cc: Arnd Bergmann, Martin Habets, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Alejandro Lucero, Arnd Bergmann,
	Jonathan Cooper, netdev, linux-kernel

On Fri, Feb 17, 2023 at 04:13:59PM +0000, Edward Cree wrote:
> On 17/02/2023 09:56, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > One local variable has become unused after a recent change:
> > 
> > drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
> > drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
> >   struct net_device *net_dev = efx->net_dev;
> >                      ^~~~~~~
> > 
> > The variable is still used in an #ifdef. Replace the #ifdef with
> > an if(IS_ENABLED()) check that lets the compiler see where it is
> > used, rather than adding another #ifdef.
> 
> So we've had Leon telling us[1] to use __maybe_unused, and you're
>  saying to use IS_ENABLED() instead.  Which is right?
> (And does it make any difference to build time?  I'm assuming the
>  compiler is smart enough that this change doesn't affect text
>  size...?)

You are mixing answers, __maybe_unused is for variables. For functions,
it will be much saner to create empty declarations for relevant
functions in tc.h, for !CONFIG_SFC_SRIOV flow.

It will be much cleaner than spaghetti code in .c files which you have now.

Thanks

> -ed
> 
> [1]: https://lore.kernel.org/netdev/cac3fa89-50a3-6de0-796c-a215400f3710@intel.com/T/#md2ecc82f18c200391dc6581ff68ff08eee9a65cf

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
  2023-02-17 16:19   ` Arnd Bergmann
@ 2023-02-19 13:31     ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2023-02-19 13:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Edward Cree, Arnd Bergmann, Martin Habets, David S . Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alejandro Lucero,
	Jonathan Cooper, Netdev, linux-kernel, Leon Romanovsky

On Fri, Feb 17, 2023 at 05:19:26PM +0100, Arnd Bergmann wrote:
> On Fri, Feb 17, 2023, at 17:13, Edward Cree wrote:
> > On 17/02/2023 09:56, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> One local variable has become unused after a recent change:
> >> 
> >> drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
> >> drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
> >>   struct net_device *net_dev = efx->net_dev;
> >>                      ^~~~~~~
> >> 
> >> The variable is still used in an #ifdef. Replace the #ifdef with
> >> an if(IS_ENABLED()) check that lets the compiler see where it is
> >> used, rather than adding another #ifdef.
> >
> > So we've had Leon telling us[1] to use __maybe_unused, and you're
> >  saying to use IS_ENABLED() instead.  Which is right?
> > (And does it make any difference to build time?  I'm assuming the
> >  compiler is smart enough that this change doesn't affect text
> >  size...?)
> > -ed
> 
> Both are correct, but I prefer the IS_ENABLED() change because it
> improves build coverage. The resulting object code should be the
> same, as the dead-code-elimination in gcc takes care of removing
> it the same way.
> 
> If you use the __maybe_uninitialized annotation, you still need
> an extra fix to initialize the ef100_probe_netdev_pf() return
> code.

FWIIW, IS_ENABLED() is the approach that is more familiar to me.
Though I have nothing in particular against other approaches.

Questions of consistency aside, this patch does look good to
me and does appear to address the build problem in question - on x86_64.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
  2023-02-17  9:56 [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV Arnd Bergmann
  2023-02-17 16:13 ` Edward Cree
@ 2023-02-20 10:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-02-20 10:50 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: ecree.xilinx, habetsm.xilinx, davem, edumazet, kuba, pabeni,
	alejandro.lucero-palau, arnd, jonathan.s.cooper, netdev,
	linux-kernel

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 17 Feb 2023 10:56:39 +0100 you wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> One local variable has become unused after a recent change:
> 
> drivers/net/ethernet/sfc/ef100_nic.c: In function 'ef100_probe_netdev_pf':
> drivers/net/ethernet/sfc/ef100_nic.c:1155:21: error: unused variable 'net_dev' [-Werror=unused-variable]
>   struct net_device *net_dev = efx->net_dev;
>                      ^~~~~~~
> 
> [...]

Here is the summary with links:
  - sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV
    https://git.kernel.org/netdev/net-next/c/a59f832a71c9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-20 10:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17  9:56 [PATCH] sfc: use IS_ENABLED() checks for CONFIG_SFC_SRIOV Arnd Bergmann
2023-02-17 16:13 ` Edward Cree
2023-02-17 16:19   ` Arnd Bergmann
2023-02-19 13:31     ` Simon Horman
2023-02-19  9:31   ` Leon Romanovsky
2023-02-20 10:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).