diff mbox

[rdma-next,6/6] RDMA/core: Unify style of IOCTL commands

Message ID 1471355123-6227-7-git-send-email-leon@kernel.org (mailing list archive)
State Superseded
Headers show

Commit Message

Leon Romanovsky Aug. 16, 2016, 1:45 p.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

MAD and HFI1 have different naming convention, this patch
simplifies and unifies their defines and names.

As part of cleanup, the HFI1 _NUM() macro was removed and MAD
indexes were renamed. It has a potential to break application
which use these defines directly.

Signed-off-by: Matan Barak <matanb@mellanox.com>
Signed-off-by: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 include/uapi/rdma/rdma_user_ioctl.h | 98 ++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 40 deletions(-)

Comments

Dennis Dalessandro Aug. 16, 2016, 2:31 p.m. UTC | #1 
T24gVHVlLCAyMDE2LTA4LTE2IGF0IDE2OjQ1ICswMzAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6
DQo+IEZyb206IExlb24gUm9tYW5vdnNreSA8bGVvbnJvQG1lbGxhbm94LmNvbT4NCj4gDQo+IE1B
RCBhbmQgSEZJMSBoYXZlIGRpZmZlcmVudCBuYW1pbmcgY29udmVudGlvbiwgdGhpcyBwYXRjaA0K
PiBzaW1wbGlmaWVzIGFuZCB1bmlmaWVzIHRoZWlyIGRlZmluZXMgYW5kIG5hbWVzLg0KDQpJIGRv
bid0IGtub3cgdGhhdCBJIGFncmVlIHRoYXQgaXQgc2ltcGxpZmllcyB0aGluZ3MuIEl0IGNoYW5n
ZXMgYSBsb3QNCm9mIGNvZGUgZm9yIG5vdCBtdWNoIHJlYWwgdmFsdWUgaW4gbXkgb3Bpbmlvbi7C
oA0KDQpXYXMgdGhpcyBzb21ldGhpbmcgdGhhdCB3YXMgZGlzY3Vzc2VkIGluIHRoZSB2ZXJicyBj
YWxsPyBJIGhhdmUgbm90DQpiZWVuIGFibGUgdG8gYXR0ZW5kIHRoYXQgdGhlIGxhc3QgZmV3IHdl
ZWtzLg0KDQo+IEFzIHBhcnQgb2YgY2xlYW51cCwgdGhlIEhGSTEgX05VTSgpIG1hY3JvIHdhcyBy
ZW1vdmVkIGFuZCBNQUQNCj4gaW5kZXhlcyB3ZXJlIHJlbmFtZWQuIEl0IGhhcyBhIHBvdGVudGlh
bCB0byBicmVhayBhcHBsaWNhdGlvbg0KPiB3aGljaCB1c2UgdGhlc2UgZGVmaW5lcyBkaXJlY3Rs
eS4NCg0KV2h5IGRvIHlvdSB3YW50IHRvIHJlbW92ZSB0aGUgX05VTSgpIG1hY3JvIHRoYXQgRG91
ZyBqdXN0IHB1dCBpbj8NCg0KLURlbm55
--
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 Aug. 16, 2016, 4:50 p.m. UTC | #2 
On Tue, Aug 16, 2016 at 02:31:32PM +0000, Dalessandro, Dennis wrote:
> On Tue, 2016-08-16 at 16:45 +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> > 
> > MAD and HFI1 have different naming convention, this patch
> > simplifies and unifies their defines and names.
> 
> I don't know that I agree that it simplifies things. It changes a lot
> of code for not much real value in my opinion. 
> 
> Was this something that was discussed in the verbs call? I have not
> been able to attend that the last few weeks.

It was discussed over mails, for example Jason's opinion [1],
Christopher Lameter's opinion [2], Christoph Hellwig's opinion [3].

> 
> > As part of cleanup, the HFI1 _NUM() macro was removed and MAD
> > indexes were renamed. It has a potential to break application
> > which use these defines directly.
> 
> Why do you want to remove the _NUM() macro that Doug just put in?

It is not used after refactoring and IMHO this macro doesn't
belong to UAPI, since it wouldn't in use by any users, but I'll be glad to
get an examples of its usage in real user space applications (libfabric???), if any.

> 
> -Denny

[1] http://marc.info/?l=linux-rdma&m=146835080018352&w=2
[2] http://marc.info/?l=linux-rdma&m=146897679911490&w=2
[3] http://marc.info/?l=linux-rdma&m=146908844509592&w=2
Dennis Dalessandro Aug. 16, 2016, 5:09 p.m. UTC | #3 
On Tue, 2016-08-16 at 19:50 +0300, Leon Romanovsky wrote:
> On Tue, Aug 16, 2016 at 02:31:32PM +0000, Dalessandro, Dennis wrote:

