[PATCH 0/8] Fix hfi1_ioctl locking

[PATCH 0/8] Fix hfi1_ioctl locking

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

It was identified that hfi1_ioctl may sleep with a spin lock held.

This was identified publicly here:

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

As well as by our internal development.

This series cleans up the code and parameter checks, as well as fixing the
locking.


Dennis Dalessandro (2):
  staging/rdma/hfi1: Reduce snoop locking scope in IOCTL handler.
  staging/rdma/hfi1: Return immediately on error

Ira Weiny (5):
  staging/rdma/hfi1: Fix camel case variables
  staging/rdma/hfi1: Return early from hfi1_ioctl parameter errors
  staging/rdma/hfi1: remove unneeded goto done
  staging/rdma/hfi1: return early if setlink state was specified
  staging/rdma/hfi1: Further clean up hfi1_ioctl parameter checks

Jubin John (1):
  staging/rdma/hfi1: diag.c Correct code style issues

 drivers/staging/rdma/hfi1/diag.c | 465 +++++++++++++++++++--------------------
 1 file changed, 223 insertions(+), 242 deletions(-)

-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/8] staging/rdma/hfi1: Fix camel case variables

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

physState, linkState, and devState should be phys_state, link_state, and
dev_state

Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/staging/rdma/hfi1/diag.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index c5cc2db1b559..c2839473f983 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -964,9 +964,9 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 	void *filter_value = NULL;
 	long ret = 0;
 	int value = 0;
-	u8 physState = 0;
-	u8 linkState = 0;
-	u16 devState = 0;
+	u8 phys_state = 0;
+	u8 link_state = 0;
+	u16 dev_state = 0;
 	unsigned long flags = 0;
 	unsigned long *argp = NULL;
 	struct hfi1_packet_filter_command filter_cmd = {0};
@@ -1030,31 +1030,31 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 			}
 
 			/* What we want to transition to */
-			physState = (value >> 4) & 0xF;
-			linkState = value & 0xF;
+			phys_state = (value >> 4) & 0xF;
+			link_state = value & 0xF;
 			snoop_dbg("Setting link state 0x%x", value);
 
