[PATCH net-next 0/4] sfc: clean up some W=1 build warnings

[PATCH net-next 0/4] sfc: clean up some W=1 build warnings

[Edward Cree]

A collection of minor fixes to issues flagged up by W=1.
After this series, the only remaining warnings in the sfc driver are
 some 'member missing in kerneldoc' warnings from ptp.c.
Tested by building on x86_64 and running 'ethtool -p' on an EF10 NIC;
 there was no error, but I couldn't observe the actual LED as I'm
 working remotely.

[ Incidentally, ethtool_phys_id()'s behaviour on an error return
  looks strange — if I'm reading it right, it will break out of the
  inner loop but not the outer one, and eventually return the rc
  from the last run of the inner loop.  Is this intended? ]

Edward Cree (4):
  sfc: fix W=1 warnings in efx_farch_handle_rx_not_ok
  sfc: fix unused-but-set-variable warning in
    efx_farch_filter_remove_safe
  sfc: fix kernel-doc on struct efx_loopback_state
  sfc: return errors from efx_mcdi_set_id_led, and de-indirect

 drivers/net/ethernet/sfc/ef10.c       | 2 --
 drivers/net/ethernet/sfc/ethtool.c    | 3 +--
 drivers/net/ethernet/sfc/farch.c      | 9 ++-------
 drivers/net/ethernet/sfc/mcdi.c       | 6 ++----
 drivers/net/ethernet/sfc/mcdi.h       | 2 +-
 drivers/net/ethernet/sfc/net_driver.h | 2 --
 drivers/net/ethernet/sfc/selftest.c   | 2 +-
 drivers/net/ethernet/sfc/siena.c      | 1 -
 8 files changed, 7 insertions(+), 20 deletions(-)

[PATCH net-next 1/4] sfc: fix W=1 warnings in efx_farch_handle_rx_not_ok

[Edward Cree]

Some of these RX-event flags aren't used at all, so remove them.
Others are used only #ifdef DEBUG to log a message; suppress the
 unused-var warnings #ifndef DEBUG with a void cast.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/farch.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/sfc/farch.c b/drivers/net/ethernet/sfc/farch.c