> > On Tue, 2016-08-16 at 16:45 +0300, Leon Romanovsky wrote:

> > > From: Leon Romanovsky <leonro@mellanox.com>

> > > 

> > > MAD and HFI1 have different naming convention, this patch

> > > simplifies and unifies their defines and names.

> > 

> > I don't know that I agree that it simplifies things. It changes a

> > lot

> > of code for not much real value in my opinion. 

> > 

> > Was this something that was discussed in the verbs call? I have not

> > been able to attend that the last few weeks.

> 

> It was discussed over mails, for example Jason's opinion [1],

> Christopher Lameter's opinion [2], Christoph Hellwig's opinion [3].


I'm not opposed to trying to unify things, however this seems to be
more than plop down the hfi1 stuff from here and put it over there. It
is certainly not simplifying anything.

> > > As part of cleanup, the HFI1 _NUM() macro was removed and MAD

> > > indexes were renamed. It has a potential to break application

> > > which use these defines directly.

> > 

> > Why do you want to remove the _NUM() macro that Doug just put in?

> 

> It is not used after refactoring and IMHO this macro doesn't

> belong to UAPI, since it wouldn't in use by any users, but I'll be

> glad to

> get an examples of its usage in real user space applications

> (libfabric???), if any.

> 


That's a fair point. I can see getting rid of it now.

-Denny
Leon Romanovsky Aug. 17, 2016, 5:19 a.m. UTC | #4 
On Tue, Aug 16, 2016 at 05:09:27PM +0000, Dalessandro, Dennis wrote:
> On Tue, 2016-08-16 at 19:50 +0300, Leon Romanovsky wrote:
> > On Tue, Aug 16, 2016 at 02:31:32PM +0000, Dalessandro, Dennis wrote:
> > > On Tue, 2016-08-16 at 16:45 +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > > 
> > > > MAD and HFI1 have different naming convention, this patch
> > > > simplifies and unifies their defines and names.
> > > 
> > > I don't know that I agree that it simplifies things. It changes a
> > > lot
> > > of code for not much real value in my opinion. 
> > > 
> > > Was this something that was discussed in the verbs call? I have not
> > > been able to attend that the last few weeks.
> > 
> > It was discussed over mails, for example Jason's opinion [1],
> > Christopher Lameter's opinion [2], Christoph Hellwig's opinion [3].
> 
> I'm not opposed to trying to unify things, however this seems to be
> more than plop down the hfi1 stuff from here and put it over there. It
> is certainly not simplifying anything.

It is "dark side" of UAPI - inability to change legacy declarations.
This is why I didn't remove anything except _NUM() macro.

The simplification comes from definition of one place for
declaration of IOCTLs numbers and exporting it to users. It gives
visibility for user space authors too.

> 
> > > > As part of cleanup, the HFI1 _NUM() macro was removed and MAD
> > > > indexes were renamed. It has a potential to break application
> > > > which use these defines directly.
> > > 
> > > Why do you want to remove the _NUM() macro that Doug just put in?
> > 
> > It is not used after refactoring and IMHO this macro doesn't
> > belong to UAPI, since it wouldn't in use by any users, but I'll be
> > glad to
> > get an examples of its usage in real user space applications
> > (libfabric???), if any.
> > 
> 
> That's a fair point. I can see getting rid of it now.
> 
> -Denny
Dennis Dalessandro Aug. 17, 2016, 1:45 p.m. UTC | #5 
On Wed, 2016-08-17 at 08:19 +0300, Leon Romanovsky wrote:


> > I'm not opposed to trying to unify things, however this seems to be

> > more than plop down the hfi1 stuff from here and put it over there.

> > It

> > is certainly not simplifying anything.

> 

> It is "dark side" of UAPI - inability to change legacy declarations.

> This is why I didn't remove anything except _NUM() macro.

> 

> The simplification comes from definition of one place for

> declaration of IOCTLs numbers and exporting it to users. It gives

> visibility for user space authors too.


How often does a user need the IOCTL numbers for MAD and PSM and
whatever else we come up with at the same time? Why would they care
about the other IOCTL numbers? 

I don't really see a problem with keeping them separate, but I'm not
really opposed to collapsing into one place either.

