Re: [PATCH for-next V5 03/12] IB/core: Add RoCE GID population

Jason Gunthorpe
2015-06-11T04:18:31Z

On Mon, Jun 08, 2015 at 05:12:06PM +0300, Matan Barak wrote:
> drivers/infiniband/core/core_priv.h | 26 ++
> drivers/infiniband/core/device.c | 77 +++++

I wouldn't mind seeing the core portion which consists of adding
the get_netdev be it's own little mini-series of three, adding the
core and the two driver enablement changes.

> + rcu_read_lock();
> + if (ib_dev->get_netdev)
> + idev =
> ib_dev->get_netdev(ib_dev, port);

If it wasn't clear from before, don't hold a rcu_read_lock around a
driver call back, return a net_device that is already dev_hold'd or
null.

> + down_read(&lists_rwsem);
> + list_for_each_entry_rcu(dev, &device_list, core_list)

No _rcu

> +void ib_dev_roce_ports_of_netdev(struct ib_device *ib_dev, roce_netdev_filter filter,
> + void *filter_cookie, roce_netdev_callback cb,
> + void *cookie)
> +{
> + u8 port;
> +
> + if (ib_dev->modify_gid)
[..]
> + if (ib_dev->get_netdev)
> + idev = ib_dev->get_netdev(ib_dev, port);

Why check modify_gid and then go on to test and call get_netdev? That
seems strange for a general purpose core API.

> + /* When calling get_netdev, the HW vendor's driver should return the
> + * net device of device @device at port @port_num. The function
> + * is called in rtnl_lock. The HW vendor's device driver must guarantee
> + * to return NULL before the net device has reached
> + * NETDEV_UNREGISTER_FINAL state.

rtnl_lock ? That doesn't seem to match what is going on..

Jason