[PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Dennis Dalessandro]

This patch series removes the write() interface for user access in favor of an
ioctl() based approach. This is in response to the complaint that we had
different handlers for write() and writev() doing different things and expecting
different types of data. See:

http://www.spinics.net/lists/linux-rdma/msg34451.html

In a response to that thread we mentioned some other possible approaches such
as using multiple files, or converting everything to writev(). However after
looking at things more it seemed a cleaner approach to use ioctl(). 

So each command that was being done through write() has been converted to
ioctl() and the write() interface is removed. We still use writev() for the data
path but control is done totally through ioctl().

The plan is to make the same sort of change in qib as well but we want to get
the opinion of the community on the approach first.

There is also a driver software version being exported via a sysfs file. This is
needed so that user space applications (psm) can  determine if it needs to do
ioctl() or write().

This patch series applies on Doug's k.o/for-4.7 branch.

Patches can also be viewed in my repo at:
https://github.com/ddalessa/kernel/tree/for-4.7

Changes since v1:
-----------------
* Dropped the last two patches in the series which created an eprom device and
consolidated ioctls.

* Handle conflict resolution, which removes the changes Jason did in
hfi1_file_write(), because it removes the function.

This does not add anything for compat_ioctl() as it is my understanding that
32 bit applications that attempt to call the ioctl() will just fail. Which is
the intended behavior. There is also a question of if we need to set 
kobj.parent [1], I'm not sure this is the case since the cdev in question lives
in /dev, not /dev/infiniband. If I'm incorrect in either of these aspects I can
spin another revision.

Other issues [1] raised by Jason are addressed in patches that follow this
series as they are comments on code which is not the subject of this patch
set.

[1] https://www.spinics.net/lists/linux-rdma/msg35319.html

---

Dennis Dalessandro (5):
      IB/hfi1: Export drivers user sw version via sysfs
      IB/hfi1: Remove unused user command
      IB/hfi1: Add ioctl() interface for user commands
      IB/hfi1: Remove write(), use ioctl() for user cmds
      IB/hfi1: Add trace message in user IOCTL handling


 drivers/staging/rdma/hfi1/common.h   |    6 +
 drivers/staging/rdma/hfi1/diag.c     |   13 --
 drivers/staging/rdma/hfi1/file_ops.c |  232 +++++++++++++++-------------------
 drivers/staging/rdma/hfi1/hfi.h      |   14 ++
 drivers/staging/rdma/hfi1/sysfs.c    |    8 +
 drivers/staging/rdma/hfi1/trace.c    |    1 
 drivers/staging/rdma/hfi1/trace.h    |    1 
 include/uapi/rdma/hfi/hfi1_user.h    |   51 +++++++
 8 files changed, 184 insertions(+), 142 deletions(-)

-- 
-Denny

[PATCH v2 1/5] IB/hfi1: Export drivers user sw version via sysfs

[Dennis Dalessandro]

User space consumers of hfi1 will need to know what version of the
driver they are dealing with. This becomes particularly important when
the write() interface is removed. User's will need to be able to query
the driver information but without asking the driver directly.

Add the driver's software version to sysfs for this exact purpose.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Mitko Haralanov <mitko.haralanov@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/staging/rdma/hfi1/sysfs.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/sysfs.c b/drivers/staging/rdma/hfi1/sysfs.c
index 8cd6df8..2ff5a14 100644
--- a/drivers/staging/rdma/hfi1/sysfs.c
+++ b/drivers/staging/rdma/hfi1/sysfs.c
@@ -622,6 +622,12 @@ static ssize_t show_tempsense(struct device *device,
 	return ret;
 }
 
+static ssize_t show_user_sw_version(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "0x%x\n", HFI1_USER_SWVERSION);
+}
+
 /*
  * end of per-unit (or driver, in some cases, but replicated
  * per unit) functions
@@ -636,6 +642,7 @@ static DEVICE_ATTR(serial, S_IRUGO, show_serial, NULL);
 static DEVICE_ATTR(boardversion, S_IRUGO, show_boardversion, NULL);
 static DEVICE_ATTR(tempsense, S_IRUGO, show_tempsense, NULL);
 static DEVICE_ATTR(chip_reset, S_IWUSR, NULL, store_chip_reset);
+static DEVICE_ATTR(user_sw_version, S_IRUGO, show_user_sw_version, NULL);
 
 static struct device_attribute *hfi1_attributes[] = {
 	&dev_attr_hw_rev,
@@ -646,6 +653,7 @@ static struct device_attribute *hfi1_attributes[] = {
 	&dev_attr_boardversion,
 	&dev_attr_tempsense,
 	&dev_attr_chip_reset,
+	&dev_attr_user_sw_version,
 };
 
 int hfi1_create_port_files(struct ib_device *ibdev, u8 port_num,

[PATCH v2 2/5] IB/hfi1: Remove unused user command

[Dennis Dalessandro]

The HFI1_CMD_SDMA_STATUS_UPD command was never implemented it has no
reason to live in the driver. Remove it.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Mitko Haralanov <mitko.haralanov@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/staging/rdma/hfi1/file_ops.c |    3 ---
 include/uapi/rdma/hfi/hfi1_user.h    |    1 -
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index c1c5bf8..c5be124 100644
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -215,7 +215,6 @@ static ssize_t hfi1_file_write(struct file *fp, const char __user *data,
 		copy = sizeof(uinfo);
 		dest = &uinfo;
 		break;
-	case HFI1_CMD_SDMA_STATUS_UPD:
 	case HFI1_CMD_CREDIT_UPD:
 		copy = 0;
 		break;
@@ -290,8 +289,6 @@ static ssize_t hfi1_file_write(struct file *fp, const char __user *data,
 		ret = get_base_info(fp, (void __user *)(unsigned long)
 				    user_val, cmd.len);
 		break;
-	case HFI1_CMD_SDMA_STATUS_UPD:
-		break;
 	case HFI1_CMD_CREDIT_UPD:
 		if (uctxt && uctxt->sc)
 			sc_return_credits(uctxt->sc);
diff --git a/include/uapi/rdma/hfi/hfi1_user.h b/include/uapi/rdma/hfi/hfi1_user.h
index a533cec..46f6caa 100644
--- a/include/uapi/rdma/hfi/hfi1_user.h
+++ b/include/uapi/rdma/hfi/hfi1_user.h
@@ -127,7 +127,6 @@
 #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 */
-#define HFI1_CMD_SDMA_STATUS_UPD 7      /* force update of SDMA status ring */
 
 #define HFI1_CMD_RECV_CTRL       8	/* control receipt of packets */
 #define HFI1_CMD_POLL_TYPE       9	/* set the kind of polling we want */

[PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Dennis Dalessandro]

IOCTL is more suited to what user space commands need to do than the
write() interface. Add IOCTL definitions for all existing write commands
and the handling for those. The write() interface will be removed in a
follow on patch.

