diff mbox

[rdma-next,13/19] RDMA/netlink: Add netlink device definitions to UAPI

Message ID 20170621060528.3752-14-leon@kernel.org (mailing list archive)
State Superseded
Headers show

Commit Message

Leon Romanovsky June 21, 2017, 6:05 a.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

Introduce new defines to rdma_netlink.h, so the RDMA configuration tool
will be able to communicate with RDMA subsystem by using the shared defines.

The addition of new client (NLDEV) revealed the fact that we exposed by
mistake the RDMA_NL_I40IW define which is not backed by any RDMA netlink
by now and it won't be exposed in the future too. So this patch reuses
the value and leaves the comment together with old definition to whose
who are using RDMA_NL_I40IW as a replacement for digit "5".

The NLDEV operates with objects. The struct ib_device has two straightforward
objects: device itself and ports of that device.

This brings us to propose the following commands to work on those objects:
 * RDMA_NLDEV_CMD_{GET,SET,NEW,DEL} - works on ib_device itself
 * RDMA_NLDEV_CMD_PORT_{GET,SET,NEW,DEL} - works on ports of specific ib_device

Those commands receive/return the device name (RDMA_NLDEV_ATTR_DEV_NAME)
and port index (RDMA_NLDEV_ATTR_PORT_INDEX). For device object accesses,
the RDMA_NLDEV_ATTR_PORT_INDEX will return the maximum number of ports
for specific ib_device and for port access the actual port index.

The port index starts from 1 to follow RDMA/core internal semantics and
the sysfs exposed knobs..

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/netlink.c |  2 +-
 include/uapi/rdma/rdma_netlink.h  | 42 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Comments

Steve Wise June 21, 2017, 2:21 p.m. UTC | #1 
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Introduce new defines to rdma_netlink.h, so the RDMA configuration tool
> will be able to communicate with RDMA subsystem by using the shared defines.
> 
> The addition of new client (NLDEV) revealed the fact that we exposed by
> mistake the RDMA_NL_I40IW define which is not backed by any RDMA netlink
> by now and it won't be exposed in the future too. So this patch reuses
> the value and leaves the comment together with old definition to whose
> who are using RDMA_NL_I40IW as a replacement for digit "5".
> 
> The NLDEV operates with objects. The struct ib_device has two straightforward
> objects: device itself and ports of that device.
> 
> This brings us to propose the following commands to work on those objects:
>  * RDMA_NLDEV_CMD_{GET,SET,NEW,DEL} - works on ib_device itself
>  * RDMA_NLDEV_CMD_PORT_{GET,SET,NEW,DEL} - works on ports of specific
> ib_device
> 
> Those commands receive/return the device name
> (RDMA_NLDEV_ATTR_DEV_NAME)
> and port index (RDMA_NLDEV_ATTR_PORT_INDEX). For device object accesses,
> the RDMA_NLDEV_ATTR_PORT_INDEX will return the maximum number of ports
> for specific ib_device and for port access the actual port index.
> 
> The port index starts from 1 to follow RDMA/core internal semantics and
> the sysfs exposed knobs..
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

Reviewed-by: Steve Wise <swise@opengridcomputing.com>


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe June 21, 2017, 4:11 p.m. UTC | #2 
On Wed, Jun 21, 2017 at 09:05:22AM +0300, Leon Romanovsky wrote:
>  	RDMA_NL_IWCM,
>  	RDMA_NL_RSVD,
>  	RDMA_NL_LS,	/* RDMA Local Services */
> +	/*
> +	 * RDMA_NL_I40IW not in use and it was added here by mistake,
> +	 * but we need to keep it anyway, because it is UAPI and for
> +	 * unknown reasons, someone in the field decided to replace "5"
> +	 * with this define.
> +	 */
>  	RDMA_NL_I40IW,

Since this doesn't do anything, never did anything, I think you should
just delete it. Anything in userspace that refers to this name is
unconditionally broken and a compile failure is the better choice.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chien Tung June 21, 2017, 4:18 p.m. UTC | #3 
> From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com]
> Sent: Wednesday, June 21, 2017 11:12 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>; linux-rdma@vger.kernel.org; Tung,
> Chien Tin <chien.tin.tung@intel.com>; Steve Wise
> <swise@opengridcomputing.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Jiri Pirko <jiri@mellanox.com>; Ariel Almog
> <ariela@mellanox.com>; Linux Netdev <netdev@vger.kernel.org>; Leon
> Romanovsky <leonro@mellanox.com>
> Subject: Re: [PATCH rdma-next 13/19] RDMA/netlink: Add netlink device
> definitions to UAPI
> 
> On Wed, Jun 21, 2017 at 09:05:22AM +0300, Leon Romanovsky wrote:
> >  	RDMA_NL_IWCM,
> >  	RDMA_NL_RSVD,
> >  	RDMA_NL_LS,	/* RDMA Local Services */
> > +	/*
> > +	 * RDMA_NL_I40IW not in use and it was added here by mistake,
> > +	 * but we need to keep it anyway, because it is UAPI and for
> > +	 * unknown reasons, someone in the field decided to replace "5"
> > +	 * with this define.
> > +	 */
> >  	RDMA_NL_I40IW,
> 
> Since this doesn't do anything, never did anything, I think you should just
> delete it. Anything in userspace that refers to this name is unconditionally
> broken and a compile failure is the better choice.
> 
> Jason