-			switch (linkState) {
+			switch (link_state) {
 			case IB_PORT_NOP:
-				if (physState == 0)
+				if (phys_state == 0)
 					break;
 					/* fall through */
 			case IB_PORT_DOWN:
-				switch (physState) {
+				switch (phys_state) {
 				case 0:
-					devState = HLS_DN_DOWNDEF;
+					dev_state = HLS_DN_DOWNDEF;
 					break;
 				case 2:
-					devState = HLS_DN_POLL;
+					dev_state = HLS_DN_POLL;
 					break;
 				case 3:
-					devState = HLS_DN_DISABLE;
+					dev_state = HLS_DN_DISABLE;
 					break;
 				default:
 					ret = -EINVAL;
 					goto done;
 				}
-				ret = set_link_state(ppd, devState);
+				ret = set_link_state(ppd, dev_state);
 				break;
 			case IB_PORT_ARMED:
 				ret = set_link_state(ppd, HLS_UP_ARMED);
-- 
1.8.2

[PATCH 3/8] staging/rdma/hfi1: Return early from hfi1_ioctl parameter errors

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Rather than have a switch in a large else clause make the parameter checks
return immediately.

Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/staging/rdma/hfi1/diag.c | 348 +++++++++++++++++++--------------------
 1 file changed, 173 insertions(+), 175 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index c2839473f983..2bb857b2a103 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -979,17 +979,15 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 	if (dd == NULL)
 		return -ENODEV;
 
-	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
-
 	mode_flag = dd->hfi1_snoop.mode_flag;
 
 	if (((_IOC_DIR(cmd) & _IOC_READ)
 	    && !access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd)))
 	    || ((_IOC_DIR(cmd) & _IOC_WRITE)
 	    && !access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd)))) {
-		ret = -EFAULT;
+		return -EFAULT;
 	} else if (!capable(CAP_SYS_ADMIN)) {
-		ret = -EPERM;
+		return -EPERM;
 	} else if ((mode_flag & HFI1_PORT_CAPTURE_MODE) &&
 		   (cmd != HFI1_SNOOP_IOCCLEARQUEUE) &&
 		   (cmd != HFI1_SNOOP_IOCCLEARFILTER) &&
@@ -1000,208 +998,208 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		 * 3.Set capture filter
 		 * Other are invalid.
 		 */
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
+
+	switch (cmd) {
+	case HFI1_SNOOP_IOCSETLINKSTATE:
+		snoop_dbg("HFI1_SNOOP_IOCSETLINKSTATE is not valid");
 		ret = -EINVAL;
-	} else {
-		switch (cmd) {
-		case HFI1_SNOOP_IOCSETLINKSTATE:
-			snoop_dbg("HFI1_SNOOP_IOCSETLINKSTATE is not valid");
+		break;
+
+	case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA:
+		memset(&link_info, 0, sizeof(link_info));
+
+		if (copy_from_user(&link_info,
+				   (struct hfi1_link_info __user *)arg,
+				   sizeof(link_info)))
+			ret = -EFAULT;
+
+		value = link_info.port_state;
+		index = link_info.port_number;
+		if (index > dd->num_pports - 1) {
 			ret = -EINVAL;
 			break;
+		}
 
-		case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA:
-			memset(&link_info, 0, sizeof(link_info));
+		ppd = &dd->pport[index];
+		if (!ppd) {
+			ret = -EINVAL;
+			break;
+		}
 
-			if (copy_from_user(&link_info,
-				(struct hfi1_link_info __user *)arg,
-				sizeof(link_info)))
-				ret = -EFAULT;
+		/* What we want to transition to */
+		phys_state = (value >> 4) & 0xF;
+		link_state = value & 0xF;
+		snoop_dbg("Setting link state 0x%x", value);
 
-			value = link_info.port_state;
-			index = link_info.port_number;
-			if (index > dd->num_pports - 1) {
-				ret = -EINVAL;
+		switch (link_state) {
+		case IB_PORT_NOP:
+			if (phys_state == 0)
 				break;
-			}
-
-			ppd = &dd->pport[index];
-			if (!ppd) {
-				ret = -EINVAL;
-				break;
-			}
-
-			/* What we want to transition to */
-			phys_state = (value >> 4) & 0xF;
-			link_state = value & 0xF;
-			snoop_dbg("Setting link state 0x%x", value);
-
-			switch (link_state) {
-			case IB_PORT_NOP:
-				if (phys_state == 0)
-					break;
-					/* fall through */
-			case IB_PORT_DOWN:
-				switch (phys_state) {
-				case 0:
-					dev_state = HLS_DN_DOWNDEF;
-					break;
-				case 2:
-					dev_state = HLS_DN_POLL;
-					break;
-				case 3:
-					dev_state = HLS_DN_DISABLE;
-					break;
-				default:
-					ret = -EINVAL;
-					goto done;
-				}
-				ret = set_link_state(ppd, dev_state);
+				/* fall through */
+		case IB_PORT_DOWN:
+			switch (phys_state) {
+			case 0:
+				dev_state = HLS_DN_DOWNDEF;
 				break;
-			case IB_PORT_ARMED:
-				ret = set_link_state(ppd, HLS_UP_ARMED);
-				if (!ret)
-					send_idle_sma(dd, SMA_IDLE_ARM);
+			case 2:
+				dev_state = HLS_DN_POLL;
 				break;
-			case IB_PORT_ACTIVE:
-				ret = set_link_state(ppd, HLS_UP_ACTIVE);
-				if (!ret)
-					send_idle_sma(dd, SMA_IDLE_ACTIVE);
+			case 3:
+				dev_state = HLS_DN_DISABLE;
 				break;
 			default:
 				ret = -EINVAL;
-				break;
-			}
-
-			if (ret)
-				break;
-			/* fall through */
-		case HFI1_SNOOP_IOCGETLINKSTATE:
-		case HFI1_SNOOP_IOCGETLINKSTATE_EXTRA:
-			if (cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) {
-				memset(&link_info, 0, sizeof(link_info));
-				if (copy_from_user(&link_info,
-					(struct hfi1_link_info __user *)arg,
-					sizeof(link_info)))
-					ret = -EFAULT;
-				index = link_info.port_number;
-			} else {
-				ret = __get_user(index, (int __user *) arg);
-				if (ret !=  0)
-					break;
-			}
-
-			if (index > dd->num_pports - 1) {
-				ret = -EINVAL;
-				break;
-			}
-
-			ppd = &dd->pport[index];
-			if (!ppd) {
-				ret = -EINVAL;
-				break;
-			}
-			value = hfi1_ibphys_portstate(ppd);
-			value <<= 4;
-			value |= driver_lstate(ppd);
-
-			snoop_dbg("Link port | Link State: %d", value);
-
-			if ((cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) ||
-			    (cmd == HFI1_SNOOP_IOCSETLINKSTATE_EXTRA)) {
-				link_info.port_state = value;
-				link_info.node_guid = cpu_to_be64(ppd->guid);
-				link_info.link_speed_active =
-							ppd->link_speed_active;
-				link_info.link_width_active =
-							ppd->link_width_active;
-				if (copy_to_user(
-					(struct hfi1_link_info __user *)arg,
-					&link_info, sizeof(link_info)))
-					ret = -EFAULT;
-			} else {
-				ret = __put_user(value, (int __user *)arg);
+				goto done;
 			}
+			ret = set_link_state(ppd, dev_state);
 			break;
-
-		case HFI1_SNOOP_IOCCLEARQUEUE:
-			snoop_dbg("Clearing snoop queue");
-			drain_snoop_list(&dd->hfi1_snoop.queue);
+		case IB_PORT_ARMED:
+			ret = set_link_state(ppd, HLS_UP_ARMED);
+			if (!ret)
+				send_idle_sma(dd, SMA_IDLE_ARM);
 			break;
-
-		case HFI1_SNOOP_IOCCLEARFILTER:
-			snoop_dbg("Clearing filter");
-			if (dd->hfi1_snoop.filter_callback) {
-				/* Drain packets first */
-				drain_snoop_list(&dd->hfi1_snoop.queue);
-				dd->hfi1_snoop.filter_callback = NULL;
-			}
-			kfree(dd->hfi1_snoop.filter_value);
-			dd->hfi1_snoop.filter_value = NULL;
+		case IB_PORT_ACTIVE:
+			ret = set_link_state(ppd, HLS_UP_ACTIVE);
+			if (!ret)
+				send_idle_sma(dd, SMA_IDLE_ACTIVE);
+			break;
+		default:
+			ret = -EINVAL;
 			break;
+		}
 
-		case HFI1_SNOOP_IOCSETFILTER:
-			snoop_dbg("Setting filter");
-			/* just copy command structure */
-			argp = (unsigned long *)arg;
-			if (copy_from_user(&filter_cmd, (void __user *)argp,
-					     sizeof(filter_cmd))) {
+		if (ret)
+			break;
+		/* fall through */
+	case HFI1_SNOOP_IOCGETLINKSTATE:
+	case HFI1_SNOOP_IOCGETLINKSTATE_EXTRA:
+		if (cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) {
+			memset(&link_info, 0, sizeof(link_info));
+			if (copy_from_user(&link_info,
+					   (struct hfi1_link_info __user *)arg,
+					   sizeof(link_info)))
 				ret = -EFAULT;
+			index = link_info.port_number;
+		} else {
+			ret = __get_user(index, (int __user *)arg);
+			if (ret !=  0)
 				break;
-			}
-			if (filter_cmd.opcode >= HFI1_MAX_FILTERS) {
-				pr_alert("Invalid opcode in request\n");
-				ret = -EINVAL;
-				break;
-			}
+		}
 
-			snoop_dbg("Opcode %d Len %d Ptr %p",
-				   filter_cmd.opcode, filter_cmd.length,
-				   filter_cmd.value_ptr);
+		if (index > dd->num_pports - 1) {
+			ret = -EINVAL;
+			break;
+		}
 
-			filter_value = kcalloc(filter_cmd.length, sizeof(u8),
-					       GFP_KERNEL);
-			if (!filter_value) {
-				pr_alert("Not enough memory\n");
-				ret = -ENOMEM;
-				break;
-			}
-			/* copy remaining data from userspace */
-			if (copy_from_user((u8 *)filter_value,
-					(void __user *)filter_cmd.value_ptr,
-					filter_cmd.length)) {
-				kfree(filter_value);
+		ppd = &dd->pport[index];
+		if (!ppd) {
+			ret = -EINVAL;
+			break;
+		}
+		value = hfi1_ibphys_portstate(ppd);
+		value <<= 4;
+		value |= driver_lstate(ppd);
+
+		snoop_dbg("Link port | Link State: %d", value);
+
+		if ((cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) ||
+		    (cmd == HFI1_SNOOP_IOCSETLINKSTATE_EXTRA)) {
+			link_info.port_state = value;
+			link_info.node_guid = cpu_to_be64(ppd->guid);
+			link_info.link_speed_active =
+						ppd->link_speed_active;
+			link_info.link_width_active =
+						ppd->link_width_active;
+			if (copy_to_user((struct hfi1_link_info __user *)arg,
+					 &link_info, sizeof(link_info)))
 				ret = -EFAULT;
-				break;
-			}
+		} else {
+			ret = __put_user(value, (int __user *)arg);
+		}
+		break;
+
+	case HFI1_SNOOP_IOCCLEARQUEUE:
+		snoop_dbg("Clearing snoop queue");
+		drain_snoop_list(&dd->hfi1_snoop.queue);
+		break;
+
+	case HFI1_SNOOP_IOCCLEARFILTER:
+		snoop_dbg("Clearing filter");
+		if (dd->hfi1_snoop.filter_callback) {
 			/* Drain packets first */
 			drain_snoop_list(&dd->hfi1_snoop.queue);
-			dd->hfi1_snoop.filter_callback =
-				hfi1_filters[filter_cmd.opcode].filter;
-			/* just in case we see back to back sets */
-			kfree(dd->hfi1_snoop.filter_value);
-			dd->hfi1_snoop.filter_value = filter_value;
+			dd->hfi1_snoop.filter_callback = NULL;
+		}
+		kfree(dd->hfi1_snoop.filter_value);
+		dd->hfi1_snoop.filter_value = NULL;
+		break;
 
+	case HFI1_SNOOP_IOCSETFILTER:
+		snoop_dbg("Setting filter");
+		/* just copy command structure */
+		argp = (unsigned long *)arg;
+		if (copy_from_user(&filter_cmd, (void __user *)argp,
+				   sizeof(filter_cmd))) {
+			ret = -EFAULT;
 			break;
-		case HFI1_SNOOP_IOCGETVERSION:
-			value = SNOOP_CAPTURE_VERSION;
-			snoop_dbg("Getting version: %d", value);
-			ret = __put_user(value, (int __user *)arg);
+		}
+		if (filter_cmd.opcode >= HFI1_MAX_FILTERS) {
+			pr_alert("Invalid opcode in request\n");
+			ret = -EINVAL;
 			break;
-		case HFI1_SNOOP_IOCSET_OPTS:
-			snoop_flags = 0;
-			ret = __get_user(value, (int __user *) arg);
-			if (ret != 0)
-				break;
+		}
 
-			snoop_dbg("Setting snoop option %d", value);
-			if (value & SNOOP_DROP_SEND)
-				snoop_flags |= SNOOP_DROP_SEND;
-			if (value & SNOOP_USE_METADATA)
-				snoop_flags |= SNOOP_USE_METADATA;
+		snoop_dbg("Opcode %d Len %d Ptr %p",
+			  filter_cmd.opcode, filter_cmd.length,
+			  filter_cmd.value_ptr);
+
+		filter_value = kcalloc(filter_cmd.length, sizeof(u8),
+				       GFP_KERNEL);
+		if (!filter_value) {
+			ret = -ENOMEM;
 			break;
-		default:
-			ret = -ENOTTY;
+		}
+		/* copy remaining data from userspace */
+		if (copy_from_user((u8 *)filter_value,
+				   (void __user *)filter_cmd.value_ptr,
+				   filter_cmd.length)) {
+			kfree(filter_value);
+			ret = -EFAULT;
 			break;
 		}
+		/* Drain packets first */
+		drain_snoop_list(&dd->hfi1_snoop.queue);
+		dd->hfi1_snoop.filter_callback =
+			hfi1_filters[filter_cmd.opcode].filter;
+		/* just in case we see back to back sets */
+		kfree(dd->hfi1_snoop.filter_value);
+		dd->hfi1_snoop.filter_value = filter_value;
+
+		break;
+	case HFI1_SNOOP_IOCGETVERSION:
+		value = SNOOP_CAPTURE_VERSION;
+		snoop_dbg("Getting version: %d", value);
+		ret = __put_user(value, (int __user *)arg);
+		break;
+	case HFI1_SNOOP_IOCSET_OPTS:
+		snoop_flags = 0;
+		ret = __get_user(value, (int __user *)arg);
+		if (ret != 0)
+			break;
+
+		snoop_dbg("Setting snoop option %d", value);
+		if (value & SNOOP_DROP_SEND)
+			snoop_flags |= SNOOP_DROP_SEND;
+		if (value & SNOOP_USE_METADATA)
+			snoop_flags |= SNOOP_USE_METADATA;
+		break;
+	default:
+		ret = -ENOTTY;
+		break;
 	}
 done:
 	spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
-- 
1.8.2

[PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

This goto done is followed by an if (ret) break in the outer switch clause.  It
is unnecessary.

Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/staging/rdma/hfi1/diag.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 2bb857b2a103..556a47591989 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -1053,7 +1053,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 				break;
 			default:
 				ret = -EINVAL;
-				goto done;
 			}
 			ret = set_link_state(ppd, dev_state);
 			break;
@@ -1201,7 +1200,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		ret = -ENOTTY;
 		break;
 	}
-done:
+
 	spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 	return ret;
 }
-- 
1.8.2

Re: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done

[Dan Carpenter]

On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This goto done is followed by an if (ret) break in the outer switch clause.  It
> is unnecessary.
> 
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/staging/rdma/hfi1/diag.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
> index 2bb857b2a103..556a47591989 100644
> --- a/drivers/staging/rdma/hfi1/diag.c
> +++ b/drivers/staging/rdma/hfi1/diag.c
> @@ -1053,7 +1053,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  				break;
>  			default:
>  				ret = -EINVAL;
> -				goto done;
>  			}
>  			ret = set_link_state(ppd, dev_state);
>  			break;

