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 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
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.