I agree.  Please delete RDMA_NL_I40IW.

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leon Romanovsky June 22, 2017, 4:28 a.m. UTC | #4 
On Wed, Jun 21, 2017 at 04:18:54PM +0000, Tung, Chien Tin wrote:
> > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com]
> > Sent: Wednesday, June 21, 2017 11:12 AM
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: Doug Ledford <dledford@redhat.com>; linux-rdma@vger.kernel.org; Tung,
> > Chien Tin <chien.tin.tung@intel.com>; Steve Wise
> > <swise@opengridcomputing.com>; Stephen Hemminger
> > <stephen@networkplumber.org>; Jiri Pirko <jiri@mellanox.com>; Ariel Almog
> > <ariela@mellanox.com>; Linux Netdev <netdev@vger.kernel.org>; Leon
> > Romanovsky <leonro@mellanox.com>
> > Subject: Re: [PATCH rdma-next 13/19] RDMA/netlink: Add netlink device
> > definitions to UAPI
> >
> > On Wed, Jun 21, 2017 at 09:05:22AM +0300, Leon Romanovsky wrote:
> > >  	RDMA_NL_IWCM,
> > >  	RDMA_NL_RSVD,
> > >  	RDMA_NL_LS,	/* RDMA Local Services */
> > > +	/*
> > > +	 * RDMA_NL_I40IW not in use and it was added here by mistake,
> > > +	 * but we need to keep it anyway, because it is UAPI and for
> > > +	 * unknown reasons, someone in the field decided to replace "5"
> > > +	 * with this define.
> > > +	 */
> > >  	RDMA_NL_I40IW,
> >
> > Since this doesn't do anything, never did anything, I think you should just
> > delete it. Anything in userspace that refers to this name is unconditionally
> > broken and a compile failure is the better choice.
> >
> > Jason
>
> I agree.  Please delete RDMA_NL_I40IW.

Fixed and Steve's comments too.

Thanks

>
> Chien
diff mbox

Patch

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index 9731c313e9b9..cce9b7af4a3b 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -60,7 +60,7 @@  static bool is_nl_msg_valid(unsigned int type, unsigned int op)
 				  RDMA_NL_IWPM_NUM_OPS,
 				  0,
 				  RDMA_NL_LS_NUM_OPS,
-				  0 };
+				  RDMA_NLDEV_NUM_OPS };
 
 	/*
 	 * This BUILD_BUG_ON is intended to catch addition of new
diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
index 02fe8390c18f..bfafd5996d52 100644
--- a/include/uapi/rdma/rdma_netlink.h
+++ b/include/uapi/rdma/rdma_netlink.h
@@ -8,7 +8,14 @@  enum {
 	RDMA_NL_IWCM,
 	RDMA_NL_RSVD,
 	RDMA_NL_LS,	/* RDMA Local Services */
+	/*
+	 * RDMA_NL_I40IW not in use and it was added here by mistake,
+	 * but we need to keep it anyway, because it is UAPI and for
+	 * unknown reasons, someone in the field decided to replace "5"
+	 * with this define.
+	 */
 	RDMA_NL_I40IW,
+	RDMA_NL_NLDEV = RDMA_NL_I40IW, /* RDMA device interface */
 	RDMA_NL_NUM_CLIENTS
 };
 
@@ -222,4 +229,39 @@  struct rdma_nla_ls_gid {
 	__u8		gid[16];
 };
 
+enum rdma_nldev_command {
+	RDMA_NLDEV_CMD_UNSPEC,
+
+	RDMA_NLDEV_CMD_GET, /* can dump */
+	RDMA_NLDEV_CMD_SET,
+	RDMA_NLDEV_CMD_NEW,
+	RDMA_NLDEV_CMD_DEL,
+
+	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
+	RDMA_NLDEV_CMD_PORT_SET,
+	RDMA_NLDEV_CMD_PORT_NEW,
+	RDMA_NLDEV_CMD_PORT_DEL,
+
+	RDMA_NLDEV_NUM_OPS
+};
+
+enum rdma_nldev_attr {
+	/* don't change the order or add anything between, this is ABI! */
+	RDMA_NLDEV_ATTR_UNSPEC,
+
+	/* Identifier for ib_device */
+	RDMA_NLDEV_ATTR_DEV_NAME,		/* string */
+	/*
+	 * Device name together with port index are identifiers
+	 * for port/link properties.
+	 *
+	 * For RDMA_NLDEV_CMD_GET comamnd, port index will return number
+	 * of available ports in ib_device, while for port specific operations,
+	 * it will be real port index as it appears in sysfs. Port index follows
+	 * sysfs notation and starts from 1 for the first port.
+	 */
+	RDMA_NLDEV_ATTR_PORT_INDEX,		/* u32 */
+
+	RDMA_NLDEV_ATTR_MAX
+};
 #endif /* _UAPI_RDMA_NETLINK_H */