-Denny
Leon Romanovsky Aug. 17, 2016, 2:20 p.m. UTC | #6 
On Wed, Aug 17, 2016 at 01:45:21PM +0000, Dalessandro, Dennis wrote:
> On Wed, 2016-08-17 at 08:19 +0300, Leon Romanovsky wrote:
> 
> 
> > > I'm not opposed to trying to unify things, however this seems to be
> > > more than plop down the hfi1 stuff from here and put it over there.
> > > It
> > > is certainly not simplifying anything.
> > 
> > It is "dark side" of UAPI - inability to change legacy declarations.
> > This is why I didn't remove anything except _NUM() macro.
> > 
> > The simplification comes from definition of one place for
> > declaration of IOCTLs numbers and exporting it to users. It gives
> > visibility for user space authors too.
> 
> How often does a user need the IOCTL numbers for MAD and PSM and
> whatever else we come up with at the same time? Why would they care
> about the other IOCTL numbers? 

We will need new IOCTLs numbers for ABI and better if they be different
from already defined.

> 
> I don't really see a problem with keeping them separate, but I'm not
> really opposed to collapsing into one place either.

Thanks

> 
> -Denny
diff mbox

Patch

diff --git a/include/uapi/rdma/rdma_user_ioctl.h b/include/uapi/rdma/rdma_user_ioctl.h
index 7ecf8cd..9abd6d1 100644
--- a/include/uapi/rdma/rdma_user_ioctl.h
+++ b/include/uapi/rdma/rdma_user_ioctl.h
@@ -39,71 +39,89 @@ 
 #include <rdma/hfi/hfi1_ioctl.h>
 
 /* Documentation/ioctl/ioctl-number.txt */
-#define RDMA_IOCTL_MAGIC		0x1b
+#define RDMA_IOCTL_MAGIC	0x1b
 /* Legacy name, for user space application which already use it */
-#define IB_IOCTL_MAGIC			RDMA_IOCTL_MAGIC
+#define IB_IOCTL_MAGIC		RDMA_IOCTL_MAGIC
 
-#define IB_USER_MAD_REGISTER_AGENT	_IOWR(RDMA_IOCTL_MAGIC, 1, \
-					      struct ib_user_mad_reg_req)
+/* General blocks assignments */
+#define MAD_CMD_BASE		0x00
+#define HFI1_CMD_BASE		0xE0
 
-#define IB_USER_MAD_UNREGISTER_AGENT	_IOW(RDMA_IOCTL_MAGIC, 2, __u32)
+/* MAD specific section */
+#define MAD_CMD_REG_AGENT	(MAD_CMD_BASE + 0x01)
+#define MAD_CMD_UNREG_AGENT	(MAD_CMD_BASE + 0x02)
+#define MAD_CMD_ENABLE_PKEY	(MAD_CMD_BASE + 0x03)
+#define MAD_CMD_REG_AGENT2	(MAD_CMD_BASE + 0x04)
 
-#define IB_USER_MAD_ENABLE_PKEY		_IO(RDMA_IOCTL_MAGIC, 3)
-
-#define IB_USER_MAD_REGISTER_AGENT2     _IOWR(RDMA_IOCTL_MAGIC, 4, \
-					      struct ib_user_mad_reg_req2)
+#define IB_USER_MAD_REGISTER_AGENT \
+	_IOWR(RDMA_IOCTL_MAGIC, MAD_CMD_REG_AGENT, struct ib_user_mad_reg_req)
+#define IB_USER_MAD_UNREGISTER_AGENT \
+	_IOW(RDMA_IOCTL_MAGIC, MAD_CMD_UNREG_AGENT, __u32)
+#define IB_USER_MAD_ENABLE_PKEY \
+	_IO(RDMA_IOCTL_MAGIC, MAD_CMD_ENABLE_PKEY)
+#define IB_USER_MAD_REGISTER_AGENT2 \
+	_IOWR(RDMA_IOCTL_MAGIC, MAD_CMD_REG_AGENT2, struct ib_user_mad_reg_req2)
 
+/* HFI specific section */
 /* User commands. */
-#define HFI1_CMD_ASSIGN_CTXT     1	/* allocate HFI and context */
-#define HFI1_CMD_CTXT_INFO       2	/* find out what resources we got */
-#define HFI1_CMD_USER_INFO       3	/* set up userspace */
-#define HFI1_CMD_TID_UPDATE      4	/* update expected TID entries */
-#define HFI1_CMD_TID_FREE        5	/* free expected TID entries */
-#define HFI1_CMD_CREDIT_UPD      6	/* force an update of PIO credit */
+/* allocate HFI and context */
+#define HFI1_CMD_ASSIGN_CTXT     (HFI1_CMD_BASE + 0x01)
+/* find out what resources we got */
+#define HFI1_CMD_CTXT_INFO       (HFI1_CMD_BASE + 0x02)
+/* set up userspace */
+#define HFI1_CMD_USER_INFO       (HFI1_CMD_BASE + 0x03)
+/* update expected TID entries */
+#define HFI1_CMD_TID_UPDATE      (HFI1_CMD_BASE + 0x04)
+/* free expected TID entries */
+#define HFI1_CMD_TID_FREE        (HFI1_CMD_BASE + 0x05)
+/* force an update of PIO credit */
+#define HFI1_CMD_CREDIT_UPD      (HFI1_CMD_BASE + 0x06)
 
