| Message ID | db235d46-96b0-ee6d-f09b-774e6fd9a072@solarflare.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | sfc: prerequisites for EF100 driver, part 3 | expand |
On Wed, 1 Jul 2020 15:51:25 +0100 Edward Cree wrote: > Unprivileged functions (such as VFs) may set their MTU by use of the > 'control' field of MC_CMD_SET_MAC_EXT, as used in efx_mcdi_set_mtu(). > If calling efx_ef10_mac_reconfigure() from efx_change_mtu(), the NIC > supports the above (SET_MAC_ENHANCED capability), and regular > efx_mcdi_set_mac() fails EPERM, then fall back to efx_mcdi_set_mtu(). Is there no way of checking the permission the function has before issuing the firmware call?
On 01/07/2020 20:03, Jakub Kicinski wrote: > On Wed, 1 Jul 2020 15:51:25 +0100 Edward Cree wrote: >> Unprivileged functions (such as VFs) may set their MTU by use of the >> 'control' field of MC_CMD_SET_MAC_EXT, as used in efx_mcdi_set_mtu(). >> If calling efx_ef10_mac_reconfigure() from efx_change_mtu(), the NIC >> supports the above (SET_MAC_ENHANCED capability), and regular >> efx_mcdi_set_mac() fails EPERM, then fall back to efx_mcdi_set_mtu(). > Is there no way of checking the permission the function has before > issuing the firmware call? We could condition on the LINKCTRL flag from the MC_CMD_DRV_ATTACH response we get at start of day; but usually in this driver we've tried to follow the EAFP principle rather than embedding knowledge of the firmware's permissions model into the driver. I suppose it might make sense to go straight to efx_mcdi_set_mtu() in the mtu_only && SET_MAC_ENHANCED case, use efx_mcdi_set_mac() otherwise, and thus never have a fallback from one to the other. WDYT? -ed
On Wed, 1 Jul 2020 23:13:13 +0100 Edward Cree wrote: > On 01/07/2020 20:03, Jakub Kicinski wrote: > > On Wed, 1 Jul 2020 15:51:25 +0100 Edward Cree wrote: > >> Unprivileged functions (such as VFs) may set their MTU by use of the > >> 'control' field of MC_CMD_SET_MAC_EXT, as used in efx_mcdi_set_mtu(). > >> If calling efx_ef10_mac_reconfigure() from efx_change_mtu(), the NIC > >> supports the above (SET_MAC_ENHANCED capability), and regular > >> efx_mcdi_set_mac() fails EPERM, then fall back to efx_mcdi_set_mtu(). > > Is there no way of checking the permission the function has before > > issuing the firmware call? > We could condition on the LINKCTRL flag from the MC_CMD_DRV_ATTACH > response we get at start of day; but usually in this driver we've > tried to follow the EAFP principle rather than embedding knowledge > of the firmware's permissions model into the driver. I see. I'm actually asking because of efx_ef10_set_udp_tnl_ports(). I'd like to rewrite the UDP tunnel code so that NETIF_F_RX_UDP_TUNNEL_PORT only appears on the interface if it _really_ can do the offload. ef10 is the only driver I've seen where I can't be sure what FW will say.. (other than liquidio, but that's more of a kernel<->FW proxy than a driver, sigh). Is there anything I can condition on there? > I suppose it might make sense to go straight to efx_mcdi_set_mtu() > in the mtu_only && SET_MAC_ENHANCED case, use efx_mcdi_set_mac() > otherwise, and thus never have a fallback from one to the other. > WDYT? For the change of MTU that indeed seems much cleaner.
On 02/07/2020 00:16, Jakub Kicinski wrote: > I see. I'm actually asking because of efx_ef10_set_udp_tnl_ports(). > I'd like to rewrite the UDP tunnel code so that > NETIF_F_RX_UDP_TUNNEL_PORT only appears on the interface if it > _really_ can do the offload. ef10 is the only driver I've seen where > I can't be sure what FW will say.. (other than liquidio, but that's > more of a kernel<->FW proxy than a driver, sigh). I suppose it's time for me to describe the gory details ofhow EF10 tunnel offloads work, then. When changing the list of UDP tunnel ports, the MC patches the header-recogniser firmware and reboots the datapath CPUs (packet parsing on EF10 is done on a pair of DPCPUs with specialised instruction set extensions, and it wasn't originally designed with tunnel offloads in mind). Unfortunately, to synchronise everything afterwards, the MC then has to reboot itself too. Needless to say, this is fairly disruptive, especially as it's global across _both_ ports of a two-port NIC (which is what that EIO check in efx_ef10_set_udp_tnl_ports() is about: when you bring up a tunnel device, both netdevs get the ndo callback, the first one offloads the port, causing a reboot, which interrupts the second port's MCDI. Once the MC comes back up, the second netdev tries again to offload the UDP port, and this time the MC only has to increase a refcount, since the first netdev already added that port, so it can return success without another reboot). > Is there anything I can condition on there? I _believe_ this is determined by another drv_attach flag, MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_TRUSTED, but again I'm not 100% sure; it might be the PRIMARY flag instead. I've CCed Matthew Slattery from our firmware team who should be able to answer. It also depends on the VXLAN_NVGRE firmware capability, which efx_ef10_set_udp_tnl_ports() already checks. Note that firmware capabilities can also change on an MC reboot (see the code that handles nic_data->must_check_datapath_caps). -ed
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 5faf2f0e4d62..d162df382dc5 100644 --- a/drivers/net/ethernet/sfc/ef10.c +++ b/drivers/net/ethernet/sfc/ef10.c @@ -3306,18 +3306,18 @@ static int efx_ef10_set_mac_address(struct efx_nic *efx) return rc; } -static int efx_ef10_mac_reconfigure(struct efx_nic *efx) +static int efx_ef10_mac_reconfigure(struct efx_nic *efx, bool mtu_only) { - efx_mcdi_filter_sync_rx_mode(efx); + int rc; - return efx_mcdi_set_mac(efx); -} + WARN_ON(!mutex_is_locked(&efx->mac_lock)); -static int efx_ef10_mac_reconfigure_vf(struct efx_nic *efx) -{ efx_mcdi_filter_sync_rx_mode(efx); - return 0; + rc = efx_mcdi_set_mac(efx); + if (rc == -EPERM && mtu_only && efx_has_cap(efx, SET_MAC_ENHANCED)) + return efx_mcdi_set_mtu(efx); + return rc; } static int efx_ef10_start_bist(struct efx_nic *efx, u32 bist_type) @@ -4038,7 +4038,7 @@ const struct efx_nic_type efx_hunt_a0_vf_nic_type = { .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_vf, + .reconfigure_mac = efx_ef10_mac_reconfigure, .check_mac_fault = efx_mcdi_mac_check_fault, .reconfigure_port = efx_mcdi_port_reconfigure, .get_wol = efx_ef10_get_wol_vf, diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index 028d826ab147..418676aba43a 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c @@ -169,10 +169,6 @@ static int efx_init_port(struct efx_nic *efx) efx->port_initialized = true; - /* Reconfigure the MAC before creating dma queues (required for - * Falcon/A1 where RX_INGR_EN/TX_DRAIN_EN isn't supported) */ - efx_mac_reconfigure(efx); - /* Ensure the PHY advertises the correct flow control settings */ rc = efx->phy_op->reconfigure(efx); if (rc && rc != -EPERM) diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c index a2f744377aaa..26dca2f9a363 100644 --- a/drivers/net/ethernet/sfc/efx_common.c +++ b/drivers/net/ethernet/sfc/efx_common.c @@ -139,11 +139,11 @@ void efx_destroy_reset_workqueue(void) /* We assume that efx->type->reconfigure_mac will always try to sync RX * filters and therefore needs to read-lock the filter table against freeing */ -void efx_mac_reconfigure(struct efx_nic *efx) +void efx_mac_reconfigure(struct efx_nic *efx, bool mtu_only) { if (efx->type->reconfigure_mac) { down_read(&efx->filter_sem); - efx->type->reconfigure_mac(efx); + efx->type->reconfigure_mac(efx, mtu_only); up_read(&efx->filter_sem); } } @@ -158,7 +158,7 @@ static void efx_mac_work(struct work_struct *data) mutex_lock(&efx->mac_lock); if (efx->port_enabled) - efx_mac_reconfigure(efx); + efx_mac_reconfigure(efx, false); mutex_unlock(&efx->mac_lock); } @@ -190,7 +190,7 @@ int efx_set_mac_address(struct net_device *net_dev, void *data) /* Reconfigure the MAC */ mutex_lock(&efx->mac_lock); - efx_mac_reconfigure(efx); + efx_mac_reconfigure(efx, false); mutex_unlock(&efx->mac_lock); return 0; @@ -304,7 +304,7 @@ int efx_change_mtu(struct net_device *net_dev, int new_mtu) mutex_lock(&efx->mac_lock); net_dev->mtu = new_mtu; - efx_mac_reconfigure(efx); + efx_mac_reconfigure(efx, true); mutex_unlock(&efx->mac_lock); efx_start_all(efx); @@ -486,7 +486,7 @@ static void efx_start_port(struct efx_nic *efx) efx->port_enabled = true; /* Ensure MAC ingress/egress is enabled */ - efx_mac_reconfigure(efx); + efx_mac_reconfigure(efx, false); mutex_unlock(&efx->mac_lock); } diff --git a/drivers/net/ethernet/sfc/efx_common.h b/drivers/net/ethernet/sfc/efx_common.h index 73c355fc2590..4056f68f04e5 100644 --- a/drivers/net/ethernet/sfc/efx_common.h +++ b/drivers/net/ethernet/sfc/efx_common.h @@ -95,7 +95,7 @@ static inline void efx_init_mcdi_logging(struct efx_nic *efx) {} static inline void efx_fini_mcdi_logging(struct efx_nic *efx) {} #endif -void efx_mac_reconfigure(struct efx_nic *efx); +void efx_mac_reconfigure(struct efx_nic *efx, bool mtu_only); int efx_set_mac_address(struct net_device *net_dev, void *data); void efx_set_rx_mode(struct net_device *net_dev); int efx_set_features(struct net_device *net_dev, netdev_features_t data); diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c index c96595e50234..738d9be86899 100644 --- a/drivers/net/ethernet/sfc/ethtool_common.c +++ b/drivers/net/ethernet/sfc/ethtool_common.c @@ -241,7 +241,7 @@ int efx_ethtool_set_pauseparam(struct net_device *net_dev, /* Reconfigure the MAC. The PHY *may* generate a link state change event * if the user just changed the advertised capabilities, but there's no * harm doing this twice */ - efx_mac_reconfigure(efx); + efx_mac_reconfigure(efx, false); out: mutex_unlock(&efx->mac_lock); diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index e0b84b2e3bd2..65a106878186 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -1356,7 +1356,7 @@ struct efx_nic_type { void (*push_irq_moderation)(struct efx_channel *channel); int (*reconfigure_port)(struct efx_nic *efx); void (*prepare_enable_fc_tx)(struct efx_nic *efx); - int (*reconfigure_mac)(struct efx_nic *efx); + int (*reconfigure_mac)(struct efx_nic *efx, bool mtu_only); bool (*check_mac_fault)(struct efx_nic *efx); void (*get_wol)(struct efx_nic *efx, struct ethtool_wolinfo *wol); int (*set_wol)(struct efx_nic *efx, u32 type); diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c index ffe193f03352..a7dcd2d3c09b 100644 --- a/drivers/net/ethernet/sfc/siena.c +++ b/drivers/net/ethernet/sfc/siena.c @@ -633,7 +633,7 @@ static size_t siena_update_nic_stats(struct efx_nic *efx, u64 *full_stats, return SIENA_STAT_COUNT; } -static int siena_mac_reconfigure(struct efx_nic *efx) +static int siena_mac_reconfigure(struct efx_nic *efx, bool mtu_only __always_unused) { MCDI_DECLARE_BUF(inbuf, MC_CMD_SET_MCAST_HASH_IN_LEN); int rc;
Unprivileged functions (such as VFs) may set their MTU by use of the 'control' field of MC_CMD_SET_MAC_EXT, as used in efx_mcdi_set_mtu(). If calling efx_ef10_mac_reconfigure() from efx_change_mtu(), the NIC supports the above (SET_MAC_ENHANCED capability), and regular efx_mcdi_set_mac() fails EPERM, then fall back to efx_mcdi_set_mtu(). Signed-off-by: Edward Cree <ecree@solarflare.com> --- drivers/net/ethernet/sfc/ef10.c | 16 ++++++++-------- drivers/net/ethernet/sfc/efx.c | 4 ---- drivers/net/ethernet/sfc/efx_common.c | 12 ++++++------ drivers/net/ethernet/sfc/efx_common.h | 2 +- drivers/net/ethernet/sfc/ethtool_common.c | 2 +- drivers/net/ethernet/sfc/net_driver.h | 2 +- drivers/net/ethernet/sfc/siena.c | 2 +- 7 files changed, 18 insertions(+), 22 deletions(-)