Re: [ RFC ] igb: first draft of igb rtnl_link_ops interface for vf creation (was Re: [net-next PATCH v3] igbvf: add new driver to support 82576 virtual functions)

[Patrick McHardy <kaber@trash.net>]

Alexander Duyck wrote:
> In the meantime I have been working on the rtnl_link_ops approach and I 
> think I have a few things going but I wanted to get some input before I 
> go much further.
> 
> First, is it ok for me to call rtnl_unlock prior to doing my settings 
> changes on the sriov config space, followed by rtnl_lock afterwards in 
> my newlink and dellink operations?  I ask because I had to do this in 
> order to prevent a deadlock when the pci-hotplug events fired for the 
> vfs and called unregister/register_netdev.

No, both functions are called with the RTNL already held. I'm not
sure I understand what kind of potential deadlock you're trying
to avoid. The ->newlink and ->dellink functions are called (mainly)
in response to userspace netlink messages and there should never
be a need to change anything related to rtnl locking.

A deadlock can happen when you call rtnl_link_unregister() while
holding the RTNL. There's an unlocked version (__rtnl_link_unregister)
for this case.

If that doesn't answer your question, please provide more detail.

> Second is it acceptable for me to just free the netdev at the end of 
> newlink and call delete on the PF interface directly?  I ask because I 
> don't have any use for the netdevs that are generated and I cannot call 
> delete on specific VFs anyway since they are allocated/freed in LIFO 
> order so I would always have to free the last one I allocated.

No, the newly created netdev is freed when returning an error, other
netdevs should not be touched.

> I have included a patch for review below that implements these changes 
> against the current driver.  Please feel free to comment.
> 
> +static int igb_new_vf(struct net_device *dev, struct nlattr *tb[],
> +                      struct nlattr *data[])
> +{
> +    struct net_device *netdev;
> +    struct igb_adapter *adapter;
> +    int err;
> +
> +    netdev = __dev_get_by_index(dev_net(dev), nla_get_u32(tb[IFLA_LINK]));
> +
> +    if (!netdev)
> +        return -ENODEV;
> +   
> +    adapter = netdev_priv(netdev);
> +    err = igb_set_num_vfs(netdev, adapter->vfs_allocated_count + 1);
> +    if (!err)
> +        free_netdev(dev);
> +
> +    return err;
> +
> +}
> +
> +static void igb_del_vf(struct net_device *dev)
> +{
> +    struct igb_adapter *adapter = netdev_priv(dev);
> +
> +    if (adapter->vfs_allocated_count > 0)
> +        igb_set_num_vfs(dev, adapter->vfs_allocated_count - 1);

Thats not really how this is supposed to work. Every device is an
independant instance, so you can delete them in arbitrary order.
If you need to assign them some device resources, you need to do
this mapping internally.