On Tue, Feb 13, 2024 at 03:57:59PM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 01:02:43PM CET, michal.swiatkow...@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.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?
> >>
> >> [...]
> >>
> >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.
> >