Reviewed-by: Mitko Haralanov <mitko.haralanov@intel.com>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/staging/rdma/hfi1/common.h   |    6 +
 drivers/staging/rdma/hfi1/diag.c     |   13 --
 drivers/staging/rdma/hfi1/file_ops.c |  224 ++++++++++++++++++++++++++++++++++
 drivers/staging/rdma/hfi1/hfi.h      |   14 ++
 include/uapi/rdma/hfi/hfi1_user.h    |   48 +++++++
 5 files changed, 294 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/common.h b/drivers/staging/rdma/hfi1/common.h
index e9b6bb3..37ac229 100644
--- a/drivers/staging/rdma/hfi1/common.h
+++ b/drivers/staging/rdma/hfi1/common.h
@@ -178,7 +178,8 @@
 		     HFI1_CAP_PKEY_CHECK |		\
 		     HFI1_CAP_NO_INTEGRITY)
 
-#define HFI1_USER_SWVERSION ((HFI1_USER_SWMAJOR << 16) | HFI1_USER_SWMINOR)
+#define HFI1_USER_SWVERSION ((HFI1_USER_SWMAJOR << HFI1_SWMAJOR_SHIFT) | \
+			     HFI1_USER_SWMINOR)
 
 #ifndef HFI1_KERN_TYPE
 #define HFI1_KERN_TYPE 0