Wut?  No it's not.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done

[ira.weiny]

On Wed, Nov 11, 2015 at 12:01:08PM +0300, Dan Carpenter wrote:
> On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > This goto done is followed by an if (ret) break in the outer switch clause.  It
> > is unnecessary.
> > 
> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/staging/rdma/hfi1/diag.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
> > index 2bb857b2a103..556a47591989 100644
> > --- a/drivers/staging/rdma/hfi1/diag.c
> > +++ b/drivers/staging/rdma/hfi1/diag.c
> > @@ -1053,7 +1053,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> >  				break;
> >  			default:
> >  				ret = -EINVAL;
> > -				goto done;
> >  			}
> >  			ret = set_link_state(ppd, dev_state);
> >  			break;
> 
> Wut?  No it's not.

Sorry, you are correct at this point in the series.

I got over zealous with my splitting of this patch.  This needs to be squashed
into 

staging/rdma/hfi1: Return immediately on error

Which returns here.

Ira

Re: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done

[Dan Carpenter]

On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This goto done is followed by an if (ret) break in the outer switch clause.  It
> is unnecessary.
> 
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Also these sign offs don't really make sense.  Signed-off-by is
basically like signing a legal document saying the code in this patch
was not stolen from SCO UnixWare.  You only need to sign if you have
handled the patch.

