Tue, Feb 13, 2024 at 01:02:43PM CET, [email protected] wrote:
>On Tue, Feb 13, 2024 at 12:27:20PM +0100, Jiri Pirko wrote:
>> Tue, Feb 13, 2024 at 10:39:47AM CET, [email protected]
>> wrote:
>> >On Tue, Feb 13, 2024 at 09:55:20AM +0100, Jiri Pirko wrote:
>> >> Tue, Feb 13, 2024 at 08:27:13AM CET, [email protected]
>> >> wrote:
>> >> >From: Piotr Raczynski <[email protected]>
>>
>> [...]
>>
>>
>> >
>> >>
>> >> >+}
>> >> >+
>> >> >+/**
>> >> >+ * 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.
>