@@ -349,6 +350,9 @@ struct hfi1_message_header {
 #define HFI1_BECN_MASK 1
 #define HFI1_BECN_SMASK BIT(HFI1_BECN_SHIFT)
 
+#define HFI1_PSM_IOC_BASE_SEQ 0x0
+#define HFI1_SNOOP_IOC_BASE_SEQ 0x80 /* leaves plenty of room for psm */
+
 static inline __u64 rhf_to_cpu(const __le32 *rbuf)
 {
 	return __le64_to_cpu(*((__le64 *)rbuf));
diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index bb2409a..4cf6e8d 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -168,9 +168,7 @@ struct hfi1_link_info {
  */
 #define SNOOP_CAPTURE_VERSION 0x1
 
-#define IB_IOCTL_MAGIC          0x1b /* See Documentation/ioctl-number.txt */
 #define HFI1_SNOOP_IOC_MAGIC IB_IOCTL_MAGIC
-#define HFI1_SNOOP_IOC_BASE_SEQ 0x80
 
 #define HFI1_SNOOP_IOCGETLINKSTATE \
 	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ)
@@ -1040,21 +1038,16 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 	struct hfi1_pportdata *ppd = NULL;
 	unsigned int index;
 	struct hfi1_link_info link_info;
-	int read_cmd, write_cmd, read_ok, write_ok;
 
 	dd = hfi1_dd_from_sc_inode(fp->f_inode);
 	if (!dd)
 		return -ENODEV;
 
-	mode_flag = dd->hfi1_snoop.mode_flag;
-	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
-	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
-	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
-	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
-
-	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
+	if (check_ioctl_access(cmd, arg))
 		return -EFAULT;
 
+	mode_flag = dd->hfi1_snoop.mode_flag;
+
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index c5be124..b4172d4 100644
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -97,6 +97,8 @@ static int user_event_ack(struct hfi1_ctxtdata *, int, unsigned long);
 static int set_ctxt_pkey(struct hfi1_ctxtdata *, unsigned, u16);
 static int manage_rcvq(struct hfi1_ctxtdata *, unsigned, int);
 static int vma_fault(struct vm_area_struct *, struct vm_fault *);
+static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,
+			    unsigned long arg);
 
 static const struct file_operations hfi1_file_ops = {
 	.owner = THIS_MODULE,
@@ -104,6 +106,7 @@ static const struct file_operations hfi1_file_ops = {
 	.write_iter = hfi1_write_iter,
 	.open = hfi1_file_open,
 	.release = hfi1_file_close,
+	.unlocked_ioctl = hfi1_file_ioctl,
 	.poll = hfi1_poll,
 	.mmap = hfi1_file_mmap,
 	.llseek = noop_llseek,
@@ -176,6 +179,227 @@ static int hfi1_file_open(struct inode *inode, struct file *fp)
 	return fp->private_data ? 0 : -ENOMEM;
 }
 
+static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct hfi1_filedata *fd = fp->private_data;
+	struct hfi1_ctxtdata *uctxt = fd->uctxt;
+	struct hfi1_user_info uinfo;
+	struct hfi1_tid_info tinfo;
+	struct hfi1_cmd ucmd;
+	int uctxt_required = 1;
+	int ret = 0;
+	unsigned long addr;
+	int uval = 0;
+	unsigned long ul_uval = 0;
+	u16 uval16 = 0;
+
+	ret = check_ioctl_access(cmd, arg);
+	if (ret)
+		return ret;
+
+	switch (cmd) {
+	case HFI1_IOCTL_ASSIGN_CTXT:
+		uctxt_required = 0; /* assigned user context not required */
+		break;
+	case HFI1_IOCTL_EP_INFO:
+	case HFI1_IOCTL_EP_ERASE_CHIP:
+	case HFI1_IOCTL_EP_ERASE_RANGE:
+	case HFI1_IOCTL_EP_READ_RANGE:
+	case HFI1_IOCTL_EP_WRITE_RANGE:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		if (copy_from_user(&ucmd,
+				   (struct hfi11_cmd __user *)arg,
+				   sizeof(ucmd)))
+			return -EFAULT;
+		return handle_eprom_command(fp, &ucmd);
+	}
+
+	if (uctxt_required && !uctxt)
+		return -EINVAL;
+
+	/* Checked for root/context process the IOCTL */
+	switch (cmd) {
+	case HFI1_IOCTL_ASSIGN_CTXT:
+		if (copy_from_user(&uinfo,
+				   (struct hfi1_user_info __user *)arg,
+				   sizeof(uinfo)))
+			return -EFAULT;
+
+		ret = assign_ctxt(fp, &uinfo);
+		if (ret < 0)
+			return ret;
+		setup_ctxt(fp);
+		if (ret)
+			return ret;
+		ret = user_init(fp);
+		break;
+	case HFI1_IOCTL_CTXT_INFO:
+		ret = get_ctxt_info(fp, (void __user *)(unsigned long)arg,
+				    sizeof(struct hfi1_ctxt_info));
+		break;
+	case HFI1_IOCTL_USER_INFO:
+		ret = get_base_info(fp, (void __user *)(unsigned long)arg,
+				    sizeof(struct hfi1_base_info));
+		break;
+	case HFI1_IOCTL_CREDIT_UPD:
+		if (uctxt && uctxt->sc)
+			sc_return_credits(uctxt->sc);
+		break;
+
+	case HFI1_IOCTL_TID_UPDATE:
+		if (copy_from_user(&tinfo,
+				   (struct hfi11_tid_info __user *)arg,
+				   sizeof(tinfo)))
+			return -EFAULT;
+
+		ret = hfi1_user_exp_rcv_setup(fp, &tinfo);
+		if (!ret) {
+			/*
+			 * Copy the number of tidlist entries we used
+			 * and the length of the buffer we registered.
+			 * These fields are adjacent in the structure so
+			 * we can copy them at the same time.
+			 */
+			addr = arg + offsetof(struct hfi1_tid_info, tidcnt);
+			if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
+					 sizeof(tinfo.tidcnt) +
+					 sizeof(tinfo.length)))
+				ret = -EFAULT;
+		}
+		break;
+
+	case HFI1_IOCTL_TID_FREE:
+		if (copy_from_user(&tinfo,
+				   (struct hfi11_tid_info __user *)arg,
+				   sizeof(tinfo)))
+			return -EFAULT;
+
+		ret = hfi1_user_exp_rcv_clear(fp, &tinfo);
+		if (ret)
+			break;
+		addr = arg + offsetof(struct hfi1_tid_info, tidcnt);
+		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
+				 sizeof(tinfo.tidcnt)))
+			ret = -EFAULT;
+		break;
+
+	case HFI1_IOCTL_TID_INVAL_READ:
+		if (copy_from_user(&tinfo,
+				   (struct hfi11_tid_info __user *)arg,
+				   sizeof(tinfo)))
+			return -EFAULT;
+
+		ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
+		if (ret)
+			break;
+		addr = arg + offsetof(struct hfi1_tid_info, tidcnt);
+		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
+				 sizeof(tinfo.tidcnt)))
+			ret = -EFAULT;
+		break;
+
+	case HFI1_IOCTL_RECV_CTRL:
+		ret = __get_user(uval, (int __user *)arg);
+		if (ret != 0)
+			return -EFAULT;
+		ret = manage_rcvq(uctxt, fd->subctxt, uval);
+		break;
+
+	case HFI1_IOCTL_POLL_TYPE:
+		ret = __get_user(uval, (int __user *)arg);
+		if (ret != 0)
+			return -EFAULT;
+		uctxt->poll_type = (typeof(uctxt->poll_type))uval;
+		break;
+
+	case HFI1_IOCTL_ACK_EVENT:
+		ret = __get_user(ul_uval, (unsigned long __user *)arg);
+		if (ret != 0)
+			return -EFAULT;
+		ret = user_event_ack(uctxt, fd->subctxt, ul_uval);
+		break;
+
+	case HFI1_IOCTL_SET_PKEY:
+		ret = __get_user(uval16, (u16 __user *)arg);
+		if (ret != 0)
+			return -EFAULT;
+		if (HFI1_CAP_IS_USET(PKEY_CHECK))
+			ret = set_ctxt_pkey(uctxt, fd->subctxt, uval16);
+		else
+			ret = -EPERM;
+		break;
+
+	case HFI1_IOCTL_CTXT_RESET: {
+		struct send_context *sc;
+		struct hfi1_devdata *dd;
+
+		if (!uctxt || !uctxt->dd || !uctxt->sc) {
+			ret = -EINVAL;
+			break;
+		}
+		/*
+		 * There is no protection here. User level has to
+		 * guarantee that no one will be writing to the send
+		 * context while it is being re-initialized.
+		 * If user level breaks that guarantee, it will break
+		 * it's own context and no one else's.
+		 */
+		dd = uctxt->dd;
+		sc = uctxt->sc;
+		/*
+		 * Wait until the interrupt handler has marked the
+		 * context as halted or frozen. Report error if we time
+		 * out.
+		 */
+		wait_event_interruptible_timeout(
+			sc->halt_wait, (sc->flags & SCF_HALTED),
+			msecs_to_jiffies(SEND_CTXT_HALT_TIMEOUT));
+		if (!(sc->flags & SCF_HALTED)) {
+			ret = -ENOLCK;
+			break;
+		}
+		/*
+		 * If the send context was halted due to a Freeze,
+		 * wait until the device has been "unfrozen" before
+		 * resetting the context.
+		 */
+		if (sc->flags & SCF_FROZEN) {
+			wait_event_interruptible_timeout(
+				dd->event_queue,
+				!(ACCESS_ONCE(dd->flags) & HFI1_FROZEN),
+				msecs_to_jiffies(SEND_CTXT_HALT_TIMEOUT));
+			if (dd->flags & HFI1_FROZEN) {
+				ret = -ENOLCK;
+				break;
+			}
+			if (dd->flags & HFI1_FORCED_FREEZE) {
+				/*
+				 * Don't allow context reset if we are into
+				 * forced freeze
+				 */
+				ret = -ENODEV;
+				break;
+			}
+			sc_disable(sc);
+			ret = sc_enable(sc);
+			hfi1_rcvctrl(dd, HFI1_RCVCTRL_CTXT_ENB,
+				     uctxt->ctxt);
+		} else {
+			ret = sc_restart(sc);
+		}
+		if (!ret)
+			sc_return_credits(sc);
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static ssize_t hfi1_file_write(struct file *fp, const char __user *data,
 			       size_t count, loff_t *offset)
 {
diff --git a/drivers/staging/rdma/hfi1/hfi.h b/drivers/staging/rdma/hfi1/hfi.h
index 7b78d56..7351898 100644
--- a/drivers/staging/rdma/hfi1/hfi.h
+++ b/drivers/staging/rdma/hfi1/hfi.h
@@ -1946,4 +1946,18 @@ static inline u32 qsfp_resource(struct hfi1_devdata *dd)
 
 int hfi1_tempsense_rd(struct hfi1_devdata *dd, struct hfi1_temp *temp);
 
+static inline int check_ioctl_access(unsigned int cmd, unsigned long arg)
+{
+	int read_cmd, write_cmd, read_ok, write_ok;
+
+	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
+	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
+	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
+	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
+
+	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
+		return -EFAULT;
+
+	return 0;
+}
 #endif                          /* _HFI1_KERNEL_H */
diff --git a/include/uapi/rdma/hfi/hfi1_user.h b/include/uapi/rdma/hfi/hfi1_user.h
index 46f6caa..37a9697 100644
--- a/include/uapi/rdma/hfi/hfi1_user.h
+++ b/include/uapi/rdma/hfi/hfi1_user.h
@@ -78,6 +78,11 @@
 #define HFI1_USER_SWMINOR 0
 
 /*
+ * We will encode the major/minor inside a single 32bit version number.
+ */
+#define HFI1_SWMAJOR_SHIFT 16
+
+/*
  * Set of HW and driver capability/feature bits.
  * These bit values are used to configure enabled/disabled HW and
  * driver features. The same set of bits are communicated to user
@@ -142,6 +147,48 @@
 #define HFI1_CMD_EP_READ_RANGE   76      /* read EPROM range */
 #define HFI1_CMD_EP_WRITE_RANGE  77      /* write EPROM range */
 
+/*
+ * User IOCTLs can not go above 128 if they do then see common.h and change the
+ * base for the snoop ioctl
+ */
+#define IB_IOCTL_MAGIC 0x1b /* See Documentation/ioctl/ioctl-number.txt */
+
+struct hfi1_cmd;
+#define HFI1_IOCTL_ASSIGN_CTXT \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_ASSIGN_CTXT, struct hfi1_user_info)
+#define HFI1_IOCTL_CTXT_INFO \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_CTXT_INFO, struct hfi1_ctxt_info)
+#define HFI1_IOCTL_USER_INFO \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_USER_INFO, struct hfi1_base_info)
+#define HFI1_IOCTL_TID_UPDATE \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_TID_UPDATE, struct hfi1_tid_info)
+#define HFI1_IOCTL_TID_FREE \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_TID_FREE, struct hfi1_tid_info)
+#define HFI1_IOCTL_CREDIT_UPD \
+	_IO(IB_IOCTL_MAGIC, HFI1_CMD_CREDIT_UPD)
+#define HFI1_IOCTL_RECV_CTRL \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_RECV_CTRL, int)
+#define HFI1_IOCTL_POLL_TYPE \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_POLL_TYPE, int)
+#define HFI1_IOCTL_ACK_EVENT \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_ACK_EVENT, unsigned long)
+#define HFI1_IOCTL_SET_PKEY \
+	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_SET_PKEY, __u16)
+#define HFI1_IOCTL_CTXT_RESET \
+	_IO(IB_IOCTL_MAGIC, HFI1_CMD_CTXT_RESET)
+#define HFI1_IOCTL_TID_INVAL_READ \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_TID_INVAL_READ, struct hfi1_tid_info)
+#define HFI1_IOCTL_EP_INFO \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_EP_INFO, struct hfi1_cmd)
+#define HFI1_IOCTL_EP_ERASE_CHIP \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_EP_ERASE_CHIP, struct hfi1_cmd)
+#define HFI1_IOCTL_EP_ERASE_RANGE \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_EP_ERASE_RANGE, struct hfi1_cmd)
+#define HFI1_IOCTL_EP_READ_RANGE \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_EP_READ_RANGE, struct hfi1_cmd)
+#define HFI1_IOCTL_EP_WRITE_RANGE \
+	_IOWR(IB_IOCTL_MAGIC, HFI1_CMD_EP_WRITE_RANGE, struct hfi1_cmd)
+
 #define _HFI1_EVENT_FROZEN_BIT         0
 #define _HFI1_EVENT_LINKDOWN_BIT       1
 #define _HFI1_EVENT_LID_CHANGE_BIT     2
@@ -242,6 +289,7 @@ struct hfi1_tid_info {
 	__u32 length;
 };
 
+/* hfi1_cmd is used for EPROM commands only */
 struct hfi1_cmd {
 	__u32 type;        /* command type */
 	__u32 len;         /* length of struct pointed to by add */

[PATCH v2 4/5] IB/hfi1: Remove write(), use ioctl() for user cmds

[Dennis Dalessandro]

Remove the write() handler for user space commands now that ioctl
handling is available. User apps will need to change to use ioctl from
this point forward.

Reviewed-by: Mitko Haralanov <mitko.haralanov@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/staging/rdma/hfi1/file_ops.c |  249 ----------------------------------
 include/uapi/rdma/hfi/hfi1_user.h    |    2 
 2 files changed, 1 insertions(+), 250 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index b4172d4..120b680 100644
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -72,8 +72,6 @@
  */
 static int hfi1_file_open(struct inode *, struct file *);
 static int hfi1_file_close(struct inode *, struct file *);
-static ssize_t hfi1_file_write(struct file *, const char __user *,
-			       size_t, loff_t *);
 static ssize_t hfi1_write_iter(struct kiocb *, struct iov_iter *);
 static unsigned int hfi1_poll(struct file *, struct poll_table_struct *);
 static int hfi1_file_mmap(struct file *, struct vm_area_struct *);
@@ -102,7 +100,6 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,
 
 static const struct file_operations hfi1_file_ops = {
 	.owner = THIS_MODULE,
-	.write = hfi1_file_write,
 	.write_iter = hfi1_write_iter,
 	.open = hfi1_file_open,
 	.release = hfi1_file_close,
@@ -400,252 +397,6 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,
 	return ret;
 }
 
-static ssize_t hfi1_file_write(struct file *fp, const char __user *data,
-			       size_t count, loff_t *offset)
-{
-	const struct hfi1_cmd __user *ucmd;
-	struct hfi1_filedata *fd = fp->private_data;
-	struct hfi1_ctxtdata *uctxt = fd->uctxt;
-	struct hfi1_cmd cmd;
-	struct hfi1_user_info uinfo;
-	struct hfi1_tid_info tinfo;
-	unsigned long addr;
-	ssize_t consumed = 0, copy = 0, ret = 0;
-	void *dest = NULL;
-	__u64 user_val = 0;
-	int uctxt_required = 1;
-	int must_be_root = 0;
-
-	/* FIXME: This interface cannot continue out of staging */
-	if (WARN_ON_ONCE(!ib_safe_file_access(fp)))
-		return -EACCES;
-
-	if (count < sizeof(cmd)) {
-		ret = -EINVAL;
-		goto bail;
-	}
-
-	ucmd = (const struct hfi1_cmd __user *)data;
-	if (copy_from_user(&cmd, ucmd, sizeof(cmd))) {
-		ret = -EFAULT;
-		goto bail;
-	}
-
-	consumed = sizeof(cmd);
-
-	switch (cmd.type) {
-	case HFI1_CMD_ASSIGN_CTXT:
-		uctxt_required = 0;	/* assigned user context not required */
-		copy = sizeof(uinfo);
-		dest = &uinfo;
-		break;
-	case HFI1_CMD_CREDIT_UPD:
-		copy = 0;
-		break;
-	case HFI1_CMD_TID_UPDATE:
-	case HFI1_CMD_TID_FREE:
-	case HFI1_CMD_TID_INVAL_READ:
-		copy = sizeof(tinfo);
-		dest = &tinfo;
-		break;
-	case HFI1_CMD_USER_INFO:
-	case HFI1_CMD_RECV_CTRL:
-	case HFI1_CMD_POLL_TYPE:
-	case HFI1_CMD_ACK_EVENT:
-	case HFI1_CMD_CTXT_INFO:
-	case HFI1_CMD_SET_PKEY:
-	case HFI1_CMD_CTXT_RESET:
-		copy = 0;
-		user_val = cmd.addr;
-		break;
-	case HFI1_CMD_EP_INFO:
-	case HFI1_CMD_EP_ERASE_CHIP:
-	case HFI1_CMD_EP_ERASE_RANGE:
-	case HFI1_CMD_EP_READ_RANGE:
-	case HFI1_CMD_EP_WRITE_RANGE:
-		uctxt_required = 0;	/* assigned user context not required */
-		must_be_root = 1;	/* validate user */
-		copy = 0;
-		break;
-	default:
-		ret = -EINVAL;
-		goto bail;
-	}
-
-	/* If the command comes with user data, copy it. */
-	if (copy) {
-		if (copy_from_user(dest, (void __user *)cmd.addr, copy)) {
-			ret = -EFAULT;
-			goto bail;
-		}
-		consumed += copy;
-	}
-
-	/*
-	 * Make sure there is a uctxt when needed.
-	 */
-	if (uctxt_required && !uctxt) {
-		ret = -EINVAL;
-		goto bail;
-	}
-
-	/* only root can do these operations */
-	if (must_be_root && !capable(CAP_SYS_ADMIN)) {
-		ret = -EPERM;
-		goto bail;
-	}
-
-	switch (cmd.type) {
-	case HFI1_CMD_ASSIGN_CTXT:
-		ret = assign_ctxt(fp, &uinfo);
-		if (ret < 0)
-			goto bail;
-		ret = setup_ctxt(fp);
-		if (ret)
-			goto bail;
-		ret = user_init(fp);
-		break;
-	case HFI1_CMD_CTXT_INFO:
-		ret = get_ctxt_info(fp, (void __user *)(unsigned long)
-				    user_val, cmd.len);
-		break;
-	case HFI1_CMD_USER_INFO:
-		ret = get_base_info(fp, (void __user *)(unsigned long)
-				    user_val, cmd.len);
-		break;
-	case HFI1_CMD_CREDIT_UPD:
-		if (uctxt && uctxt->sc)
-			sc_return_credits(uctxt->sc);
-		break;
-	case HFI1_CMD_TID_UPDATE:
-		ret = hfi1_user_exp_rcv_setup(fp, &tinfo);
-		if (!ret) {
-			/*
-			 * Copy the number of tidlist entries we used
-			 * and the length of the buffer we registered.
-			 * These fields are adjacent in the structure so
-			 * we can copy them at the same time.
-			 */
-			addr = (unsigned long)cmd.addr +
-				offsetof(struct hfi1_tid_info, tidcnt);
-			if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
-					 sizeof(tinfo.tidcnt) +
-					 sizeof(tinfo.length)))
-				ret = -EFAULT;
-		}
-		break;
-	case HFI1_CMD_TID_INVAL_READ:
-		ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
-		if (ret)
-			break;
-		addr = (unsigned long)cmd.addr +
-			offsetof(struct hfi1_tid_info, tidcnt);
-		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
-				 sizeof(tinfo.tidcnt)))
-			ret = -EFAULT;
-		break;
-	case HFI1_CMD_TID_FREE:
-		ret = hfi1_user_exp_rcv_clear(fp, &tinfo);
-		if (ret)
-			break;
-		addr = (unsigned long)cmd.addr +
-			offsetof(struct hfi1_tid_info, tidcnt);
-		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
-				 sizeof(tinfo.tidcnt)))
-			ret = -EFAULT;
-		break;
-	case HFI1_CMD_RECV_CTRL:
-		ret = manage_rcvq(uctxt, fd->subctxt, (int)user_val);
-		break;
-	case HFI1_CMD_POLL_TYPE:
-		uctxt->poll_type = (typeof(uctxt->poll_type))user_val;
-		break;
-	case HFI1_CMD_ACK_EVENT:
-		ret = user_event_ack(uctxt, fd->subctxt, user_val);
-		break;
-	case HFI1_CMD_SET_PKEY:
-		if (HFI1_CAP_IS_USET(PKEY_CHECK))
-			ret = set_ctxt_pkey(uctxt, fd->subctxt, user_val);
-		else
-			ret = -EPERM;
-		break;
-	case HFI1_CMD_CTXT_RESET: {
-		struct send_context *sc;
-		struct hfi1_devdata *dd;
-
-		if (!uctxt || !uctxt->dd || !uctxt->sc) {
-			ret = -EINVAL;
-			break;
-		}
-		/*
-		 * There is no protection here. User level has to
-		 * guarantee that no one will be writing to the send
-		 * context while it is being re-initialized.
-		 * If user level breaks that guarantee, it will break
-		 * it's own context and no one else's.
-		 */
-		dd = uctxt->dd;
-		sc = uctxt->sc;
-		/*
-		 * Wait until the interrupt handler has marked the
-		 * context as halted or frozen. Report error if we time
-		 * out.
-		 */
-		wait_event_interruptible_timeout(
-			sc->halt_wait, (sc->flags & SCF_HALTED),
-			msecs_to_jiffies(SEND_CTXT_HALT_TIMEOUT));
-		if (!(sc->flags & SCF_HALTED)) {
-			ret = -ENOLCK;
-			break;
-		}
-		/*
-		 * If the send context was halted due to a Freeze,
-		 * wait until the device has been "unfrozen" before
-		 * resetting the context.
-		 */
-		if (sc->flags & SCF_FROZEN) {
-			wait_event_interruptible_timeout(
-				dd->event_queue,
-				!(ACCESS_ONCE(dd->flags) & HFI1_FROZEN),
-				msecs_to_jiffies(SEND_CTXT_HALT_TIMEOUT));
-			if (dd->flags & HFI1_FROZEN) {
-				ret = -ENOLCK;
-				break;
-			}
-			if (dd->flags & HFI1_FORCED_FREEZE) {
-				/*
-				 * Don't allow context reset if we are into
-				 * forced freeze
-				 */
-				ret = -ENODEV;
-				break;
-			}
-			sc_disable(sc);
-			ret = sc_enable(sc);
-			hfi1_rcvctrl(dd, HFI1_RCVCTRL_CTXT_ENB,
-				     uctxt->ctxt);
-		} else {
-			ret = sc_restart(sc);
-		}
-		if (!ret)
-			sc_return_credits(sc);
-		break;
-	}
-	case HFI1_CMD_EP_INFO:
-	case HFI1_CMD_EP_ERASE_CHIP:
-	case HFI1_CMD_EP_ERASE_RANGE:
-	case HFI1_CMD_EP_READ_RANGE:
-	case HFI1_CMD_EP_WRITE_RANGE:
-		ret = handle_eprom_command(fp, &cmd);
-		break;
-	}
-
-	if (ret >= 0)
-		ret = consumed;
-bail:
-	return ret;
-}
-
 static ssize_t hfi1_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 {
 	struct hfi1_filedata *fd = kiocb->ki_filp->private_data;
diff --git a/include/uapi/rdma/hfi/hfi1_user.h b/include/uapi/rdma/hfi/hfi1_user.h
index 37a9697..383d036 100644
--- a/include/uapi/rdma/hfi/hfi1_user.h
+++ b/include/uapi/rdma/hfi/hfi1_user.h
@@ -66,7 +66,7 @@
  * The major version changes when data structures change in an incompatible
  * way. The driver must be the same for initialization to succeed.
  */
-#define HFI1_USER_SWMAJOR 5
+#define HFI1_USER_SWMAJOR 6
 
 /*
  * Minor version differences are always compatible

[PATCH v2 5/5] IB/hfi1: Add trace message in user IOCTL handling

[Dennis Dalessandro]

Add a trace message to HFI1s user IOCTL handling. This allows debugging
of which IOCTLs are being handled by the driver.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/staging/rdma/hfi1/file_ops.c |    2 ++
 drivers/staging/rdma/hfi1/trace.c    |    1 +
 drivers/staging/rdma/hfi1/trace.h    |    1 +
 3 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index 120b680..bfcf1a2 100644
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -195,6 +195,8 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,
 	if (ret)
 		return ret;
 
+	hfi1_cdbg(IOCTL, "IOCTL recv: 0x%x", cmd);
+
 	switch (cmd) {
 	case HFI1_IOCTL_ASSIGN_CTXT:
 		uctxt_required = 0; /* assigned user context not required */
diff --git a/drivers/staging/rdma/hfi1/trace.c b/drivers/staging/rdma/hfi1/trace.c
index 8b62fef..caddb2a 100644
--- a/drivers/staging/rdma/hfi1/trace.c
+++ b/drivers/staging/rdma/hfi1/trace.c
@@ -233,3 +233,4 @@ __hfi1_trace_fn(FIRMWARE);
 __hfi1_trace_fn(RCVCTRL);
 __hfi1_trace_fn(TID);
 __hfi1_trace_fn(MMU);
+__hfi1_trace_fn(IOCTL);
diff --git a/drivers/staging/rdma/hfi1/trace.h b/drivers/staging/rdma/hfi1/trace.h
index 963dc94..3cb0391 100644
--- a/drivers/staging/rdma/hfi1/trace.h
+++ b/drivers/staging/rdma/hfi1/trace.h
@@ -1341,6 +1341,7 @@ __hfi1_trace_def(FIRMWARE);
 __hfi1_trace_def(RCVCTRL);
 __hfi1_trace_def(TID);
 __hfi1_trace_def(MMU);
+__hfi1_trace_def(IOCTL);
 
 #define hfi1_cdbg(which, fmt, ...) \
 	__hfi1_trace_##which(__func__, fmt, ##__VA_ARGS__)

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Jason Gunthorpe]