Did Dennis write this patch or did he Ack it or Review it somehow?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done

[Dennis Dalessandro]

On Wed, Nov 11, 2015 at 12:03:36PM +0300, Dan Carpenter wrote:
>On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
>> From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 
>> This goto done is followed by an if (ret) break in the outer switch clause.  It
>> is unnecessary.
>> 
>> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
>Also these sign offs don't really make sense.  Signed-off-by is
>basically like signing a legal document saying the code in this patch
>was not stolen from SCO UnixWare.  You only need to sign if you have
>handled the patch.
>
>Did Dennis write this patch or did he Ack it or Review it somehow?
>
>regards,
>dan carpenter

We both participated in creating the patch(s) in this series. I think the 
dual signed-off-by is appropriate here.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] staging/rdma/hfi1: diag.c Correct code style issues

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Using the latest checkpatch correct the checks on diag.c

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/diag.c | 81 ++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 0aaad7412842..c5cc2db1b559 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -78,8 +78,8 @@
 	hfi1_cdbg(SNOOP, fmt, ##__VA_ARGS__)
 
 /* Snoop option mask */
-#define SNOOP_DROP_SEND	(1 << 0)
-#define SNOOP_USE_METADATA	(1 << 1)
+#define SNOOP_DROP_SEND		BIT(0)
+#define SNOOP_USE_METADATA	BIT(1)
 
 static u8 snoop_flags;
 
@@ -135,7 +135,7 @@ static struct cdev diagpkt_cdev;
 static struct device *diagpkt_device;
 
 static ssize_t diagpkt_write(struct file *fp, const char __user *data,
-				 size_t count, loff_t *off);
+			     size_t count, loff_t *off);
 
 static const struct file_operations diagpkt_file_ops = {
 	.owner = THIS_MODULE,
@@ -177,37 +177,37 @@ struct hfi1_link_info {
 #define HFI1_SNOOP_IOCGETLINKSTATE \
 	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ)
 #define HFI1_SNOOP_IOCSETLINKSTATE \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+1)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 1)
 #define HFI1_SNOOP_IOCCLEARQUEUE \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+2)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 2)
 #define HFI1_SNOOP_IOCCLEARFILTER \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+3)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 3)
 #define HFI1_SNOOP_IOCSETFILTER \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+4)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 4)
 #define HFI1_SNOOP_IOCGETVERSION \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+5)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 5)
 #define HFI1_SNOOP_IOCSET_OPTS \
-	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+6)
+	_IO(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 6)
 
 /*
  * These offsets +6/+7 could change, but these are already known and used
  * IOCTL numbers so don't change them without a good reason.
  */
 #define HFI1_SNOOP_IOCGETLINKSTATE_EXTRA \
