On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote:
> To avoid inconsistent user experience for PKey interfaces that are
> created via netlink vs PKey interfaces that are created via sysfs and the
> base interface, make sure to set the rtnl_link_ops for all ipoib network
> devices, so the ipoib attributes will be reported/modified via iproute2
> for all ipoib interfaces regardless of how they are created.
>
> Also, after setting the rtnl_link_ops for the base interface, implement
> the dellink() callback to block users from trying to remove it.
>
> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
> Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
> v2: Update commit message.
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++
> drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++
> drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++
> 3 files changed, 15 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index ab75b7f745d4..96b6be5d507d 100644
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format,
> /* call event handler to ensure pkey in sync */
> queue_work(ipoib_workqueue, &priv->flush_heavy);
>
> + ndev->rtnl_link_ops = ipoib_get_link_ops();
> +
> result = register_netdev(ndev);
Why do we need this one? I understand fixing the sysfs but not this part.
> if (result) {
> pr_warn("%s: couldn't register ipoib port %d; error %d\n",
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
> index 38c984d16996..d5a90a66b45c 100644
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
> @@ -144,6 +144,16 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,
> return 0;
> }
>
> +static void ipoib_del_child_link(struct net_device *dev, struct list_head *head)
> +{
> + struct ipoib_dev_priv *priv = ipoib_priv(dev);
> +
> + if (!priv->parent)
> + return;
> +
> + unregister_netdevice_queue(dev, head);
> +}
The above is why this hunk was added right?
> static size_t ipoib_get_size(const struct net_device *dev)
> {
> return nla_total_size(2) + /* IFLA_IPOIB_PKEY */
> @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = {
> .priv_size = sizeof(struct ipoib_dev_priv),
> .setup = ipoib_setup_common,
> .newlink = ipoib_new_child_link,
> + .dellink = ipoib_del_child_link,
> .changelink = ipoib_changelink,
> .get_size = ipoib_get_size,
> .fill_info = ipoib_fill_info,
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> index 30865605e098..c60db9f3f5ac 100644
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
> goto out_early;
> }
>
> + ndev->rtnl_link_ops = ipoib_get_link_ops();
> +
If this is only for the sysfs case why isn't it in ipoib_vlan_add()
which is the sysfs only flow?
Jason
