Re: [PATCH RFC for-next 1/6] RDMA/core: support deliver net device event

[Leon Romanovsky <leon@kernel.org>]

On Thu, Jan 16, 2020 at 12:10:42PM +0800, Weihang Li wrote:
> From: Lang Cheng <chenglang@huawei.com>
>
> For the process of handling the link event of the net device, the driver
> of each provider is similar, so it can be integrated into the ib_core for
> unified processing.
>
> Signed-off-by: Lang Cheng <chenglang@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/core/cache.c  |  21 ++++++-
>  drivers/infiniband/core/device.c | 123 +++++++++++++++++++++++++++++++++++++++
>  include/rdma/ib_cache.h          |  13 +++++
>  include/rdma/ib_verbs.h          |   8 +++
>  4 files changed, 164 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 17bfedd..791e965 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -1174,6 +1174,23 @@ int ib_get_cached_port_state(struct ib_device   *device,
>  }
>  EXPORT_SYMBOL(ib_get_cached_port_state);
>
> +int ib_get_cached_port_event_flags(struct ib_device   *device,
> +				   u8                  port_num,
> +				   enum ib_port_flags *event_flags)
> +{
> +	unsigned long flags;
> +
> +	if (!rdma_is_port_valid(device, port_num))
> +		return -EINVAL;

I know that ib_get_cached_port_state() has the same check, but it is
dumb, we don't need to call this function with wrong port_num.

> +
> +	read_lock_irqsave(&device->cache_lock, flags);
> +	*event_flags = device->port_data[port_num].cache.port_event_flags;
> +	read_unlock_irqrestore(&device->cache_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ib_get_cached_port_event_flags);

This function is limited to ib_core.ko, no need in EXPORT_SYMBOL.

> +
>  /**
>   * rdma_get_gid_attr - Returns GID attributes for a port of a device
>   * at a requested gid_index, if a valid GID entry exists.
> @@ -1391,7 +1408,7 @@ ib_cache_update(struct ib_device *device, u8 port, bool enforce_security)
>  	if (!rdma_is_port_valid(device, port))
>  		return -EINVAL;
>
> -	tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
> +	tprops = kzalloc(sizeof(*tprops), GFP_KERNEL);

Unrelated change but OK.

>  	if (!tprops)
>  		return -ENOMEM;
>
> @@ -1435,6 +1452,8 @@ ib_cache_update(struct ib_device *device, u8 port, bool enforce_security)
>  	device->port_data[port].cache.pkey = pkey_cache;
>  	device->port_data[port].cache.lmc = tprops->lmc;
>  	device->port_data[port].cache.port_state = tprops->state;
> +	device->port_data[port].cache.port_event_flags =
> +						tprops->port_event_flags;
>
>  	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
>  	write_unlock_irq(&device->cache_lock);
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index f6c2552..f03d6ce 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1325,6 +1325,77 @@ static int enable_device_and_get(struct ib_device *device)
>  	return ret;
>  }
>
> +unsigned int ib_query_ndev_port_num(struct ib_device *device,
> +				    struct net_device *netdev)
> +{
> +	unsigned int port_num;
> +
> +	rdma_for_each_port(device, port_num)
> +		if (netdev == device->port_data[port_num].netdev)
> +			break;

Each event will do this function, while mlx5 has different logic separated
to RoCE, IB and representors.

> +
> +	return port_num;
> +}
> +EXPORT_SYMBOL(ib_query_ndev_port_num);

No need in EXPORT_SYMBOL.

> +
> +static inline enum ib_port_state get_port_state(struct net_device *netdev)

No inline function in C files.
> +{
> +	return (netif_running(netdev) && netif_carrier_ok(netdev)) ?
> +		IB_PORT_ACTIVE : IB_PORT_DOWN;
> +}
> +
> +static int ib_netdev_event(struct notifier_block *this,
> +			   unsigned long event, void *ptr)
> +{
> +	struct ib_device *device = container_of(this, struct ib_device, nb);
> +	struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> +
> +	switch (event) {
> +	case NETDEV_CHANGE:
> +	case NETDEV_UP:
> +	case NETDEV_DOWN: {
> +		unsigned int port_num = ib_query_ndev_port_num(device, netdev);
> +		enum ib_port_state last_state;
> +		enum ib_port_state curr_state;
> +		struct ib_event ibev;
> +		enum ib_port_flags flags;
> +
> +		if (ib_get_cached_port_event_flags(device, port_num, &flags))
> +			return NOTIFY_DONE;
> +
> +		if (flags & IB_PORT_BONDING_SLAVE)
> +			goto done;

I afraid that this "goto ..." breaks RoCE bonding.

> +
> +		if (ib_get_cached_port_state(device, port_num, &last_state))
> +			return NOTIFY_DONE;
> +
> +		curr_state = get_port_state(netdev);
> +
> +		if (last_state == curr_state)
> +			goto done;
> +
> +		ibev.device = device;
> +		if (curr_state == IB_PORT_DOWN)
> +			ibev.event = IB_EVENT_PORT_ERR;
> +		else if (curr_state == IB_PORT_ACTIVE)
> +			ibev.event = IB_EVENT_PORT_ACTIVE;
> +		else
> +			goto done;
> +
> +		ibev.element.port_num = port_num;
> +		ib_dispatch_event(&ibev);
> +		dev_dbg(&device->dev,
> +			"core send %s\n", ib_event_msg(ibev.event));
> +		break;

You put break in wrong place.

> +	}
> +
> +	default:
> +		break;
> +	}
> +done:
> +	return NOTIFY_DONE;
> +}
> +
>  /**
>   * ib_register_device - Register an IB device with IB core
>   * @device: Device to register
> @@ -1342,6 +1413,7 @@ static int enable_device_and_get(struct ib_device *device)
>   */
>  int ib_register_device(struct ib_device *device, const char *name)
>  {
> +	unsigned int port;
>  	int ret;
>
>  	ret = assign_name(device, name);
> @@ -1406,6 +1478,34 @@ int ib_register_device(struct ib_device *device, const char *name)
>  	}
>  	ib_device_put(device);
>
> +	device->nb.notifier_call = ib_netdev_event;
> +	ret = register_netdevice_notifier(&device->nb);

It is not relevant for IB and OPA devices.

> +	if (ret) {
> +		device->nb.notifier_call = NULL;
> +		return ret;
> +	}
> +
> +	rdma_for_each_port(device, port) {
> +		struct ib_event event;
> +		struct net_device *netdev = ib_device_get_netdev(device, port);
> +		enum ib_port_flags flags;
> +
> +		if (ib_get_cached_port_event_flags(device, port, &flags))
> +			continue;
> +
> +		if (flags & IB_PORT_BONDING_SLAVE)
> +			continue;
> +
> +		if (get_port_state(netdev) == IB_PORT_ACTIVE) {
> +			event.device = device;
> +			event.event = IB_EVENT_PORT_ACTIVE;
> +			event.element.port_num = port;
> +			ib_dispatch_event(&event);
> +			dev_dbg(&device->dev, "init event %s\n",
> +				ib_event_msg(event.event));

You already added print to newly ib_netdev_event().

> +		}
> +	}
> +
>  	return 0;
>
>  dev_cleanup:
> @@ -1470,6 +1570,29 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
>   */
>  void ib_unregister_device(struct ib_device *ib_dev)
>  {
> +	unsigned int port;
> +
> +	unregister_netdevice_notifier(&ib_dev->nb);

Not relevant for IB and OPA.

> +
> +	rdma_for_each_port(ib_dev, port) {
> +		struct net_device *netdev = ib_device_get_netdev(ib_dev, port);
> +		struct ib_event event;
> +		u32 flags;
> +
> +		if (ib_get_cached_port_event_flags(ib_dev, port, &flags))
> +			continue;
> +
> +		if (flags)
> +			continue;
> +
> +		if (get_port_state(netdev) == IB_PORT_ACTIVE) {
> +			event.device = ib_dev;
> +			event.event = IB_EVENT_PORT_ERR;
> +			event.element.port_num = port;
> +			ib_dispatch_event(&event);
> +		}
> +	}
> +
>  	get_device(&ib_dev->dev);
>  	__ib_unregister_device(ib_dev);
>  	put_device(&ib_dev->dev);
> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
> index 870b5e6..f37f667 100644
> --- a/include/rdma/ib_cache.h
> +++ b/include/rdma/ib_cache.h
> @@ -131,6 +131,19 @@ int ib_get_cached_port_state(struct ib_device *device,
>  			      u8                port_num,
>  			      enum ib_port_state *port_active);
>
> +/**
> + * ib_get_cached_port_event_flags - Returns a cached port event flags
> + * @device: The device to query.
> + * @port_num: The port number of the device to query.
> + * @event_flags: port_state for the specified port for that device.
> + *
> + * ib_get_cached_port_event_flags() fetches the specified event flags stored in
> + * the local software cache.
> + */
> +int ib_get_cached_port_event_flags(struct ib_device   *device,
> +				   u8                  port_num,
> +				   enum ib_port_flags *event_flags);
> +
>  bool rdma_is_zero_gid(const union ib_gid *gid);
>  const struct ib_gid_attr *rdma_get_gid_attr(struct ib_device *device,
>  					    u8 port_num, int index);
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index d8031f6..ce88d0b 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -670,6 +670,7 @@ struct ib_port_attr {
>  	u8			active_speed;
>  	u8                      phys_state;
>  	u16			port_cap_flags2;
> +	u32			port_event_flags;
>  };
>
>  enum ib_device_modify_flags {
> @@ -2149,12 +2150,18 @@ enum ib_mad_result {
>  	IB_MAD_RESULT_CONSUMED = 1 << 2  /* Packet consumed: stop processing */
>  };
>
> +enum ib_port_flags {

Please don't do named enums for bitwise declarations.

> +	IB_PORT_NORMAL = 0,		/* normarl port            */

normarl -> normal
Better to describe what do you mean by "normal port".

> +	IB_PORT_BONDING_SLAVE = 1 << 0, /* rdma bonding slave port */
> +};
> +
>  struct ib_port_cache {
>  	u64		      subnet_prefix;
>  	struct ib_pkey_cache  *pkey;
>  	struct ib_gid_table   *gid;
>  	u8                     lmc;
>  	enum ib_port_state     port_state;
> +	enum ib_port_flags     port_event_flags;

Please no, u32.

>  };
>
>  struct ib_port_immutable {
> @@ -2706,6 +2713,7 @@ struct ib_device {
>  	/* Used by iWarp CM */
>  	char iw_ifname[IFNAMSIZ];
>  	u32 iw_driver_flags;
> +	struct notifier_block nb;
>  };
>
>  struct ib_client_nl_info;
> --
> 2.8.1
>