Re: [Intel-wired-lan] [iwl-next v1 04/15] ice: add basic devlink subfunctions support

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Feb 13, 2024 at 03:57:59PM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 01:02:43PM CET, michal.swiatkowski@linux.intel.com wrote:
> >On Tue, Feb 13, 2024 at 12:27:20PM +0100, Jiri Pirko wrote:
> >> Tue, Feb 13, 2024 at 10:39:47AM CET, michal.swiatkowski@linux.intel.com wrote:
> >> >On Tue, Feb 13, 2024 at 09:55:20AM +0100, Jiri Pirko wrote:
> >> >> Tue, Feb 13, 2024 at 08:27:13AM CET, michal.swiatkowski@linux.intel.com wrote:
> >> >> >From: Piotr Raczynski <piotr.raczynski@intel.com>
> >> 
> >> [...]
> >> 
> >> 
> >> >
> >> >> 
> >> >> >+}
> >> >> >+
> >> >> >+/**
> >> >> >+ * ice_dealloc_dynamic_port - Deallocate and remove a dynamic port
> >> >> >+ * @dyn_port: dynamic port instance to deallocate
> >> >> >+ *
> >> >> >+ * Free resources associated with a dynamically added devlink port. Will
> >> >> >+ * deactivate the port if its currently active.
> >> >> >+ */
> >> >> >+static void ice_dealloc_dynamic_port(struct ice_dynamic_port *dyn_port)
> >> >> >+{
> >> >> >+	struct devlink_port *devlink_port = &dyn_port->devlink_port;
> >> >> >+	struct ice_pf *pf = dyn_port->pf;
> >> >> >+
> >> >> >+	if (dyn_port->active)
> >> >> >+		ice_deactivate_dynamic_port(dyn_port);
> >> >> >+
> >> >> >+	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_SF)
> >> >> 
> >> >> I don't understand how this check could be false. Remove it.
> >> >>
> >> >Yeah, will remove
> >> >
> >> >> 
> >> >> >+		xa_erase(&pf->sf_nums, devlink_port->attrs.pci_sf.sf);
> >> >> >+
> >> >> >+	devl_port_unregister(devlink_port);
> >> >> >+	ice_vsi_free(dyn_port->vsi);
> >> >> >+	xa_erase(&pf->dyn_ports, dyn_port->vsi->idx);
> >> >> >+	kfree(dyn_port);
> >> >> >+}
> >> >> >+
> >> >> >+/**
> >> >> >+ * ice_dealloc_all_dynamic_ports - Deallocate all dynamic devlink ports
> >> >> >+ * @pf: pointer to the pf structure
> >> >> >+ */
> >> >> >+void ice_dealloc_all_dynamic_ports(struct ice_pf *pf)
> >> >> >+{
> >> >> >+	struct devlink *devlink = priv_to_devlink(pf);
> >> >> >+	struct ice_dynamic_port *dyn_port;
> >> >> >+	unsigned long index;
> >> >> >+
> >> >> >+	devl_lock(devlink);
> >> >> >+	xa_for_each(&pf->dyn_ports, index, dyn_port)
> >> >> >+		ice_dealloc_dynamic_port(dyn_port);
> >> >> >+	devl_unlock(devlink);
> >> >> 
> >> >> Hmm, I would assume that the called should already hold the devlink
> >> >> instance lock when doing remove. What is stopping user from issuing
> >> >> port_new command here, after devl_unlock()?
> >> >>
> >> >It is only called from remove path, but I can move it upper.
> >> 
> >> I know it is called on remove path. Again, what is stopping user from
> >> issuing port_new after ice_dealloc_all_dynamic_ports() is called?
> >> 
> >> [...]
> >> 
> >What is a problem here? Calling port_new from user perspective will have
> >devlink lock, right? Do you mean that devlink lock should be taken for
> >whole cleanup, so from the start to the moment when devlink is
> >unregister? I wrote that, I will do that in next version (moving it
> 
> Yep, otherwise you can ice_dealloc_all_dynamic_ports() and end up with
> another port created after that which nobody cleans-up.
> 

Thanks for pointing it, as you mentioned in other patch, I will take a
lock for whole init/cleanup.

> >upper).
> >
> >> 
> >> >> 
> >> >> >+	struct device *dev = ice_pf_to_dev(pf);
> >> >> >+	int err;
> >> >> >+
> >> >> >+	dev_dbg(dev, "%s flavour:%d index:%d pfnum:%d\n", __func__,
> >> >> >+		new_attr->flavour, new_attr->port_index, new_attr->pfnum);
> >> >> 
> >> >> How this message could ever help anyone?
> >> >>
> >> >Probably only developer of the code :p, will remove it
> >> 
> >> How exactly?
> >>
> >I meant this code developer, it probably was used to check if number and
> >indexes are correct, but now it should be removed. Like, leftover after
> >developing, sorry.
> >
> >> [...]
> >> 
> >> 
> >> >> >+static int ice_sf_cfg_netdev(struct ice_dynamic_port *dyn_port)
> >> >> >+{
> >> >> >+	struct net_device *netdev;
> >> >> >+	struct ice_vsi *vsi = dyn_port->vsi;
> >> >> >+	struct ice_netdev_priv *np;
> >> >> >+	int err;
> >> >> >+
> >> >> >+	netdev = alloc_etherdev_mqs(sizeof(*np), vsi->alloc_txq,
> >> >> >+				    vsi->alloc_rxq);
> >> >> >+	if (!netdev)
> >> >> >+		return -ENOMEM;
> >> >> >+
> >> >> >+	SET_NETDEV_DEV(netdev, &vsi->back->pdev->dev);
> >> >> >+	set_bit(ICE_VSI_NETDEV_ALLOCD, vsi->state);
> >> >> >+	vsi->netdev = netdev;
> >> >> >+	np = netdev_priv(netdev);
> >> >> >+	np->vsi = vsi;
> >> >> >+
> >> >> >+	ice_set_netdev_features(netdev);
> >> >> >+
> >> >> >+	netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> >> >> >+			       NETDEV_XDP_ACT_XSK_ZEROCOPY |
> >> >> >+			       NETDEV_XDP_ACT_RX_SG;
> >> >> >+
> >> >> >+	eth_hw_addr_set(netdev, dyn_port->hw_addr);
> >> >> >+	ether_addr_copy(netdev->perm_addr, dyn_port->hw_addr);
> >> >> >+	netdev->netdev_ops = &ice_sf_netdev_ops;
> >> >> >+	SET_NETDEV_DEVLINK_PORT(netdev, &dyn_port->devlink_port);
> >> >> >+
> >> >> >+	err = register_netdev(netdev);
> >> >> 
> >> >> It the the actual subfunction or eswitch port representor of the
> >> >> subfunction. Looks like the port representor. In that case. It should be
> >> >> created no matter if the subfunction is activated, when it it created.
> >> >> 
> >> >> If this is the actual subfunction netdev, you should not link it to
> >> >> devlink port here.
> >> >>
> >> >This is the actual subfunction netdev. Where in this case it should be
> >> >linked?
> >> 
> >> To the SF auxdev, obviously.
> >> 
> >> Here, you should have eswitch port representor netdev.
> >> 
> >Oh, ok, thanks, will link it correctly in next version.
> >