IB/hfi1: IOCTL cleanup and refactoring
Subsystems (2)
cover IB/hfi1: IOCTL cleanup and refactoring
Hi Doug,
Here is a set of patches that does some code rework to our IOCTL handling from Mike. I have bundled it as a separate series to make handling review feedback easier.
The HFI1 ioctl() function is long and complex. To reduce that complexity the following patch set refactors each IOCTL into a common pattern, and where necessary calls an associated function to handle the specific IOCTL.
Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15
---
Michael J. Ruhl (9):
IB/hfi1: Refactor assign_ctxt() IOCTL IB/hfi1: Refactor get_ctxt_info
IB/hfi1: Fix parenthesis alignment issues IB/hfi1: Refactor get_base_info
IB/hfi1: Refactor hfi_user_exp_rcv_setup() IOCTL IB/hfi1: Refactor hfi_user_exp_rcv_clear() IOCTLs IB/hfi1: Refactor hfi_user_exp_rcv_invalid() IOCTLs IB/hfi1: Refactor get_user() IOCTLs IB/hfi1: Refactor reset_ctxt() IOCTL
drivers/infiniband/hw/hfi1/file_ops.c | 463 +++++++++++++++++------------ drivers/infiniband/hw/hfi1/user_exp_rcv.c | 3 2 files changed, 272 insertions(+), 194 deletions(-)
--
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20170926140201.16637.45981.stgit-9QXIwq+3FY+1XWohqUldA0EOCMrvLtNR@public.gmane.org
PATCH 1/9 IB/hfi1: Refactor assign_ctxt() IOCTL
From: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern. Refactor the assign_ctxt() IOCTL.
--- drivers/infiniband/hw/hfi1/file_ops.c | 36 +++++++++++++++++---------------- 1 files changed, 19 insertions(+), 17 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +19 -17
@@ -78,7 +78,7 @@static int hfi1_file_mmap (struct file *fp,struct vm_area_struct *vma);static u64 kvirt_to_phys (void *addr); -static int assign_ctxt (struct hfi1_filedata *fd,struct hfi1_user_info *uinfo); +static int assign_ctxt (struct hfi1_filedata *fd,unsigned long arg,u32 len);static void init_subctxts (struct hfi1_ctxtdata *uctxt,const struct hfi1_user_info *uinfo); static int init_user_ctxt(struct hfi1_filedata *fd,
@@ -221,7 +221,6 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd, {struct hfi1_filedata *fd = fp->private_data;struct hfi1_ctxtdata *uctxt = fd->uctxt; -struct hfi1_user_info uinfo;struct hfi1_tid_info tinfo;int ret =0 ;unsigned long addr;
@@ -237,16 +236,9 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,switch (cmd) {case HFI1_IOCTL_ASSIGN_CTXT : -if (uctxt) -return -EINVAL ; - -if (copy_from_user (&uinfo, - (struct hfi1_user_info __user *)arg, -sizeof (uinfo))) -return -EFAULT ; - - ret =assign_ctxt (fd, &uinfo); + ret =assign_ctxt (fd, arg,_IOC_SIZE (cmd));break ; +case HFI1_IOCTL_CTXT_INFO : ret =get_ctxt_info (fd, (void __user *)(unsigned long )arg,sizeof (struct hfi1_ctxt_info ));
@@ -889,20 +881,30 @@ static int complete_subctxt(struct hfi1_filedata *fd)return ret; } -static int assign_ctxt (struct hfi1_filedata *fd,struct hfi1_user_info *uinfo) +static int assign_ctxt (struct hfi1_filedata *fd,unsigned long arg,u32 len) {int ret;unsigned int swmajor, swminor;struct hfi1_ctxtdata *uctxt =NULL ; +struct hfi1_user_info uinfo; + +if (fd->uctxt) +return -EINVAL ; + +if (sizeof (uinfo) != len) +return -EINVAL ; + +if (copy_from_user (&uinfo, (void __user *)arg,sizeof (uinfo))) +return -EFAULT ; - swmajor = uinfo->userversion >>16 ; + swmajor = uinfo.userversion >>16 ;if (swmajor !=HFI1_USER_SWMAJOR )return -ENODEV ; -if (uinfo->subctxt_cnt >HFI1_MAX_SHARED_CTXTS ) +if (uinfo.subctxt_cnt >HFI1_MAX_SHARED_CTXTS )return -EINVAL ; - swminor = uinfo->userversion &0xffff ; + swminor = uinfo.userversion &0xffff ;/* * Acquire the mutex to protect against multiple creations of what
@@ -913,14 +915,14 @@ static int assign_ctxt(struct hfi1_filedata *fd, struct hfi1_user_info *uinfo)* Get a sub context if available (fd->uctxt will be set). * ret < 0 error, 0 no context, 1 sub-context found */ - ret =find_sub_ctxt (fd, uinfo); + ret =find_sub_ctxt (fd, &uinfo);/* * Allocate a base context if context sharing is not required or a * sub context wasn't found. */ if (!ret) - ret =allocate_ctxt (fd, fd->dd, uinfo, &uctxt); + ret =allocate_ctxt (fd, fd->dd, &uinfo, &uctxt);mutex_unlock (&hfi1_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/9 IB/hfi1: Refactor get_ctxt_info
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern.
--- drivers/infiniband/hw/hfi1/file_ops.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +10 -10
@@ -84,8 +84,7 @@ static void init_subctxts(struct hfi1_ctxtdata *uctxt,static int init_user_ctxt (struct hfi1_filedata *fd,struct hfi1_ctxtdata *uctxt);static void user_init (struct hfi1_ctxtdata *uctxt); -static int get_ctxt_info (struct hfi1_filedata *fd,void __user *ubase, -__u32 len); +static int get_ctxt_info (struct hfi1_filedata *fd,unsigned long arg,u32 len);static int get_base_info (struct hfi1_filedata *fd,void __user *ubase,__u32 len); static int setup_base_ctxt(struct hfi1_filedata *fd,
@@ -240,9 +239,9 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,break ;case HFI1_IOCTL_CTXT_INFO : - ret =get_ctxt_info (fd, (void __user *)(unsigned long )arg, -sizeof (struct hfi1_ctxt_info )); + ret =get_ctxt_info (fd, arg,_IOC_SIZE (cmd));break ; +case HFI1_IOCTL_USER_INFO : ret =get_base_info (fd, (void __user *)(unsigned long )arg,sizeof (struct hfi1_base_info ));
@@ -1230,12 +1229,13 @@ static void user_init(struct hfi1_ctxtdata *uctxt)hfi1_rcvctrl (uctxt->dd, rcvctrl_ops, uctxt); } -static int get_ctxt_info (struct hfi1_filedata *fd,void __user *ubase, -__u32 len) +static int get_ctxt_info (struct hfi1_filedata *fd,unsigned long arg,u32 len) {struct hfi1_ctxt_info cinfo;struct hfi1_ctxtdata *uctxt = fd->uctxt; -int ret =0 ; + +if (sizeof (cinfo) != len) +return -EINVAL ;memset (&cinfo,0 ,sizeof (cinfo)); cinfo.runtime_flags = (((uctxt->flags >> HFI1_CAP_MISC_SHIFT) &
@@ -1265,10 +1265,10 @@ static int get_ctxt_info(struct hfi1_filedata *fd, void __user *ubase, cinfo.rcvegr_size = uctxt->egrbufs.rcvtid_size;trace_hfi1_ctxt_info (uctxt->dd, uctxt->ctxt, fd->subctxt, cinfo); -if (copy_to_user (ubase, &cinfo,sizeof (cinfo))) - ret = -EFAULT ; +if (copy_to_user ((void __user *)arg, &cinfo, len)) +return -EFAULT ; -return ret; +return 0 ; } static int init_user_ctxt(struct hfi1_filedata *fd,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/9 IB/hfi1: Fix parenthesis alignment issues
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
In preparation to refactoring get_base_info(), cleanup some checkpatch issues.
--- drivers/infiniband/hw/hfi1/file_ops.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +12 -12
@@ -1385,34 +1385,34 @@ static int get_base_info(struct hfi1_filedata *fd, void __user *ubase, fd->subctxt, uctxt->egrbufs.rcvtids[0].dma); binfo.sdma_comp_bufbase =HFI1_MMAP_TOKEN (SDMA_COMP , uctxt->ctxt, - fd->subctxt,0 ); + fd->subctxt,0 );/* * user regs are at * (RXE_PER_CONTEXT_USER + (ctxt * RXE_PER_CONTEXT_SIZE)) */ binfo.user_regbase =HFI1_MMAP_TOKEN (UREGS , uctxt->ctxt, - fd->subctxt,0 ); + fd->subctxt,0 ); offset =offset_in_page ((uctxt_offset (uctxt) + fd->subctxt) *sizeof (*dd->events)); binfo.events_bufbase =HFI1_MMAP_TOKEN (EVENTS , uctxt->ctxt, - fd->subctxt, - offset); + fd->subctxt, + offset); binfo.status_bufbase =HFI1_MMAP_TOKEN (STATUS , uctxt->ctxt, - fd->subctxt, - dd->status); + fd->subctxt, + dd->status);if (HFI1_CAP_IS_USET (DMA_RTAIL )) binfo.rcvhdrtail_base =HFI1_MMAP_TOKEN (RTAIL , uctxt->ctxt, - fd->subctxt,0 ); + fd->subctxt,0 );if (uctxt->subctxt_cnt) { binfo.subctxt_uregbase =HFI1_MMAP_TOKEN (SUBCTXT_UREGS , - uctxt->ctxt, - fd->subctxt,0 ); - binfo.subctxt_rcvhdrbuf =HFI1_MMAP_TOKEN (SUBCTXT_RCV_HDRQ , uctxt->ctxt, fd->subctxt,0 ); + binfo.subctxt_rcvhdrbuf =HFI1_MMAP_TOKEN (SUBCTXT_RCV_HDRQ , + uctxt->ctxt, + fd->subctxt,0 ); binfo.subctxt_rcvegrbuf =HFI1_MMAP_TOKEN (SUBCTXT_EGRBUF , - uctxt->ctxt, - fd->subctxt,0 ); + uctxt->ctxt, + fd->subctxt,0 ); } sz = (len <sizeof (binfo)) ? len :sizeof (binfo);if (copy_to_user (ubase, &binfo, sz))
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/9 IB/hfi1: Refactor get_base_info
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern.
--- drivers/infiniband/hw/hfi1/file_ops.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +12 -12
@@ -85,8 +85,7 @@ static int init_user_ctxt(struct hfi1_filedata *fd, struct hfi1_ctxtdata *uctxt);static void user_init (struct hfi1_ctxtdata *uctxt);static int get_ctxt_info (struct hfi1_filedata *fd,unsigned long arg,u32 len); -static int get_base_info (struct hfi1_filedata *fd,void __user *ubase, -__u32 len); +static int get_base_info (struct hfi1_filedata *fd,unsigned long arg,u32 len);static int setup_base_ctxt (struct hfi1_filedata *fd,struct hfi1_ctxtdata *uctxt);static int setup_subctxt (struct hfi1_ctxtdata *uctxt);
@@ -243,9 +242,9 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,break ;case HFI1_IOCTL_USER_INFO : - ret =get_base_info (fd, (void __user *)(unsigned long )arg, -sizeof (struct hfi1_base_info )); + ret =get_base_info (fd, arg,_IOC_SIZE (cmd));break ; +case HFI1_IOCTL_CREDIT_UPD :if (uctxt)sc_return_credits (uctxt->sc);
@@ -1344,18 +1343,18 @@ static int setup_base_ctxt(struct hfi1_filedata *fd,return ret; } -static int get_base_info (struct hfi1_filedata *fd,void __user *ubase, -__u32 len) +static int get_base_info (struct hfi1_filedata *fd,unsigned long arg,u32 len) {struct hfi1_base_info binfo;struct hfi1_ctxtdata *uctxt = fd->uctxt;struct hfi1_devdata *dd = uctxt->dd; -ssize_t sz;unsigned offset; -int ret =0 ;trace_hfi1_uctxtdata (uctxt->dd, uctxt, fd->subctxt); +if (sizeof (binfo) != len) +return -EINVAL ; +memset (&binfo,0 ,sizeof (binfo)); binfo.hw_version = dd->revision; binfo.sw_version =HFI1_KERN_SWVERSION ;
@@ -1414,10 +1413,11 @@ static int get_base_info(struct hfi1_filedata *fd, void __user *ubase, uctxt->ctxt, fd->subctxt, 0); } - sz = (len <sizeof (binfo)) ? len :sizeof (binfo); -if (copy_to_user (ubase, &binfo, sz)) - ret = -EFAULT ; -return ret; + +if (copy_to_user ((void __user *)arg, &binfo, len)) +return -EFAULT ; + +return 0 ; } static unsigned int poll_urgent(struct file *fp,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/9 IB/hfi1: Refactor hfi_user_exp_rcv_setup() IOCTL
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern. Refactor the _TID_UPDATE IOCTL.
--- drivers/infiniband/hw/hfi1/file_ops.c | 66 +++++++++++++++++++++++---------- 1 files changed, 45 insertions(+), 21 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +45 -21
@@ -86,6 +86,8 @@ static int init_user_ctxt(struct hfi1_filedata *fd,static void user_init (struct hfi1_ctxtdata *uctxt);static int get_ctxt_info (struct hfi1_filedata *fd,unsigned long arg,u32 len);static int get_base_info (struct hfi1_filedata *fd,unsigned long arg,u32 len); +static int user_exp_rcv_setup (struct hfi1_filedata *fd,unsigned long arg, +u32 len);static int setup_base_ctxt (struct hfi1_filedata *fd,struct hfi1_ctxtdata *uctxt);static int setup_subctxt (struct hfi1_ctxtdata *uctxt);
@@ -251,27 +253,7 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,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 (fd, &tinfo); -if (!ret) { -/* -* Copy the number of tidlist entries we used -* and the length of the buffer we registered. -*/ - addr = arg + offsetof(struct hfi1_tid_info , tidcnt); -if (copy_to_user ((void __user *)addr, &tinfo.tidcnt, -sizeof (tinfo.tidcnt))) -return -EFAULT ; - - addr = arg + offsetof(struct hfi1_tid_info , length); -if (copy_to_user ((void __user *)addr, &tinfo.length, -sizeof (tinfo.length))) - ret = -EFAULT ; - } + ret =user_exp_rcv_setup (fd, arg,_IOC_SIZE (cmd));break ;case HFI1_IOCTL_TID_FREE :
@@ -1420,6 +1402,48 @@ static int get_base_info(struct hfi1_filedata *fd, unsigned long arg, u32 len)return 0 ; } +/** +* user_exp_rcv_setup - Set up the given tid rcv list +* @fd: file data of the current driver instance +* @arg: ioctl argumnent for user space information +* @len: length of data structure associated with ioctl command +* +* Wrapper to validate ioctl information before doing _rcv_setup. +* +*/ +static int user_exp_rcv_setup (struct hfi1_filedata *fd,unsigned long arg, +u32 len) +{ +int ret; +unsigned long addr; +struct hfi1_tid_info tinfo; + +if (sizeof (tinfo) != len) +return -EINVAL ; + +if (copy_from_user (&tinfo, (void __user *)arg, (sizeof (tinfo)))) +return -EFAULT ; + + ret =hfi1_user_exp_rcv_setup (fd, &tinfo); +if (!ret) { +/* +* Copy the number of tidlist entries we used +* and the length of the buffer we registered. +*/ + addr = arg + offsetof(struct hfi1_tid_info , tidcnt); +if (copy_to_user ((void __user *)addr, &tinfo.tidcnt, +sizeof (tinfo.tidcnt))) +return -EFAULT ; + + addr = arg + offsetof(struct hfi1_tid_info , length); +if (copy_to_user ((void __user *)addr, &tinfo.length, +sizeof (tinfo.length))) + ret = -EFAULT ; + } + +return ret; +} +static unsigned int poll_urgent (struct file *fp,struct poll_table_struct *pt) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 6/9 IB/hfi1: Refactor hfi_user_exp_rcv_clear() IOCTLs
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern. Refactor the _TID_FREE IOCTL.
--- drivers/infiniband/hw/hfi1/file_ops.c | 49 +++++++++++++++++++++++++-------- 1 files changed, 37 insertions(+), 12 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +37 -12
@@ -88,6 +88,8 @@ static int init_user_ctxt(struct hfi1_filedata *fd,static int get_base_info (struct hfi1_filedata *fd,unsigned long arg,u32 len);static int user_exp_rcv_setup (struct hfi1_filedata *fd,unsigned long arg,u32 len); +static int user_exp_rcv_clear (struct hfi1_filedata *fd,unsigned long arg, +u32 len);static int setup_base_ctxt (struct hfi1_filedata *fd,struct hfi1_ctxtdata *uctxt);static int setup_subctxt (struct hfi1_ctxtdata *uctxt);
@@ -257,18 +259,7 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,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 (fd, &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 ; + ret =user_exp_rcv_clear (fd, arg,_IOC_SIZE (cmd));break ;case HFI1_IOCTL_TID_INVAL_READ :
@@ -1444,6 +1435,40 @@ static int user_exp_rcv_setup(struct hfi1_filedata *fd, unsigned long arg,return ret; } +/** +* user_exp_rcv_clear - Clear the given tid rcv list +* @fd: file data of the current driver instance +* @arg: ioctl argumnent for user space information +* @len: length of data structure associated with ioctl command +* +* The hfi1_user_exp_rcv_clear() can be called from the error path. Because +* of this, we need to use this wrapper to copy the user space information +* before doing the clear. +*/ +static int user_exp_rcv_clear (struct hfi1_filedata *fd,unsigned long arg, +u32 len) +{ +int ret; +unsigned long addr; +struct hfi1_tid_info tinfo; + +if (sizeof (tinfo) != len) +return -EINVAL ; + +if (copy_from_user (&tinfo, (void __user *)arg, (sizeof (tinfo)))) +return -EFAULT ; + + ret =hfi1_user_exp_rcv_clear (fd, &tinfo); +if (!ret) { + addr = arg + offsetof(struct hfi1_tid_info , tidcnt); +if (copy_to_user ((void __user *)addr, &tinfo.tidcnt, +sizeof (tinfo.tidcnt))) +return -EFAULT ; + } + +return ret; +} +static unsigned int poll_urgent (struct file *fp,struct poll_table_struct *pt) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 7/9 IB/hfi1: Refactor hfi_user_exp_rcv_invalid() IOCTLs
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern. Refactor _TID_INVAL_READ IOCTLs.
--- drivers/infiniband/hw/hfi1/file_ops.c | 54 +++++++++++++++++++++-------- drivers/infiniband/hw/hfi1/user_exp_rcv.c | 3 -- 2 files changed, 40 insertions(+), 17 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +40 -14
@@ -90,6 +90,8 @@ static int user_exp_rcv_setup(struct hfi1_filedata *fd, unsigned long arg, u32 len);static int user_exp_rcv_clear (struct hfi1_filedata *fd,unsigned long arg,u32 len); +static int user_exp_rcv_invalid (struct hfi1_filedata *fd,unsigned long arg, +u32 len);static int setup_base_ctxt (struct hfi1_filedata *fd,struct hfi1_ctxtdata *uctxt);static int setup_subctxt (struct hfi1_ctxtdata *uctxt);
@@ -223,9 +225,7 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd, {struct hfi1_filedata *fd = fp->private_data;struct hfi1_ctxtdata *uctxt = fd->uctxt; -struct hfi1_tid_info tinfo;int ret =0 ; -unsigned long addr;int uval =0 ;unsigned long ul_uval =0 ;u16 uval16 =0 ;
@@ -263,18 +263,7 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,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 (fd, &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 ; + ret =user_exp_rcv_invalid (fd, arg,_IOC_SIZE (cmd));break ;case HFI1_IOCTL_RECV_CTRL :
@@ -1469,6 +1458,43 @@ static int user_exp_rcv_clear(struct hfi1_filedata *fd, unsigned long arg,return ret; } +/** +* user_exp_rcv_invalid - Invalidate the given tid rcv list +* @fd: file data of the current driver instance +* @arg: ioctl argumnent for user space information +* @len: length of data structure associated with ioctl command +* +* Wrapper to validate ioctl information before doing _rcv_invalid. +* +*/ +static int user_exp_rcv_invalid (struct hfi1_filedata *fd,unsigned long arg, +u32 len) +{ +int ret; +unsigned long addr; +struct hfi1_tid_info tinfo; + +if (sizeof (tinfo) != len) +return -EINVAL ; + +if (!fd->invalid_tids) +return -EINVAL ; + +if (copy_from_user (&tinfo, (void __user *)arg, (sizeof (tinfo)))) +return -EFAULT ; + + ret =hfi1_user_exp_rcv_invalid (fd, &tinfo); +if (ret) +return ret; + + addr = arg + offsetof(struct hfi1_tid_info , tidcnt); +if (copy_to_user ((void __user *)addr, &tinfo.tidcnt, +sizeof (tinfo.tidcnt))) + ret = -EFAULT ; + +return ret; +} +static unsigned int poll_urgent (struct file *fp,struct poll_table_struct *pt) {
drivers/infiniband/hw/hfi1/user_exp_rcv.c +0 -3
@@ -546,9 +546,6 @@ int hfi1_user_exp_rcv_invalid(struct hfi1_filedata *fd,u32 *array;int ret =0 ; -if (!fd->invalid_tids) -return -EINVAL ; -/* * copy_to_user() can sleep, which will leave the invalid_lock * locked and cause the MMU notifier to be blocked on the lock
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 8/9 IB/hfi1: Refactor get_user() IOCTLs
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor to a common pattern. Refactor _RECV_CTRL, _POLL_TYPE, _ACK_EVENT and _SET_PKEY IOCTLs to a common pattern.
--- drivers/infiniband/hw/hfi1/file_ops.c | 74 +++++++++++++++------------------ 1 files changed, 34 insertions(+), 40 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +34 -40
@@ -105,10 +105,10 @@ static int allocate_ctxt(struct hfi1_filedata *fd, struct hfi1_devdata *dd,static unsigned int poll_urgent (struct file *fp,struct poll_table_struct *pt);static unsigned int poll_next (struct file *fp,struct poll_table_struct *pt);static int user_event_ack (struct hfi1_ctxtdata *uctxt,u16 subctxt, -unsigned long events); -static int set_ctxt_pkey (struct hfi1_ctxtdata *uctxt,u16 subctxt,u16 pkey); +unsigned long arg); +static int set_ctxt_pkey (struct hfi1_ctxtdata *uctxt,unsigned long arg);static int manage_rcvq (struct hfi1_ctxtdata *uctxt,u16 subctxt, -int start_stop); +unsigned long arg);static int vma_fault (struct vm_fault *vmf);static long hfi1_file_ioctl (struct file *fp,unsigned int cmd,unsigned long arg);
@@ -227,8 +227,6 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,struct hfi1_ctxtdata *uctxt = fd->uctxt;int ret =0 ;int uval =0 ; -unsigned long ul_uval =0 ; -u16 uval16 =0 ;hfi1_cdbg (IOCTL ,"IOCTL recv: 0x%x" , cmd); if (cmd != HFI1_IOCTL_ASSIGN_CTXT &&
@@ -267,34 +265,21 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd,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); + ret =manage_rcvq (uctxt, fd->subctxt, arg);break ;case HFI1_IOCTL_POLL_TYPE : - ret =get_user (uval, (int __user *)arg); -if (ret !=0 ) +if (get_user (uval, (int __user *)arg))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); + ret =user_event_ack (uctxt, fd->subctxt, arg);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 -return -EPERM ; + ret =set_ctxt_pkey (uctxt, arg);break ;case HFI1_IOCTL_CTXT_RESET : {
@@ -1587,13 +1572,18 @@ int hfi1_set_uevent_bits(struct hfi1_pportdata *ppd, const int evtbit)* re-init the software copy of the head register */ static int manage_rcvq (struct hfi1_ctxtdata *uctxt,u16 subctxt, -int start_stop) +unsigned long arg) {struct hfi1_devdata *dd = uctxt->dd;unsigned int rcvctrl_op; +int start_stop;if (subctxt) - goto bail; +return 0 ; + +if (get_user (start_stop, (int __user *)arg)) +return -EFAULT ; +/* atomically clear receive enable ctxt. */ if (start_stop) {/*
@@ -1612,7 +1602,7 @@ static int manage_rcvq(struct hfi1_ctxtdata *uctxt, u16 subctxt, }hfi1_rcvctrl (dd, rcvctrl_op, uctxt);/* always; new head should be equal to new tail; see above */ -bail: +return 0 ; }
@@ -1622,15 +1612,19 @@ static int manage_rcvq(struct hfi1_ctxtdata *uctxt, u16 subctxt,* set, if desired, and checks again in future. */ static int user_event_ack (struct hfi1_ctxtdata *uctxt,u16 subctxt, -unsigned long events) +unsigned long arg) {int i;struct hfi1_devdata *dd = uctxt->dd;unsigned long *evs; +unsigned long events;if (!dd->events)return 0 ; +if (get_user (events, (unsigned long __user *)arg)) +return -EFAULT ; + evs = dd->events +uctxt_offset (uctxt) + subctxt;for (i =0 ; i <= _HFI1_MAX_EVENT_BIT; i++) {
@@ -1641,27 +1635,27 @@ static int user_event_ack(struct hfi1_ctxtdata *uctxt, u16 subctxt,return 0 ; } -static int set_ctxt_pkey (struct hfi1_ctxtdata *uctxt,u16 subctxt,u16 pkey) +static int set_ctxt_pkey (struct hfi1_ctxtdata *uctxt,unsigned long arg) { -int ret = -ENOENT , i, intable =0 ; +int i;struct hfi1_pportdata *ppd = uctxt->ppd;struct hfi1_devdata *dd = uctxt->dd; +u16 pkey; -if (pkey ==LIM_MGMT_P_KEY || pkey ==FULL_MGMT_P_KEY ) { - ret = -EINVAL ; - goto done; - } +if (!HFI1_CAP_IS_USET (PKEY_CHECK )) +return -EPERM ; + +if (get_user (pkey, (u16 __user *)arg)) +return -EFAULT ; + +if (pkey ==LIM_MGMT_P_KEY || pkey ==FULL_MGMT_P_KEY ) +return -EINVAL ;for (i =0 ; i <ARRAY_SIZE (ppd->pkeys); i++) -if (pkey == ppd->pkeys[i]) { - intable =1 ; -break ; - } +if (pkey == ppd->pkeys[i]) +return hfi1_set_ctxt_pkey (dd, uctxt, pkey); -if (intable) - ret =hfi1_set_ctxt_pkey (dd, uctxt, pkey); -done: -return ret; +return -ENOENT ; }static void user_remove (struct hfi1_devdata *dd)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 9/9 IB/hfi1: Refactor reset_ctxt() IOCTL
In reply to a message not in the archiveFrom: Michael J. Ruhl <[email protected]>
The IOCTL is a bit unwieldy. Refactor reset_ctxt() to be a bit more manageable.
--- drivers/infiniband/hw/hfi1/file_ops.c | 122 ++++++++++++++++++--------------- 1 files changed, 66 insertions(+), 56 deletions(-)
drivers/infiniband/hw/hfi1/file_ops.c +66 -56
@@ -107,6 +107,7 @@ static int allocate_ctxt(struct hfi1_filedata *fd, struct hfi1_devdata *dd,static int user_event_ack (struct hfi1_ctxtdata *uctxt,u16 subctxt,unsigned long arg);static int set_ctxt_pkey (struct hfi1_ctxtdata *uctxt,unsigned long arg); +static int ctxt_reset (struct hfi1_ctxtdata *uctxt);static int manage_rcvq (struct hfi1_ctxtdata *uctxt,u16 subctxt,unsigned long arg);static int vma_fault (struct vm_fault *vmf);
@@ -282,63 +283,9 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd, ret =set_ctxt_pkey (uctxt, arg);break ; -case HFI1_IOCTL_CTXT_RESET : { -struct send_context *sc; -struct hfi1_devdata *dd; - -if (!uctxt || !uctxt->dd || !uctxt->sc) -return -EINVAL ; - -/* -* 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 )) -return -ENOLCK ; - -/* -* 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 ) -return -ENOLCK ; - -if (dd->flags &HFI1_FORCED_FREEZE ) -/* -* Don't allow context reset if we are into -* forced freeze -*/ -return -ENODEV ; - -sc_disable (sc); - ret =sc_enable (sc); -hfi1_rcvctrl (dd,HFI1_RCVCTRL_CTXT_ENB , uctxt); - }else { - ret =sc_restart (sc); - } -if (!ret) -sc_return_credits (sc); +case HFI1_IOCTL_CTXT_RESET : + ret =ctxt_reset (uctxt);break ; - }case HFI1_IOCTL_GET_VERS : uval =HFI1_USER_SWVERSION ;
@@ -1658,6 +1605,69 @@ static int set_ctxt_pkey(struct hfi1_ctxtdata *uctxt, unsigned long arg)return -ENOENT ; } +/** +* ctxt_reset - Reset the user context +* @uctxt: valid user context +*/ +static int ctxt_reset (struct hfi1_ctxtdata *uctxt) +{ +struct send_context *sc; +struct hfi1_devdata *dd; +int ret =0 ; + +if (!uctxt || !uctxt->dd || !uctxt->sc) +return -EINVAL ; + +/* +* 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 )) +return -ENOLCK ; + +/* +* 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, + !(READ_ONCE (dd->flags) &HFI1_FROZEN ), +msecs_to_jiffies (SEND_CTXT_HALT_TIMEOUT )); +if (dd->flags &HFI1_FROZEN ) +return -ENOLCK ; + +if (dd->flags &HFI1_FORCED_FREEZE ) +/* +* Don't allow context reset if we are into +* forced freeze +*/ +return -ENODEV ; + +sc_disable (sc); + ret =sc_enable (sc); +hfi1_rcvctrl (dd,HFI1_RCVCTRL_CTXT_ENB , uctxt); + }else { + ret =sc_restart (sc); + } +if (!ret) +sc_return_credits (sc); + +return ret; +} +static void user_remove (struct hfi1_devdata *dd) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 31 lines · Dennis Dalessandro
Hi Doug,
Here is a set of patches that does some code rework to our IOCTL handling from
Mike. I have bundled it as a separate series to make handling review feedback
easier.The HFI1
ioctl()function is long and complex. To reduce that complexity the
following patch set refactors each IOCTL into a common pattern, and where
necessary calls an associated function to handle the specific IOCTL.Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15---
Michael J. Ruhl (9):
IB/hfi1: Refactorassign_ctxt()IOCTL IB/hfi1: Refactorget_ctxt_info
IB/hfi1: Fix parenthesis alignment issues IB/hfi1: Refactorget_base_info
IB/hfi1: Refactorhfi_user_exp_rcv_setup()IOCTL IB/hfi1: Refactorhfi_user_exp_rcv_clear()IOCTLs IB/hfi1: Refactorhfi_user_exp_rcv_invalid()IOCTLs IB/hfi1: Refactorget_user()IOCTLs IB/hfi1: Refactorreset_ctxt()IOCTL
Quoted 4 lines
drivers/infiniband/hw/hfi1/file_ops.c | 463 +++++++++++++++++ ------------ drivers/infiniband/hw/hfi1/user_exp_rcv.c | 3 2 files changed, 272 insertions(+), 194 deletions(-)
Hi Denny,
These all look sane, pulling them in today, thanks.
Signature · 8 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html