On Wed, 8 Jul 2020 10:00:36 -0700 Alexander Duyck wrote:
> On Tue, Jul 7, 2020 at 2:28 PM Jakub Kicinski <k...@kernel.org> wrote:
> >
> > Make use of new common udp_tunnel_nic infra. ixgbe supports
> > IPv4 only, and only single VxLAN and Geneve ports (one each).
> >
> > I'm dropping the confusing piece of code in ixgbe_set_features().
> > ndo_udp_tunnel_add and ndo_udp_tunnel_del did not check if RXCSUM
> > is enabled, so this code was either unnecessary or buggy anyway.
>
> The code is unnecessary from what I can tell. I suspect the reason for
> adding the code was because the port values are used when performing
> the Rx checksum offload. However we never disable it in hardware, and
> the software path in ixgbe_rx_checksum will simply disable the related
> code anyway since we cannot set skb->encapsulation if RXCSUM is
> disabled.
>
> With that said moving this to a separate patch might be preferable as
> it would make the patch more readable.
Ack on all points!
> > -static void ixgbe_clear_udp_tunnel_port(struct ixgbe_adapter *adapter, u32
> > mask)
> > +static int ixgbe_udp_tunnel_sync(struct net_device *dev, unsigned int
> > table)
> > {
> > + struct ixgbe_adapter *adapter = netdev_priv(dev);
> > struct ixgbe_hw *hw = &adapter->hw;
> > - u32 vxlanctrl;
> > -
> > - if (!(adapter->flags & (IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE |
> > - IXGBE_FLAG_GENEVE_OFFLOAD_CAPABLE)))
> > - return;
> > + struct udp_tunnel_info ti;
> >
> > - vxlanctrl = IXGBE_READ_REG(hw, IXGBE_VXLANCTRL) & ~mask;
> > - IXGBE_WRITE_REG(hw, IXGBE_VXLANCTRL, vxlanctrl);
> > -
> > - if (mask & IXGBE_VXLANCTRL_VXLAN_UDPPORT_MASK)
> > - adapter->vxlan_port = 0;
> > + udp_tunnel_nic_get_port(dev, table, 0, &ti);
> > + if (!table)
> > + adapter->vxlan_port = ti.port;
> > + else
> > + adapter->geneve_port = ti.port;
>
> So this !table thing is a bit hard to read. It might be more useful if
> you had a define that made it clear that the expectation is that entry
> 0 is a VXLAN table and entry 1 is the GENEVE.
How about:
if (ti.type == UDP_TUNNEL_TYPE_VXLAN)
Tunnel info will have the type.
> >
> > - if (mask & IXGBE_VXLANCTRL_GENEVE_UDPPORT_MASK)
> > - adapter->geneve_port = 0;
> > + IXGBE_WRITE_REG(hw, IXGBE_VXLANCTRL,
> > + ntohs(adapter->vxlan_port) |
> > + ntohs(adapter->geneve_port) <<
> > + IXGBE_VXLANCTRL_GENEVE_UDPPORT_SHIFT);
> > + return 0;
> > }
>
> I'm assuming the new logic will call this for all entries in the
> tables regardless of if they are set or not. If so I suppose this is
> fine.
Yup, I think this is preferred form of sync for devices which just
write registers. I wrote it for NFP initially but it seems to work
in more places.
The driver can query the entire table and will get either the port
or 0 if port is not set.
> > #ifdef CONFIG_IXGBE_DCB
> > /**
> > * ixgbe_configure_dcb - Configure DCB hardware
> > @@ -6227,6 +6242,7 @@ static void ixgbe_init_dcb(struct ixgbe_adapter
> > *adapter)
> > /**
> > * ixgbe_sw_init - Initialize general software structures (struct
> > ixgbe_adapter)
> > * @adapter: board private structure to initialize
> > + * @netdev: network interface device structure
> > * @ii: pointer to ixgbe_info for device
> > *
> > * ixgbe_sw_init initializes the Adapter private data structure.
> > @@ -6234,6 +6250,7 @@ static void ixgbe_init_dcb(struct ixgbe_adapter
> > *adapter)
> > * OS network device settings (MTU size).
> > **/
> > static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
> > + struct net_device *netdev,
> > const struct ixgbe_info *ii)
> > {
> > struct ixgbe_hw *hw = &adapter->hw;
>
> There is no need to add the argument. It should be accessible via
> adapter->netdev, or for that matter you could pass the netdev and drop
> the adapter and just pull it out via netdev_priv since the two are all
> contained in the same structure anyway. Another option would be to
> just pull the logic out of this section and put it in a switch
> statement of its own in the probe function.
Hm, new switch section definitely looks best. I'll do that.
> > @@ -6332,7 +6349,7 @@ static int ixgbe_sw_init(struct ixgbe_adapter
> > *adapter,
> > adapter->flags2 |= IXGBE_FLAG2_TEMP_SENSOR_CAPABLE;
> > break;
> > case ixgbe_mac_x550em_a:
> > - adapter->flags |= IXGBE_FLAG_GENEVE_OFFLOAD_CAPABLE;
> > + netdev->udp_tunnel_nic_info = &ixgbe_udp_tunnels_x550em_a;
> > switch (hw->device_id) {
> > case IXGBE_DEV_ID_X550EM_A_1G_T:
> > case IXGBE_DEV_ID_X550EM_A_1G_T_L:
> > @@ -6359,7 +6376,8 @@ static int ixgbe_sw_init(struct ixgbe_adapter
> > *adapter,
> > #ifdef CONFIG_IXGBE_DCA
> > adapter->flags &= ~IXGBE_FLAG_DCA_CAPABLE;
> > #endif
> > - adapter->flags |= IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE;
> > + if (!netdev->udp_tunnel_nic_info)
> > + netdev->udp_tunnel_nic_info =
> > &ixgbe_udp_tunnels_x550;
> > break;
> > default:
> > break;
>
> Not a fan of the !udp_tunnel_nic_info check, but I understand you are
> having to do it because of the fall-through.
Yup.
> > @@ -6798,8 +6816,7 @@ int ixgbe_open(struct net_device *netdev)
> >
> > ixgbe_up_complete(adapter);
> >
> > - ixgbe_clear_udp_tunnel_port(adapter,
> > IXGBE_VXLANCTRL_ALL_UDPPORT_MASK);
> > - udp_tunnel_get_rx_info(netdev);
> > + udp_tunnel_nic_reset_ntf(netdev);
> >
> > return 0;
>
> So I went looking through the earlier patches for an explanation of
> udp_tunnel_nic_reset_ntf. It might be useful to have some doxygen
> comments with the declaration explaining what it does and when/why we
> should use it.
Looks like I hated the need for this one so much I forgot to document
it :S
I was hoping the core can issue all the callbacks just based on netdev
notifications, but it seems like most drivers have internal resets
procedures which require re-programming the ports :( Hence this helper
was born.
I'll document it like this:
* Called by the driver to inform the core that the entire UDP tunnel port
* state has been lost, usually due to device reset. Core will assume device
* forgot all the ports and issue .set_port and .sync_table callbacks as
* necessary.
Thanks for the review! I'll post v2 in a couple hours hoping someone
else will find time to review :S
