On Wed, Jul 10, 2024 at 10:59:15AM -0700, David Ahern wrote:
> On 7/10/24 12:09 AM, Leon Romanovsky wrote:
> >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> >> index 55aa7aa32d4a..7ddaec923569 100644
> >> --- a/drivers/infiniband/core/device.c
> >> +++ b/drivers/infiniband/core/device.c
> >> @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev,
> >> }
> >>
> >> if (old_ndev)
> >> - netdev_tracker_free(ndev, &pdata->netdev_tracker);
> >> + netdev_put(old_ndev, &pdata->netdev_tracker);
> >
> > It should stay netdev_tracker_free() and not netdev_put(). We are
> > calling to __dev_put(old_ndev) later in the function.
> >
>
> missed that and KASAN and refcount debugging did not complain ...
>
> Anyways, why have the 2 split apart? ie., why not remove the __dev_put
> and just do netdev_put here? old_ndev is not needed in between calls.
> Asymmetric calls like this are always confusing.
Maybe it is Ok like this:
@@ -2166,15 +2166,14 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev,
return 0;
}
+ rcu_assign_pointer(pdata->netdev, ndev);
if (old_ndev)
- netdev_tracker_free(ndev, &pdata->netdev_tracker);
+ netdev_put(old_ndev, &pdata->netdev_tracker);
if (ndev)
netdev_hold(ndev, &pdata->netdev_tracker, GFP_ATOMIC);
- rcu_assign_pointer(pdata->netdev, ndev);
spin_unlock_irqrestore(&pdata->netdev_lock, flags);
add_ndev_hash(pdata);
- __dev_put(old_ndev);
return 0;
}
Don't like that we drop the ref and leave the pdata->netdev assigned
to something with no ref, even though it is OK for RCU reasons..
Jason
