Re: [PATCH net-next] efx: convert to new udp_tunnel infrastructure

[Edward Cree <ecree@solarflare.com>]

Subject line prefix should probably be sfc:, that's what we
 usually use for this driver.

On 18/07/2020 00:53, Jakub Kicinski wrote:
> Check MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_TRUSTED, before setting
> the info, which will hopefully protect us from -EPERM errors
Sorry, I forgot to pass on the answer I got from the firmware
 team, which was "TRUSTED is the wrong thing and there isn't a
 right thing".  The TRUSTED flag will be set on any function
 that can set UDP ports, but may also be set on some that can't
 (it's not clear what they're Trusted to do but this isn't it).
So it's OK to check it, but the EPERMs could still happen.
> the previous code was gracefully ignoring. Shared code reports
> the port information back to user space, so we really want
> to know what was added and what failed.
<snip>
> -static int efx_ef10_udp_tnl_add_port(struct efx_nic *efx,
> -				     struct efx_udp_tunnel tnl)
> -{
> -	struct efx_ef10_nic_data *nic_data = efx->nic_data;
> -	struct efx_udp_tunnel *match;
> -	char typebuf[8];
> -	size_t i;
> -	int rc;
> +	if (ti->type == UDP_TUNNEL_TYPE_VXLAN)
> +		efx_tunnel_type = TUNNEL_ENCAP_UDP_PORT_ENTRY_VXLAN;
> +	else
> +		efx_tunnel_type = TUNNEL_ENCAP_UDP_PORT_ENTRY_GENEVE;
I think I'd prefer to keep the switch() that explicitlychecks
 for UDP_TUNNEL_TYPE_GENEVE; even though the infrastructure
 makes sure it won't ever not be, I'd still feel more comfortable
 that way.  But it's up to you.
> @@ -3873,6 +3835,7 @@ static int efx_ef10_udp_tnl_add_port(struct efx_nic *efx,
>  static bool efx_ef10_udp_tnl_has_port(struct efx_nic *efx, __be16 port)
>  {
>  	struct efx_ef10_nic_data *nic_data = efx->nic_data;
> +	size_t i;
>  
>  	if (!(nic_data->datapath_caps &
>  	      (1 << MC_CMD_GET_CAPABILITIES_OUT_VXLAN_NVGRE_LBN)))
> @@ -3884,58 +3847,48 @@ static bool efx_ef10_udp_tnl_has_port(struct efx_nic *efx, __be16 port)
>  		 */
>  		return false;
>  
> -	return __efx_ef10_udp_tnl_lookup_port(efx, port) != NULL;
> +	for (i = 0; i < ARRAY_SIZE(nic_data->udp_tunnels); ++i)
> +		if (nic_data->udp_tunnels[i].port == port)
> +			return true;
> +
> +	return false;
>  }
>  
> -static int efx_ef10_udp_tnl_del_port(struct efx_nic *efx,
> -				     struct efx_udp_tunnel tnl)
> +static int efx_ef10_udp_tnl_unset_port(struct net_device *dev,
> +				       unsigned int table, unsigned int entry,
> +				       struct udp_tunnel_info *ti)
>  {
> -	struct efx_ef10_nic_data *nic_data = efx->nic_data;
> -	struct efx_udp_tunnel *match;
> -	char typebuf[8];
> +	struct efx_nic *efx = netdev_priv(dev);
> +	struct efx_ef10_nic_data *nic_data;
>  	int rc;
>  
> -	if (!(nic_data->datapath_caps &
> -	      (1 << MC_CMD_GET_CAPABILITIES_OUT_VXLAN_NVGRE_LBN)))
> -		return 0;
> -
> -	efx_get_udp_tunnel_type_name(tnl.type, typebuf, sizeof(typebuf));
> -	netif_dbg(efx, drv, efx->net_dev, "Removing UDP tunnel (%s) port %d\n",
> -		  typebuf, ntohs(tnl.port));
> +	nic_data = efx->nic_data;
>  
>  	mutex_lock(&nic_data->udp_tunnels_lock);
>  	/* Make sure all TX are stopped while we remove from the table, else we
>  	 * might race against an efx_features_check().
>  	 */
>  	efx_device_detach_sync(efx);
> -
> -	match = __efx_ef10_udp_tnl_lookup_port(efx, tnl.port);
> -	if (match != NULL) {
> -		if (match->type == tnl.type) {
> -			if (--match->count) {
> -				/* Port is still in use, so nothing to do */
> -				netif_dbg(efx, drv, efx->net_dev,
> -					  "UDP tunnel port %d remains active\n",
> -					  ntohs(tnl.port));
> -				rc = 0;
> -				goto out_unlock;
> -			}
> -			rc = efx_ef10_set_udp_tnl_ports(efx, false);
> -			goto out_unlock;
> -		}
> -		efx_get_udp_tunnel_type_name(match->type,
> -					     typebuf, sizeof(typebuf));
> -		netif_warn(efx, drv, efx->net_dev,
> -			   "UDP port %d is actually in use by %s, not removing\n",
> -			   ntohs(tnl.port), typebuf);
> -	}
> -	rc = -ENOENT;
> -
> -out_unlock:
> +	nic_data->udp_tunnels[entry].port = 0;
I'm a little concerned that efx_ef10_udp_tnl_has_port(efx, 0);
 willgenerally return true, so in our yet-to-come TX offloads
 patch we'll need to check for !port when handling an skb with
 skb->encapsulation == true before calling has_port.
(I realise that the kernel almost certainly won't ever give us
 an skb with encap on UDP port 0, but it never hurts to be
 paranoid about things like that).
Could we not keep a 'valid'/'used' flag in the table, used in
 roughly the same way we were checking count != 0?

Apart from that this all looks fine, and the amount of deleted
 codemakes me happy :)

-ed