On Thu, May 12, 2016 at 10:18:27AM -0700, Dennis Dalessandro wrote:

> There is also a driver software version being exported via a sysfs
> file. This is needed so that user space applications (psm) can
> determine if it needs to do ioctl() or write().

Why? Don't do this, just call ioctl() and if it fails then use write().

> This does not add anything for compat_ioctl() as it is my understanding that
> 32 bit applications that attempt to call the ioctl() will just fail. Which is
> the intended behavior.

qib works fine with a 64 bit kernel and 32 bit user space, don't
break it.

> There is also a question of if we need to set kobj.parent [1], I'm

It is needed, you need to audit this stuff, there might be more wrong
than that.

> not sure this is the case since the cdev in question lives
> in /dev, not /dev/infiniband.

Irrelevant. kobj.parent is needed because the cdev is embedded within
another reference counted structure. You need to consider how all this
works when the driver is removed while the cdev is still open (or
driver remove is racing with the cdev release).

Jason

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Jason Gunthorpe]

On Thu, May 12, 2016 at 10:18:47AM -0700, Dennis Dalessandro wrote:
> +	case HFI1_IOCTL_EP_INFO:
> +	case HFI1_IOCTL_EP_ERASE_CHIP:
> +	case HFI1_IOCTL_EP_ERASE_RANGE:
> +	case HFI1_IOCTL_EP_READ_RANGE:
> +	case HFI1_IOCTL_EP_WRITE_RANGE:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +		if (copy_from_user(&ucmd,
> +				   (struct hfi11_cmd __user *)arg,
> +				   sizeof(ucmd)))
> +			return -EFAULT;
> +		return handle_eprom_command(fp, &ucmd);