index d07eeaad9bdf..aff2974e66df 100644
--- a/drivers/net/ethernet/sfc/farch.c
+++ b/drivers/net/ethernet/sfc/farch.c
@@ -863,13 +863,8 @@ static u16 efx_farch_handle_rx_not_ok(struct efx_rx_queue *rx_queue,
 	bool rx_ev_tcp_udp_chksum_err, rx_ev_eth_crc_err;
 	bool rx_ev_frm_trunc, rx_ev_tobe_disc;
 	bool rx_ev_other_err, rx_ev_pause_frm;
-	bool rx_ev_hdr_type, rx_ev_mcast_pkt;
-	unsigned rx_ev_pkt_type;
 
-	rx_ev_hdr_type = EFX_QWORD_FIELD(*event, FSF_AZ_RX_EV_HDR_TYPE);
-	rx_ev_mcast_pkt = EFX_QWORD_FIELD(*event, FSF_AZ_RX_EV_MCAST_PKT);
 	rx_ev_tobe_disc = EFX_QWORD_FIELD(*event, FSF_AZ_RX_EV_TOBE_DISC);
-	rx_ev_pkt_type = EFX_QWORD_FIELD(*event, FSF_AZ_RX_EV_PKT_TYPE);
 	rx_ev_buf_owner_id_err = EFX_QWORD_FIELD(*event,
 						 FSF_AZ_RX_EV_BUF_OWNER_ID_ERR);
 	rx_ev_ip_hdr_chksum_err = EFX_QWORD_FIELD(*event,
@@ -918,6 +913,8 @@ static u16 efx_farch_handle_rx_not_ok(struct efx_rx_queue *rx_queue,
 			  rx_ev_tobe_disc ? " [TOBE_DISC]" : "",
 			  rx_ev_pause_frm ? " [PAUSE]" : "");
 	}
+#else
+	(void) rx_ev_other_err;
 #endif
 
 	if (efx->net_dev->features & NETIF_F_RXALL)

[PATCH net-next 2/4] sfc: fix unused-but-set-variable warning in efx_farch_filter_remove_safe

[Edward Cree]

Thanks to some past refactor, 'spec' is not actually used in this
 function; the code using it moved to the callee efx_farch_filter_remove.
Remove the variable to fix a W=1 warning.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/farch.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/farch.c b/drivers/net/ethernet/sfc/farch.c
index aff2974e66df..0d9795fb9356 100644
--- a/drivers/net/ethernet/sfc/farch.c
+++ b/drivers/net/ethernet/sfc/farch.c
@@ -2589,7 +2589,6 @@ int efx_farch_filter_remove_safe(struct efx_nic *efx,
 	enum efx_farch_filter_table_id table_id;
 	struct efx_farch_filter_table *table;
 	unsigned int filter_idx;
-	struct efx_farch_filter_spec *spec;
 	int rc;
 
 	table_id = efx_farch_filter_id_table_id(filter_id);
@@ -2601,7 +2600,6 @@ int efx_farch_filter_remove_safe(struct efx_nic *efx,
 	if (filter_idx >= table->size)
 		return -ENOENT;
 	down_write(&state->lock);
-	spec = &table->spec[filter_idx];
 
 	rc = efx_farch_filter_remove(efx, table, filter_idx, priority);
 	up_write(&state->lock);

[PATCH net-next 3/4] sfc: fix kernel-doc on struct efx_loopback_state

[Edward Cree]

Missing 'struct' keyword caused "cannot understand function prototype"
 warnings.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/selftest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c
index e71d6d37a317..34b9c7d50c4e 100644
--- a/drivers/net/ethernet/sfc/selftest.c
+++ b/drivers/net/ethernet/sfc/selftest.c
@@ -67,7 +67,7 @@ static const char *const efx_interrupt_mode_names[] = {
 	STRING_TABLE_LOOKUP(efx->interrupt_mode, efx_interrupt_mode)
 
 /**
- * efx_loopback_state - persistent state during a loopback selftest
+ * struct efx_loopback_state - persistent state during a loopback selftest
  * @flush:		Drop all packets in efx_loopback_rx_packet
  * @packet_count:	Number of packets being used in this test
  * @skbs:		An array of skbs transmitted

[PATCH net-next 4/4] sfc: return errors from efx_mcdi_set_id_led, and de-indirect

[Edward Cree]

W=1 warnings indicated that 'rc' was unused in efx_mcdi_set_id_led();
 change the function to return int instead of void and plumb the rc
 through the caller efx_ethtool_phys_id().
Since (post-Falcon) all sfc NICs use MCDI for this, there's no point in
 indirecting through a nic_type method, so remove that and just call
 efx_mcdi_set_id_led() directly.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/ef10.c       | 2 --
 drivers/net/ethernet/sfc/ethtool.c    | 3 +--
 drivers/net/ethernet/sfc/mcdi.c       | 6 ++----
 drivers/net/ethernet/sfc/mcdi.h       | 2 +-
 drivers/net/ethernet/sfc/net_driver.h | 2 --
 drivers/net/ethernet/sfc/siena.c      | 1 -
 6 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 4b0b2cf026a5..0b4bcac53f18 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -3955,7 +3955,6 @@ const struct efx_nic_type efx_hunt_a0_vf_nic_type = {
 	.start_stats = efx_port_dummy_op_void,
 	.pull_stats = efx_port_dummy_op_void,
 	.stop_stats = efx_port_dummy_op_void,
-	.set_id_led = efx_mcdi_set_id_led,
 	.push_irq_moderation = efx_ef10_push_irq_moderation,
 	.reconfigure_mac = efx_ef10_mac_reconfigure,
 	.check_mac_fault = efx_mcdi_mac_check_fault,
@@ -4066,7 +4065,6 @@ const struct efx_nic_type efx_hunt_a0_nic_type = {
 	.start_stats = efx_mcdi_mac_start_stats,
 	.pull_stats = efx_mcdi_mac_pull_stats,
 	.stop_stats = efx_mcdi_mac_stop_stats,
-	.set_id_led = efx_mcdi_set_id_led,
 	.push_irq_moderation = efx_ef10_push_irq_moderation,
 	.reconfigure_mac = efx_ef10_mac_reconfigure,
 	.check_mac_fault = efx_mcdi_mac_check_fault,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 4ffda7782f68..12a91c559aa2 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -50,8 +50,7 @@ static int efx_ethtool_phys_id(struct net_device *net_dev,
 		return 1;	/* cycle on/off once per second */
 	}
 
-	efx->type->set_id_led(efx, mode);
-	return 0;
+	return efx_mcdi_set_id_led(efx, mode);
 }
 
 static int efx_ethtool_get_regs_len(struct net_device *net_dev)
diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c
index 5467819aef6e..be6bfd6b7ec7 100644
--- a/drivers/net/ethernet/sfc/mcdi.c
+++ b/drivers/net/ethernet/sfc/mcdi.c
@@ -1868,10 +1868,9 @@ int efx_mcdi_handle_assertion(struct efx_nic *efx)
 	return efx_mcdi_exit_assertion(efx);
 }
 
-void efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode)
+int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_SET_ID_LED_IN_LEN);
-	int rc;
 
 	BUILD_BUG_ON(EFX_LED_OFF != MC_CMD_LED_OFF);
 	BUILD_BUG_ON(EFX_LED_ON != MC_CMD_LED_ON);
