Tue, Feb 13, 2024 at 10:39:47AM CET, michal.swiatkow...@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.swiatkow...@linux.intel.com
>> wrote:
>> >From: Piotr Raczynski <piotr.raczyn...@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?
[...]
>>
>> >+ 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?
[...]
>> >+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.