I thought we agreed to get rid of this as well? It certainly does not
belong here, and as a general rule, I don't think ioctls should be
doing capable tests..

> +static inline int check_ioctl_access(unsigned int cmd, unsigned long arg)
> +{
> +	int read_cmd, write_cmd, read_ok, write_ok;
> +
> +	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
> +	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
> +	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
> +	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> +
> +	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
> +		return -EFAULT;

This seems kind of goofy, didn't Ira say this is performance senstive?

Driver shouldn't be open coding __get_user like that, IMHO.

> +#define HFI1_IOCTL_RECV_CTRL \
> +	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_RECV_CTRL, int)

Have you audited this? Confused why this is marked IOW when I see
this:

+       case HFI1_IOCTL_RECV_CTRL:
+               ret = __get_user(uval, (int __user *)arg);

Seeing many other examples.

I stopped looking again

Jason

RE: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Hefty, Sean]

> On Thu, May 12, 2016 at 10:18:47AM -0700, Dennis Dalessandro wrote:
> > +	case HFI1_IOCTL_EP_INFO:
> > +	case HFI1_IOCTL_EP_ERASE_CHIP:
> > +	case HFI1_IOCTL_EP_ERASE_RANGE:
> > +	case HFI1_IOCTL_EP_READ_RANGE:
> > +	case HFI1_IOCTL_EP_WRITE_RANGE:
> > +		if (!capable(CAP_SYS_ADMIN))
> > +			return -EPERM;
> > +		if (copy_from_user(&ucmd,
> > +				   (struct hfi11_cmd __user *)arg,
> > +				   sizeof(ucmd)))
> > +			return -EFAULT;
> > +		return handle_eprom_command(fp, &ucmd);
> 
> I thought we agreed to get rid of this as well? It certainly does not
> belong here, and as a general rule, I don't think ioctls should be
> doing capable tests..

