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

[Jiri Pirko <jiri@resnulli.us>]

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.

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