-	_IOWR(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+6, \
+	_IOWR(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 6, \
 		struct hfi1_link_info)
 #define HFI1_SNOOP_IOCSETLINKSTATE_EXTRA \
-	_IOWR(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ+7, \
+	_IOWR(HFI1_SNOOP_IOC_MAGIC, HFI1_SNOOP_IOC_BASE_SEQ + 7, \
 		struct hfi1_link_info)
 
 static int hfi1_snoop_open(struct inode *in, struct file *fp);
 static ssize_t hfi1_snoop_read(struct file *fp, char __user *data,
-				size_t pkt_len, loff_t *off);
+			       size_t pkt_len, loff_t *off);
 static ssize_t hfi1_snoop_write(struct file *fp, const char __user *data,
-				 size_t count, loff_t *off);
+				size_t count, loff_t *off);
 static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg);
 static unsigned int hfi1_snoop_poll(struct file *fp,
-					struct poll_table_struct *wait);
+				    struct poll_table_struct *wait);
 static int hfi1_snoop_release(struct inode *in, struct file *fp);
 
 struct hfi1_packet_filter_command {
@@ -323,14 +323,12 @@ static void hfi1_snoop_remove(struct hfi1_devdata *dd)
 
 void hfi1_diag_remove(struct hfi1_devdata *dd)
 {
-
 	hfi1_snoop_remove(dd);
 	if (atomic_dec_and_test(&diagpkt_count))
 		hfi1_cdev_cleanup(&diagpkt_cdev, &diagpkt_device);
 	hfi1_cdev_cleanup(&dd->diag_cdev, &dd->diag_device);
 }
 
-
 /*
  * Allocated structure shared between the credit return mechanism and
  * diagpkt_send().
@@ -393,7 +391,7 @@ static ssize_t diagpkt_send(struct diag_pkt *dp)
 
 	if (dp->version != _DIAG_PKT_VERS) {
 		dd_dev_err(dd, "Invalid version %u for diagpkt_write\n",
-			    dp->version);
+			   dp->version);
 		ret = -EINVAL;
 		goto bail;
 	}
@@ -440,7 +438,7 @@ static ssize_t diagpkt_send(struct diag_pkt *dp)
 	}
 
 	if (copy_from_user(tmpbuf,
-			   (const void __user *) (unsigned long) dp->data,
+			   (const void __user *)(unsigned long)dp->data,
 			   dp->len)) {
 		ret = -EFAULT;
 		goto bail;
@@ -530,9 +528,9 @@ static ssize_t diagpkt_send(struct diag_pkt *dp)
 		 * NOTE: PRC_FILL_ERR is at best informational and cannot
 		 * be depended on.
 		 */
-		if (!ret && (((wait->code & PRC_STATUS_ERR)
-				|| (wait->code & PRC_FILL_ERR)
-				|| (wait->code & PRC_SC_DISABLE))))
+		if (!ret && (((wait->code & PRC_STATUS_ERR) ||
+			      (wait->code & PRC_FILL_ERR) ||
+			      (wait->code & PRC_SC_DISABLE))))
 			ret = -EIO;
 
 		put_diagpkt_wait(wait);	/* finished with the structure */
@@ -545,7 +543,7 @@ bail:
 }
 
 static ssize_t diagpkt_write(struct file *fp, const char __user *data,
-				 size_t count, loff_t *off)
+			     size_t count, loff_t *off)
 {
 	struct hfi1_devdata *dd;
 	struct send_context *sc;
@@ -565,7 +563,7 @@ static ssize_t diagpkt_write(struct file *fp, const char __user *data,
 	*/
 	if (dp.pbc) {
 		dd = hfi1_lookup(dp.unit);
-		if (dd == NULL)
+		if (!dd)
 			return -ENODEV;
 		vl = (dp.pbc >> PBC_VL_SHIFT) & PBC_VL_MASK;
 		sc = dd->vld[vl].sc;
@@ -598,7 +596,7 @@ static int hfi1_snoop_add(struct hfi1_devdata *dd, const char *name)
 	if (ret) {
 		dd_dev_err(dd, "Couldn't create %s device: %d", name, ret);
 		hfi1_cdev_cleanup(&dd->hfi1_snoop.cdev,
-				 &dd->hfi1_snoop.class_dev);
+				  &dd->hfi1_snoop.class_dev);
 	}
 
 	return ret;
@@ -611,7 +609,6 @@ static struct hfi1_devdata *hfi1_dd_from_sc_inode(struct inode *in)
 
 	dd = hfi1_lookup(unit);
 	return dd;
-
 }
 
 /* clear or restore send context integrity checks */
@@ -652,7 +649,7 @@ static int hfi1_snoop_open(struct inode *in, struct file *fp)
 	mutex_lock(&hfi1_mutex);
 
 	dd = hfi1_dd_from_sc_inode(in);
-	if (dd == NULL) {
+	if (!dd) {
 		ret = -ENODEV;
 		goto bail;
 	}
@@ -739,7 +736,7 @@ static int hfi1_snoop_release(struct inode *in, struct file *fp)
 	int mode_flag;
 
 	dd = hfi1_dd_from_sc_inode(in);
-	if (dd == NULL)
+	if (!dd)
 		return -ENODEV;
 
 	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
@@ -794,7 +791,7 @@ static unsigned int hfi1_snoop_poll(struct file *fp,
 	struct hfi1_devdata *dd;
 
 	dd = hfi1_dd_from_sc_inode(fp->f_inode);
-	if (dd == NULL)
+	if (!dd)
 		return -ENODEV;
 
 	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
@@ -805,7 +802,6 @@ static unsigned int hfi1_snoop_poll(struct file *fp,
 
 	spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 	return ret;
-
 }
 
 static ssize_t hfi1_snoop_write(struct file *fp, const char __user *data,
@@ -822,7 +818,7 @@ static ssize_t hfi1_snoop_write(struct file *fp, const char __user *data,
 	struct hfi1_pportdata *ppd;
 
 	dd = hfi1_dd_from_sc_inode(fp->f_inode);
-	if (dd == NULL)
+	if (!dd)
 		return -ENODEV;
 
 	ppd = dd->pport;
@@ -847,7 +843,7 @@ static ssize_t hfi1_snoop_write(struct file *fp, const char __user *data,
 		if (copy_from_user(&byte_one, data, 1))
 			return -EINVAL;
 
-		if (copy_from_user(&byte_two, data+1, 1))
+		if (copy_from_user(&byte_two, data + 1, 1))
 			return -EINVAL;
 
 		sc4 = (byte_one >> 4) & 0xf;
@@ -920,7 +916,7 @@ static ssize_t hfi1_snoop_read(struct file *fp, char __user *data,
 	struct hfi1_devdata *dd;
 
 	dd = hfi1_dd_from_sc_inode(fp->f_inode);
-	if (dd == NULL)
+	if (!dd)
 		return -ENODEV;
 
 	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
@@ -946,16 +942,18 @@ static ssize_t hfi1_snoop_read(struct file *fp, char __user *data,
 		spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 		if (pkt_len >= packet->total_len) {
 			if (copy_to_user(data, packet->data,
-				packet->total_len))
+					 packet->total_len))
 				ret = -EFAULT;
 			else
 				ret = packet->total_len;
-		} else
+		} else {
 			ret = -EINVAL;
+		}
 
 		kfree(packet);
-	} else
+	} else {
 		spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
+	}
 
 	return ret;
 }
@@ -1321,7 +1319,6 @@ static int hfi1_filter_mad_mgmt_class(void *ibhdr, void *packet_data,
 
 static int hfi1_filter_qp_number(void *ibhdr, void *packet_data, void *value)
 {
-
 	struct hfi1_ib_header *hdr;
 	struct hfi1_other_headers *ohdr = NULL;
 	int ret;
@@ -1404,7 +1401,6 @@ static int hfi1_filter_ib_service_level(void *ibhdr, void *packet_data,
 
 static int hfi1_filter_ib_pkey(void *ibhdr, void *packet_data, void *value)
 {
-
 	u32 lnh = 0;
 	struct hfi1_ib_header *hdr;
 	struct hfi1_other_headers *ohdr = NULL;
@@ -1479,13 +1475,12 @@ static struct snoop_packet *allocate_snoop_packet(u32 hdr_len,
 
 	struct snoop_packet *packet;
 
-	packet = kzalloc(sizeof(struct snoop_packet) + hdr_len + data_len
+	packet = kzalloc(sizeof(*packet) + hdr_len + data_len
 			 + md_len,
 			 GFP_ATOMIC | __GFP_NOWARN);
 	if (likely(packet))
 		INIT_LIST_HEAD(&packet->list);
 
-
 	return packet;
 }
 
@@ -1542,12 +1537,11 @@ int snoop_recv_handler(struct hfi1_packet *packet)
 		    unlikely(snoop_flags & SNOOP_USE_METADATA))
 			md_len = sizeof(struct capture_md);
 
-
 		s_packet = allocate_snoop_packet(header_size,
 						 tlen - header_size,
 						 md_len);
 
-		if (unlikely(s_packet == NULL)) {
+		if (unlikely(!s_packet)) {
 			dd_dev_warn_ratelimited(ppd->dd, "Unable to allocate snoop/capture packet\n");
 			break;
 		}
@@ -1668,7 +1662,7 @@ int snoop_send_pio_handler(struct hfi1_qp *qp, struct hfi1_pkt_state *ps,
 	/* not using ss->total_len as arg 2 b/c that does not count CRC */
 	s_packet = allocate_snoop_packet(hdr_len, tlen - hdr_len, md_len);
 
-	if (unlikely(s_packet == NULL)) {
+	if (unlikely(!s_packet)) {
 		dd_dev_warn_ratelimited(ppd->dd, "Unable to allocate snoop/capture packet\n");
 		goto out;
 	}
@@ -1837,7 +1831,7 @@ void snoop_inline_pio_send(struct hfi1_devdata *dd, struct pio_buf *pbuf,
 
 		s_packet = allocate_snoop_packet(packet_len, 0, md_len);
 
-		if (unlikely(s_packet == NULL)) {
+		if (unlikely(!s_packet)) {
 			dd_dev_warn_ratelimited(dd, "Unable to allocate snoop/capture packet\n");
 			goto inline_pio_out;
 		}
@@ -1869,5 +1863,4 @@ void snoop_inline_pio_send(struct hfi1_devdata *dd, struct pio_buf *pbuf,
 
 inline_pio_out:
 	pio_copy(dd, pbuf, pbc, from, count);
-
 }
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/8] staging/rdma/hfi1: return early if setlink state was specified

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Set link state was not supported and so we can return early in the parameter
checks rather than falling through the switch clause.

Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/diag.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 556a47591989..a489a79dd3b6 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -999,16 +999,14 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		 * Other are invalid.
 		 */
 		return -EINVAL;
+	} else if (cmd == HFI1_SNOOP_IOCSETLINKSTATE) {
+		/* We do not support the old setlink state */
+		return -EINVAL;
 	}
 
 	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
 
 	switch (cmd) {
-	case HFI1_SNOOP_IOCSETLINKSTATE:
-		snoop_dbg("HFI1_SNOOP_IOCSETLINKSTATE is not valid");
-		ret = -EINVAL;
-		break;
-
 	case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA:
 		memset(&link_info, 0, sizeof(link_info));
 
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] staging/rdma/hfi1: return early if setlink state was specified

[Dan Carpenter]

On Wed, Nov 11, 2015 at 12:43:06AM -0500, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Set link state was not supported and so we can return early in the parameter
> checks rather than falling through the switch clause.
> 
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/staging/rdma/hfi1/diag.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
> index 556a47591989..a489a79dd3b6 100644
> --- a/drivers/staging/rdma/hfi1/diag.c
> +++ b/drivers/staging/rdma/hfi1/diag.c
> @@ -999,16 +999,14 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		 * Other are invalid.
>  		 */
>  		return -EINVAL;
> +	} else if (cmd == HFI1_SNOOP_IOCSETLINKSTATE) {
> +		/* We do not support the old setlink state */
> +		return -EINVAL;

Just delete it and let the default in the switch statement return
-ENOTTY.

regards,
dan carpenter

[PATCH 6/8] staging/rdma/hfi1: Further clean up hfi1_ioctl parameter checks

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Final clean up of the if/then/else clause for the parameter checks of
hfi1_ioctl

Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/diag.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index a489a79dd3b6..43f08080480c 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -974,24 +974,28 @@ 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 == NULL)
 		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 (((_IOC_DIR(cmd) & _IOC_READ)
-	    && !access_ok(VERIFY_WRITE, (void __user *)arg, _IOC_SIZE(cmd)))
-	    || ((_IOC_DIR(cmd) & _IOC_WRITE)
-	    && !access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd)))) {
+	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
 		return -EFAULT;
-	} else if (!capable(CAP_SYS_ADMIN)) {
+
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	} else if ((mode_flag & HFI1_PORT_CAPTURE_MODE) &&
-		   (cmd != HFI1_SNOOP_IOCCLEARQUEUE) &&
-		   (cmd != HFI1_SNOOP_IOCCLEARFILTER) &&
-		   (cmd != HFI1_SNOOP_IOCSETFILTER)) {
+
+	if ((mode_flag & HFI1_PORT_CAPTURE_MODE) &&
+	    (cmd != HFI1_SNOOP_IOCCLEARQUEUE) &&
+	    (cmd != HFI1_SNOOP_IOCCLEARFILTER) &&
+	    (cmd != HFI1_SNOOP_IOCSETFILTER))
 		/* Capture devices are allowed only 3 operations
 		 * 1.Clear capture queue
 		 * 2.Clear capture filter
@@ -999,10 +1003,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		 * Other are invalid.
 		 */
 		return -EINVAL;
-	} else if (cmd == HFI1_SNOOP_IOCSETLINKSTATE) {
+
+	if (cmd == HFI1_SNOOP_IOCSETLINKSTATE)
 		/* We do not support the old setlink state */
 		return -EINVAL;
-	}
 
 	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
 
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] staging/rdma/hfi1: Return immediately on error

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Now that the spinlock is not taken throughout hfi1_ioctl it is safe to return
early rather than setting a variable and falling through the switch.

Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/diag.c | 59 ++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 34a8c4da71d2..0f01618cc5b7 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -1015,20 +1015,16 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&link_info,
 				   (struct hfi1_link_info __user *)arg,
 				   sizeof(link_info)))
-			ret = -EFAULT;
+			return -EFAULT;
 
 		value = link_info.port_state;
 		index = link_info.port_number;
-		if (index > dd->num_pports - 1) {
-			ret = -EINVAL;
-			break;
-		}
+		if (index > dd->num_pports - 1)
+			return -EINVAL;
 
 		ppd = &dd->pport[index];
-		if (!ppd) {
-			ret = -EINVAL;
-			break;
-		}
+		if (!ppd)
+			return -EINVAL;
 
 		/* What we want to transition to */
 		phys_state = (value >> 4) & 0xF;
@@ -1052,7 +1048,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 				dev_state = HLS_DN_DISABLE;
 				break;
 			default:
-				ret = -EINVAL;
+				return -EINVAL;
 			}
 			ret = set_link_state(ppd, dev_state);
 			break;
@@ -1067,8 +1063,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 				send_idle_sma(dd, SMA_IDLE_ACTIVE);
 			break;
 		default:
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		if (ret)
@@ -1081,7 +1076,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 			if (copy_from_user(&link_info,
 					   (struct hfi1_link_info __user *)arg,
 					   sizeof(link_info)))
-				ret = -EFAULT;
+				return -EFAULT;
 			index = link_info.port_number;
 		} else {
 			ret = __get_user(index, (int __user *)arg);
@@ -1089,16 +1084,13 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 				break;
 		}
 
-		if (index > dd->num_pports - 1) {
-			ret = -EINVAL;
-			break;
-		}
+		if (index > dd->num_pports - 1)
+			return -EINVAL;
 
 		ppd = &dd->pport[index];
-		if (!ppd) {
-			ret = -EINVAL;
-			break;
-		}
+		if (!ppd)
+			return -EINVAL;
+
 		value = hfi1_ibphys_portstate(ppd);
 		value <<= 4;
 		value |= driver_lstate(ppd);
@@ -1115,7 +1107,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 						ppd->link_width_active;
 			if (copy_to_user((struct hfi1_link_info __user *)arg,
 					 &link_info, sizeof(link_info)))
-				ret = -EFAULT;
+				return -EFAULT;
 		} else {
 			ret = __put_user(value, (int __user *)arg);
 		}