At least the drm ioctl code has similar capable test

http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_ioctl.c#L519


> > +static inline int check_ioctl_access(unsigned int cmd, unsigned long
> arg)
> > +{
> > +	int read_cmd, write_cmd, read_ok, write_ok;
> > +
> > +	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
> > +	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
> > +	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg,
> _IOC_SIZE(cmd));
> > +	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> > +
> > +	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
> > +		return -EFAULT;
> 
> This seems kind of goofy, didn't Ira say this is performance senstive?
> 
> Driver shouldn't be open coding __get_user like that, IMHO.

FWIW, drm keeps an ioctl 'descriptor', which maintains a kernel copy of the ioctl cmd.  It uses the kernel's version of the cmd for processing, instead of the cmd value passed in from user space.

It doesn't open code get_user or do checks similar to what's here.  But if there's concern that the cmd value cannot be trusted, a similar descriptor mechanism could be used here.

- Sean

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Dennis Dalessandro]

On Thu, May 12, 2016 at 11:34:45AM -0600, Jason Gunthorpe wrote:
>On Thu, May 12, 2016 at 10:18:27AM -0700, Dennis Dalessandro wrote:
>
>> There is also a driver software version being exported via a sysfs
>> file. This is needed so that user space applications (psm) can
>> determine if it needs to do ioctl() or write().
>
>Why? Don't do this, just call ioctl() and if it fails then use write().

Is it really that big of a deal to export a version number?

There are cases where psm could be built with a different kernel (depends on  
a header file) than the currently running one.  This allows psm to check the 
current running version with what it was built against. This could be useful 
for other scenarios in the future as well. Not just the write vs ioctl.

>> This does not add anything for compat_ioctl() as it is my understanding that
>> 32 bit applications that attempt to call the ioctl() will just fail. Which is
>> the intended behavior.
>
>qib works fine with a 64 bit kernel and 32 bit user space, don't
>break it.

We will handle this issue when we get to the qib patches. I want to hammer 
out what the solution for hfi1 looks like first.

>> There is also a question of if we need to set kobj.parent [1], I'm
>
>It is needed, you need to audit this stuff, there might be more wrong
>than that.

>> not sure this is the case since the cdev in question lives
>> in /dev, not /dev/infiniband.
>
>Irrelevant. kobj.parent is needed because the cdev is embedded within
>another reference counted structure. You need to consider how all this
>works when the driver is removed while the cdev is still open (or
>driver remove is racing with the cdev release).