-#define HFI1_CMD_RECV_CTRL       8	/* control receipt of packets */
-#define HFI1_CMD_POLL_TYPE       9	/* set the kind of polling we want */
-#define HFI1_CMD_ACK_EVENT       10	/* ack & clear user status bits */
-#define HFI1_CMD_SET_PKEY        11     /* set context's pkey */
-#define HFI1_CMD_CTXT_RESET      12     /* reset context's HW send context */
-#define HFI1_CMD_TID_INVAL_READ  13     /* read TID cache invalidations */
-#define HFI1_CMD_GET_VERS	 14	/* get the version of the user cdev */
+/* control receipt of packets */
+#define HFI1_CMD_RECV_CTRL       (HFI1_CMD_BASE + 0x08)
+/* set the kind of polling we want */
+#define HFI1_CMD_POLL_TYPE       (HFI1_CMD_BASE + 0x09)
+/* ack & clear user status bits */
+#define HFI1_CMD_ACK_EVENT       (HFI1_CMD_BASE + 0x0A)
+/* set context's pkey */
+#define HFI1_CMD_SET_PKEY        (HFI1_CMD_BASE + 0x0B)
+/* reset context's HW send context */
+#define HFI1_CMD_CTXT_RESET      (HFI1_CMD_BASE + 0x0C)
+/* read TID cache invalidations */
+#define HFI1_CMD_TID_INVAL_READ  (HFI1_CMD_BASE + 0x0D)
+/* get the version of the user cdev */
+#define HFI1_CMD_GET_VERS	 (HFI1_CMD_BASE + 0x0E)
 
 /*
  * User IOCTLs can not go above 128 if they do then see common.h and change the
  * base for the snoop ioctl
  */
 
-/*
- * Make the ioctls occupy the last 0xf0-0xff portion of the IB range
- */
-#define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0)
-
 #define HFI1_IOCTL_ASSIGN_CTXT \
-	_IOWR(RDMA_IOCTL_MAGIC, __NUM(ASSIGN_CTXT), struct hfi1_user_info)
+	_IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_ASSIGN_CTXT, struct hfi1_user_info)
 #define HFI1_IOCTL_CTXT_INFO \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(CTXT_INFO), struct hfi1_ctxt_info)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_CTXT_INFO, struct hfi1_ctxt_info)
 #define HFI1_IOCTL_USER_INFO \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(USER_INFO), struct hfi1_base_info)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_USER_INFO, struct hfi1_base_info)
 #define HFI1_IOCTL_TID_UPDATE \
-	_IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_UPDATE), struct hfi1_tid_info)
+	_IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_UPDATE, struct hfi1_tid_info)
 #define HFI1_IOCTL_TID_FREE \
-	_IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_FREE), struct hfi1_tid_info)
+	_IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_FREE, struct hfi1_tid_info)
 #define HFI1_IOCTL_CREDIT_UPD \
-	_IO(RDMA_IOCTL_MAGIC, __NUM(CREDIT_UPD))
+	_IO(RDMA_IOCTL_MAGIC, HFI1_CMD_CREDIT_UPD)
 #define HFI1_IOCTL_RECV_CTRL \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(RECV_CTRL), int)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_RECV_CTRL, int)
 #define HFI1_IOCTL_POLL_TYPE \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(POLL_TYPE), int)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_POLL_TYPE, int)
 #define HFI1_IOCTL_ACK_EVENT \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(ACK_EVENT), unsigned long)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_ACK_EVENT, unsigned long)
 #define HFI1_IOCTL_SET_PKEY \
-	_IOW(RDMA_IOCTL_MAGIC, __NUM(SET_PKEY), __u16)
+	_IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_SET_PKEY, __u16)
 #define HFI1_IOCTL_CTXT_RESET \
-	_IO(RDMA_IOCTL_MAGIC, __NUM(CTXT_RESET))
+	_IO(RDMA_IOCTL_MAGIC, HFI1_CMD_CTXT_RESET)
 #define HFI1_IOCTL_TID_INVAL_READ \
-	_IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_INVAL_READ), struct hfi1_tid_info)
+	_IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_INVAL_READ, struct hfi1_tid_info)
 #define HFI1_IOCTL_GET_VERS \
-	_IOR(RDMA_IOCTL_MAGIC, __NUM(GET_VERS), int)
+	_IOR(RDMA_IOCTL_MAGIC, HFI1_CMD_GET_VERS, int)
 
 #endif /* RDMA_USER_IOCTL_H */