@@ -1146,14 +1138,12 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		/* just copy command structure */
 		argp = (unsigned long *)arg;
 		if (copy_from_user(&filter_cmd, (void __user *)argp,
-				   sizeof(filter_cmd))) {
-			ret = -EFAULT;
-			break;
-		}
+				   sizeof(filter_cmd)))
+			return -EFAULT;
+
 		if (filter_cmd.opcode >= HFI1_MAX_FILTERS) {
 			pr_alert("Invalid opcode in request\n");
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		snoop_dbg("Opcode %d Len %d Ptr %p",
@@ -1162,17 +1152,15 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 
 		filter_value = kcalloc(filter_cmd.length, sizeof(u8),
 				       GFP_KERNEL);
-		if (!filter_value) {
-			ret = -ENOMEM;
-			break;
-		}
+		if (!filter_value)
+			return -ENOMEM;
+
 		/* copy remaining data from userspace */
 		if (copy_from_user((u8 *)filter_value,
 				   (void __user *)filter_cmd.value_ptr,
 				   filter_cmd.length)) {
 			kfree(filter_value);
-			ret = -EFAULT;
-			break;
+			return -EFAULT;
 		}
 		/* Drain packets first */
 		spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
@@ -1202,8 +1190,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 			snoop_flags |= SNOOP_USE_METADATA;
 		break;
 	default:
-		ret = -ENOTTY;
-		break;
+		return -ENOTTY;
 	}
 
 	return ret;
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] staging/rdma/hfi1: Reduce snoop locking scope in IOCTL handler.

