On Wed, Jul 8, 2020 at 2:25 PM Jakub Kicinski <k...@kernel.org> wrote:
>
> 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.
That would work too.
> > >
> > > - 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.
Okay, sounds good.
> > > #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
Thanks, this does appear to do quite a bit to simplify the driver code.
- Alex