The driver can't be removed while the cdev is still open. I tested with a
test code that opens /dev/hfi1_0 and spins. The use count as reported by 
lsmod ticks up and the driver can not be unloaded until I ctrl+c the test 
program.

Are you saying the driver's cdev release function could race with the 
drivers moudle_exit()? I don't think this happens. I put a sleep in the 
release function and ctrl+c my test program and it hangs until the sleep 
expires.  I tried to rmmod the driver (during the window)  and it fails as 
in use.

-Denny

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Jason Gunthorpe]

On Thu, May 12, 2016 at 03:07:38PM -0400, Dennis Dalessandro wrote:
> >>There is also a driver software version being exported via a sysfs
> >>file. This is needed so that user space applications (psm) can
> >>determine if it needs to do ioctl() or write().
> >
> >Why? Don't do this, just call ioctl() and if it fails then use write().
> 
> Is it really that big of a deal to export a version number?

If it isn't needed, don't add it..

> >another reference counted structure. You need to consider how all this
> >works when the driver is removed while the cdev is still open (or
> >driver remove is racing with the cdev release).
> 
> The driver can't be removed while the cdev is still open. I tested with a
> test code that opens /dev/hfi1_0 and spins. The use count as reported by
> lsmod ticks up and the driver can not be unloaded until I ctrl+c the test
> program.

Drivers can be removed in other ways, eg pci hot unplug. Do not assume
module_exit is the only way and rely on module ref counting for
correctness.

Jason

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Dennis Dalessandro]

On Thu, May 12, 2016 at 11:43:32AM -0600, Jason Gunthorpe wrote:
>On Thu, May 12, 2016 at 10:18:47AM -0700, Dennis Dalessandro wrote:
>> +	case HFI1_IOCTL_EP_INFO:
>> +	case HFI1_IOCTL_EP_ERASE_CHIP:
>> +	case HFI1_IOCTL_EP_ERASE_RANGE:
>> +	case HFI1_IOCTL_EP_READ_RANGE:
>> +	case HFI1_IOCTL_EP_WRITE_RANGE:
>> +		if (!capable(CAP_SYS_ADMIN))
>> +			return -EPERM;
>> +		if (copy_from_user(&ucmd,
>> +				   (struct hfi11_cmd __user *)arg,
>> +				   sizeof(ucmd)))
>> +			return -EFAULT;
>> +		return handle_eprom_command(fp, &ucmd);
>
>I thought we agreed to get rid of this as well? It certainly does not
>belong here, and as a general rule, I don't think ioctls should be
>doing capable tests..

Yeah. I left it in this patch set because this just "ports" our existing 
code to ioctl(). The eprom stuff is completely removed in another patch set 
that I posted shortly after this. It's at:

http://marc.info/?l=linux-rdma&m=146307409301822&w=2

>> +static inline int check_ioctl_access(unsigned int cmd, unsigned long arg)
>> +{
>> +	int read_cmd, write_cmd, read_ok, write_ok;
>> +
>> +	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
>> +	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
>> +	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
>> +	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
>> +
>> +	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
>> +		return -EFAULT;
>
>This seems kind of goofy, didn't Ira say this is performance senstive?

I'll let Ira comment here on the performance aspect. I agree it looks goofy.  
Suggestion on how to make it look better? Or are you saying this is 
incorrect?

>Driver shouldn't be open coding __get_user like that, IMHO.

Can you explain what you mean here? We should not use __get_user()?

>
>> +#define HFI1_IOCTL_RECV_CTRL \
>> +	_IOW(IB_IOCTL_MAGIC, HFI1_CMD_RECV_CTRL, int)
>
>Have you audited this? Confused why this is marked IOW when I see
>this:
>
>+       case HFI1_IOCTL_RECV_CTRL:
>+               ret = __get_user(uval, (int __user *)arg);
>
>Seeing many other examples.
>
>I stopped looking again

_IOW means user is writing data to the device. So the device is reading data 
from the user. Or am I missing your point?

-Denny

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Jason Gunthorpe]

On Thu, May 12, 2016 at 03:27:27PM -0400, Dennis Dalessandro wrote:
> >I thought we agreed to get rid of this as well? It certainly does not
> >belong here, and as a general rule, I don't think ioctls should be
> >doing capable tests..
> 
> Yeah. I left it in this patch set because this just "ports" our existing
> code to ioctl(). The eprom stuff is completely removed in another patch set
> that I posted shortly after this. It's at:

Adding code and then removing it in a later patch is not a best
practice.. Just don't add it or define ioctl numbers at all..

> >>+static inline int check_ioctl_access(unsigned int cmd, unsigned long arg)
> >>+{
> >>+	int read_cmd, write_cmd, read_ok, write_ok;
> >>+
> >>+	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
> >>+	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
> >>+	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
> >>+	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> >>+
> >>+	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
> >>+		return -EFAULT;
> >
> >This seems kind of goofy, didn't Ira say this is performance senstive?

Well, calling access_ok twice when only once is typically needed is
certainly not performant. Typically this check is done at every access
via get_user/put_user/copy_to/from_user - why is it being hoisted like
this?

> >Driver shouldn't be open coding __get_user like that, IMHO.
> 
> Can you explain what you mean here? We should not use __get_user()?

Generally speaking, yes. Use get_user() that includes the correct
access_ok. Unless there is a good reason to avoid it, the standard API
should be used.

> _IOW means user is writing data to the device. So the device is reading data
> from the user. Or am I missing your point?

You are right, I spaced on this when reading the above - 'write_ok'
and 'write_cmd' seem like they should have been related, but really
aren't. It is doing the right tests, just odd. (eg use names like
write_cmd_ok, write_cmd for better clarity)

Jason

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

On 05/12/2016 03:40 PM, Jason Gunthorpe wrote:
> On Thu, May 12, 2016 at 03:27:27PM -0400, Dennis Dalessandro wrote:
>>> I thought we agreed to get rid of this as well? It certainly does not
>>> belong here, and as a general rule, I don't think ioctls should be
>>> doing capable tests..
>>
>> Yeah. I left it in this patch set because this just "ports" our existing
>> code to ioctl(). The eprom stuff is completely removed in another patch set
>> that I posted shortly after this. It's at:
> 
> Adding code and then removing it in a later patch is not a best
> practice.. Just don't add it or define ioctl numbers at all..

Yeah, but then that changes the nature of the patchset.  It goes from
being "We ported the existing write API to ioctl API" to "We ported
existing write API to ioctl API and also changed the scope of the API in
the process", which is considered bad coding practice (one stand alone
change per commit).  It's pretty 6 of one, half dozen of the other if
you ask me.  A better solution would have been to remove the EEPROM
stuff from the write ioctl, then only port the remaining stuff.  That
would have avoided both coding issues, but I also understand how they
got here, which is they did the ioctl conversion before they knew they
were going to rip out the EEPROM code.  In any case, the best fix would
be to rebase the two series that are remaining and move any "rip out
things like eeprom support" patches to prior to the ioctl patches and
make it so that they rip out the write interface version of it instead,
and then squash a second copy of the ioctl removal into this patch.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Dennis Dalessandro]