[ira.weiny]

From: Dennis Dalessandro <dennis.dalessandro@intel.com>

This patch avoids issues while calling into copy from/to user while holding the
lock by only taking the lock when it is absolutely required.

The only commands which require the snoop lock are: *Set Filter *Clear Filter
*Clear Queue

Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/staging/rdma/hfi1/diag.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 43f08080480c..34a8c4da71d2 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -1008,8 +1008,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		/* We do not support the old setlink state */
 		return -EINVAL;
 
-	spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
-
 	switch (cmd) {
 	case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA:
 		memset(&link_info, 0, sizeof(link_info));
@@ -1125,11 +1123,14 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 
 	case HFI1_SNOOP_IOCCLEARQUEUE:
 		snoop_dbg("Clearing snoop queue");
+		spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
 		drain_snoop_list(&dd->hfi1_snoop.queue);
+		spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 		break;
 
 	case HFI1_SNOOP_IOCCLEARFILTER:
 		snoop_dbg("Clearing filter");
+		spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
 		if (dd->hfi1_snoop.filter_callback) {
 			/* Drain packets first */
 			drain_snoop_list(&dd->hfi1_snoop.queue);
@@ -1137,6 +1138,7 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		}
 		kfree(dd->hfi1_snoop.filter_value);
 		dd->hfi1_snoop.filter_value = NULL;
+		spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 		break;
 
 	case HFI1_SNOOP_IOCSETFILTER:
@@ -1173,13 +1175,14 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 			break;
 		}
 		/* Drain packets first */
+		spin_lock_irqsave(&dd->hfi1_snoop.snoop_lock, flags);
 		drain_snoop_list(&dd->hfi1_snoop.queue);
 		dd->hfi1_snoop.filter_callback =
 			hfi1_filters[filter_cmd.opcode].filter;
 		/* just in case we see back to back sets */
 		kfree(dd->hfi1_snoop.filter_value);
 		dd->hfi1_snoop.filter_value = filter_value;
-
+		spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 		break;
 	case HFI1_SNOOP_IOCGETVERSION:
 		value = SNOOP_CAPTURE_VERSION;
@@ -1203,7 +1206,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		break;
 	}
 
-	spin_unlock_irqrestore(&dd->hfi1_snoop.snoop_lock, flags);
 	return ret;
 }
 
-- 
1.8.2