From: Leon Romanovsky <leon@kernel.org>
To: Weihang Li <liweihang@huawei.com>
Cc: dledford@redhat.com, jgg@ziepe.ca, shiraz.saleem@intel.com,
aditr@vmware.com, mkalderon@marvell.com, aelior@marvell.com,
linux-rdma@vger.kernel.org, linuxarm@huawei.com
Subject: Re: [PATCH RFC for-next 1/6] RDMA/core: support deliver net device event
Date: Thu, 16 Jan 2020 13:37:10 +0200 [thread overview]
Message-ID: <20200116113710.GD6853@unreal> (raw)
In-Reply-To: <1579147847-12158-2-git-send-email-liweihang@huawei.com>
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
>
next prev parent reply other threads:[~2020-01-16 11:37 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-16 4:10 [PATCH RFC for-next 0/6] ofed support to send ib port link event Weihang Li
2020-01-16 4:10 ` [PATCH RFC for-next 1/6] RDMA/core: support deliver net device event Weihang Li
2020-01-16 11:37 ` Leon Romanovsky [this message]
2020-01-17 14:23 ` Jason Gunthorpe
[not found] ` <19bd56ac-5df5-f5bb-e024-54ef3cd0d0ad@huawei.com>
2020-01-19 9:32 ` Leon Romanovsky
2020-01-16 4:10 ` [PATCH RFC for-next 2/6] RDMA/mlx5: remove " Weihang Li
2020-01-16 11:41 ` Leon Romanovsky
[not found] ` <7f3f8190-6b62-f3c6-e4db-2425411fa639@huawei.com>
2020-01-20 7:45 ` Leon Romanovsky
2020-01-16 4:10 ` [PATCH RFC for-next 3/6] RDMA/i40iw: " Weihang Li
2020-01-16 4:10 ` [PATCH RFC for-next 4/6] RDMA/qedr: " Weihang Li
2020-01-16 4:10 ` [PATCH RFC for-next 5/6] RDMA/vmw_pvrdma: " Weihang Li
2020-01-16 4:10 ` [PATCH RFC for-next 6/6] qede: remove invalid notify operation Weihang Li
2020-01-16 11:15 ` [PATCH RFC for-next 0/6] ofed support to send ib port link event Leon Romanovsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200116113710.GD6853@unreal \
--to=leon@kernel.org \
--cc=aditr@vmware.com \
--cc=aelior@marvell.com \
--cc=dledford@redhat.com \
--cc=jgg@ziepe.ca \
--cc=linux-rdma@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=liweihang@huawei.com \
--cc=mkalderon@marvell.com \
--cc=shiraz.saleem@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.