@@ -1881,8 +1880,7 @@ void efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode)
 
 	MCDI_SET_DWORD(inbuf, SET_ID_LED_IN_STATE, mode);
 
-	rc = efx_mcdi_rpc(efx, MC_CMD_SET_ID_LED, inbuf, sizeof(inbuf),
-			  NULL, 0, NULL);
+	return efx_mcdi_rpc(efx, MC_CMD_SET_ID_LED, inbuf, sizeof(inbuf), NULL, 0, NULL);
 }
 
 static int efx_mcdi_reset_func(struct efx_nic *efx)
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index 658cf345420d..8aed65018964 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -348,7 +348,7 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type,
 int efx_new_mcdi_nvram_test_all(struct efx_nic *efx);
 int efx_mcdi_nvram_test_all(struct efx_nic *efx);
 int efx_mcdi_handle_assertion(struct efx_nic *efx);
-void efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode);
+int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode);
 int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac,
 				  int *id_out);
 int efx_mcdi_wol_filter_get_magic(struct efx_nic *efx, int *id_out);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 062462a13847..338ebb0402be 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1217,7 +1217,6 @@ struct efx_udp_tunnel {
  * @start_stats: Start the regular fetching of statistics
  * @pull_stats: Pull stats from the NIC and wait until they arrive.
  * @stop_stats: Stop the regular fetching of statistics
- * @set_id_led: Set state of identifying LED or revert to automatic function
  * @push_irq_moderation: Apply interrupt moderation value
  * @reconfigure_port: Push loopback/power/txdis changes to the MAC and PHY
  * @prepare_enable_fc_tx: Prepare MAC to enable pause frame TX (may be %NULL)
@@ -1362,7 +1361,6 @@ struct efx_nic_type {
 	void (*start_stats)(struct efx_nic *efx);
 	void (*pull_stats)(struct efx_nic *efx);
 	void (*stop_stats)(struct efx_nic *efx);
-	void (*set_id_led)(struct efx_nic *efx, enum efx_led_mode mode);
 	void (*push_irq_moderation)(struct efx_channel *channel);
 	int (*reconfigure_port)(struct efx_nic *efx);
 	void (*prepare_enable_fc_tx)(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c
index a7ea630bb5e6..16347a6d0c47 100644
--- a/drivers/net/ethernet/sfc/siena.c
+++ b/drivers/net/ethernet/sfc/siena.c
@@ -994,7 +994,6 @@ const struct efx_nic_type siena_a0_nic_type = {
 	.start_stats = efx_mcdi_mac_start_stats,
 	.pull_stats = efx_mcdi_mac_pull_stats,
 	.stop_stats = efx_mcdi_mac_stop_stats,
-	.set_id_led = efx_mcdi_set_id_led,
 	.push_irq_moderation = siena_push_irq_moderation,
 	.reconfigure_mac = siena_mac_reconfigure,
 	.check_mac_fault = efx_mcdi_mac_check_fault,

Re: [PATCH net-next 0/4] sfc: clean up some W=1 build warnings

[Jakub Kicinski]

On Fri, 28 Aug 2020 18:48:25 +0100 Edward Cree wrote:
> A collection of minor fixes to issues flagged up by W=1.
> After this series, the only remaining warnings in the sfc driver are
>  some 'member missing in kerneldoc' warnings from ptp.c.
> Tested by building on x86_64 and running 'ethtool -p' on an EF10 NIC;
>  there was no error, but I couldn't observe the actual LED as I'm
>  working remotely.

LGTM, although borderline net if you ask me.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

> [ Incidentally, ethtool_phys_id()'s behaviour on an error return
>   looks strange — if I'm reading it right, it will break out of the
>   inner loop but not the outer one, and eventually return the rc
>   from the last run of the inner loop.  Is this intended? ]

I think you're right. Care to fix?

Re: [PATCH net-next 0/4] sfc: clean up some W=1 build warnings

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Fri, 28 Aug 2020 18:48:25 +0100

> A collection of minor fixes to issues flagged up by W=1.
> After this series, the only remaining warnings in the sfc driver are
>  some 'member missing in kerneldoc' warnings from ptp.c.
> Tested by building on x86_64 and running 'ethtool -p' on an EF10 NIC;
>  there was no error, but I couldn't observe the actual LED as I'm
>  working remotely.
> 
> [ Incidentally, ethtool_phys_id()'s behaviour on an error return
>   looks strange ― if I'm reading it right, it will break out of the
>   inner loop but not the outer one, and eventually return the rc
>   from the last run of the inner loop.  Is this intended? ]

Series applied, thanks.