On Thu, May 12, 2016 at 01:25:08PM -0600, Jason Gunthorpe wrote:
>On Thu, May 12, 2016 at 03:07:38PM -0400, Dennis Dalessandro wrote:
>> >>There is also a driver software version being exported via a sysfs
>> >>file. This is needed so that user space applications (psm) can
>> >>determine if it needs to do ioctl() or write().
>> >
>> >Why? Don't do this, just call ioctl() and if it fails then use write().
>> 
>> Is it really that big of a deal to export a version number?
>
>If it isn't needed, don't add it..

For the reason I gave, I think it is needed so unless you are vehemently 
opposed to it I would prefer to leave it.

>> >another reference counted structure. You need to consider how all this
>> >works when the driver is removed while the cdev is still open (or
>> >driver remove is racing with the cdev release).
>> 
>> The driver can't be removed while the cdev is still open. I tested with a
>> test code that opens /dev/hfi1_0 and spins. The use count as reported by
>> lsmod ticks up and the driver can not be unloaded until I ctrl+c the test
>> program.
>
>Drivers can be removed in other ways, eg pci hot unplug. Do not assume
>module_exit is the only way and rely on module ref counting for
>correctness.

Point taken. I'll look into this. So are you perhaps suggesting we do 
something like is done for uverbs_dev in ib_verbs_add_one() where there is a 
kobj for uverbs_dev and the parent of uverbs_dev->cdeb is set to that? In 
our case it would probably be something like hfi1_devdata.

-Denny

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On 05/12/2016 03:53 PM, Dennis Dalessandro wrote:
> On Thu, May 12, 2016 at 01:25:08PM -0600, Jason Gunthorpe wrote:
>> On Thu, May 12, 2016 at 03:07:38PM -0400, Dennis Dalessandro wrote:
>>> >>There is also a driver software version being exported via a sysfs
>>> >>file. This is needed so that user space applications (psm) can
>>> >>determine if it needs to do ioctl() or write().
>>> >
>>> >Why? Don't do this, just call ioctl() and if it fails then use write().
>>>
>>> Is it really that big of a deal to export a version number?
>>
>> If it isn't needed, don't add it..
> 
> For the reason I gave, I think it is needed so unless you are vehemently
> opposed to it I would prefer to leave it.

I think Jason has a point here.  In particular, if you create an ioctl
to return a driver version, it is probably safe that all future hfi1/psm
user space code can try the ioctl, in the absence of the ioctl working
assume version 0, and in the current and all future versions of the
driver, get a valid return, including potential updates to the version
number, so it's completely possible to use this as an ongoing test of
more than just "is this pre-release code or release code".


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/5] IB/hfi1: Remove write() and use ioctl() for user access

[Jason Gunthorpe]

On Thu, May 12, 2016 at 03:53:04PM -0400, Dennis Dalessandro wrote:
> >>Is it really that big of a deal to export a version number?
> >
> >If it isn't needed, don't add it..
> 
> For the reason I gave, I think it is needed so unless you are vehemently
> opposed to it I would prefer to leave it.

It represents the opposite of what we want to see in good uapi
design. Linux generally doesn't use versioning, it uses active in-band
discovery (eg via ENOSYS or something)

If you do something that needs versioning then add it, otherwise
assume a design using in-band discovery.

> >Drivers can be removed in other ways, eg pci hot unplug. Do not assume
> >module_exit is the only way and rely on module ref counting for
> >correctness.
> 
> Point taken. I'll look into this. So are you perhaps suggesting we do
> something like is done for uverbs_dev in ib_verbs_add_one() where there is a
> kobj for uverbs_dev and the parent of uverbs_dev->cdeb is set to that? In
> our case it would probably be something like hfi1_devdata.

Unconditionally the cdev parent kref must point to the kref that holds
the liftime of the memory containing the struct cdev.

See

commit 35d4a0b63dc0c6d1177d4f532a9deae958f0662c
Author: Yishai Hadas <yishaih@mellanox.com>
Date:   Thu Aug 13 18:32:03 2015 +0300

    IB/uverbs: Fix race between ib_uverbs_open and remove_one

But I thought there might be more issues with release racing with
remove as some of the release functions were quite complex, and I
didn't see much locking (I did not study it closely). It needs a
careful analysis to show that is OK.  Remember unregistering the cdev
is not a fence and open fd's can remain and be released at any time.

Jason

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Jason Gunthorpe]

On Thu, May 12, 2016 at 03:48:16PM -0400, Doug Ledford wrote:
> were going to rip out the EEPROM code.  In any case, the best fix would
> be to rebase the two series that are remaining and move any "rip out
> things like eeprom support" patches to prior to the ioctl patches and
> make it so that they rip out the write interface version of it instead,
> and then squash a second copy of the ioctl removal into this patch.

Yes, this would look best

Jason

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Dennis Dalessandro]

On Thu, May 12, 2016 at 03:28:21PM -0600, Jason Gunthorpe wrote:
>On Thu, May 12, 2016 at 03:48:16PM -0400, Doug Ledford wrote:
>> were going to rip out the EEPROM code.  In any case, the best fix would
>> be to rebase the two series that are remaining and move any "rip out
>> things like eeprom support" patches to prior to the ioctl patches and
>> make it so that they rip out the write interface version of it instead,
>> and then squash a second copy of the ioctl removal into this patch.
>
>Yes, this would look best

The end result will end up being the exact same code so I don't have a 
problem doing that. 

Perhaps it would be better if I just combine the two patch sets into one 
series? I still need to look more into the kobj stuff about the cdev, if 
needed I'll add a patch for that as well.

-Denny

Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[ira.weiny]

On Thu, May 12, 2016 at 01:40:06PM -0600, Jason Gunthorpe wrote:
> On Thu, May 12, 2016 at 03:27:27PM -0400, Dennis Dalessandro wrote:
> 
> > >>+static inline int check_ioctl_access(unsigned int cmd, unsigned long arg)
> > >>+{
> > >>+	int read_cmd, write_cmd, read_ok, write_ok;
> > >>+
> > >>+	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
> > >>+	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
> > >>+	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd));
> > >>+	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> > >>+
> > >>+	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
> > >>+		return -EFAULT;
> > >
> > >This seems kind of goofy, didn't Ira say this is performance senstive?
> 
> Well, calling access_ok twice when only once is typically needed is
> certainly not performant. Typically this check is done at every access
> via get_user/put_user/copy_to/from_user - why is it being hoisted like
> this?
>
> 
> > >Driver shouldn't be open coding __get_user like that, IMHO.
> > 
> > Can you explain what you mean here? We should not use __get_user()?
> 
> Generally speaking, yes. Use get_user() that includes the correct
> access_ok. Unless there is a good reason to avoid it, the standard API
> should be used.

I know this code was refactored while we were still submitting patches to Greg
KH back in Nov/Dec.  Part of this was cleaning up branch on error rather than
success.  Hence the check for access at the top of the function and early
return.

At that time I _thought_ there were multiple __get_users in some of the
operations so a single common access_ok would speed those up.  However, I don't
see that happening any longer, so either I don't remember correctly, or we have
made this cleaner.

As it stands now I think you are correct we could use get_user and
copy_to/from_user.

Ira