[PATCH 0/7] staging: unisys: visornic testing fix series

[PATCH 0/7] staging: unisys: visornic testing fix series

[Benjamin Romer]

This patch series fixes more technical issues and bugs found in visornic
testing.

David Kershner (1):
  staging: unisys: Process more than one response per check

Neil Horman (1):
  staging: unisys: visornic: fix serialization mechanism around usage
    atomic

Tim Sell (5):
  staging: unisys: add simple error-check into visornic receive path
  staging: unisys: visornic - ensure proper net locking in tx reset
    logic
  staging: unisys: visornic - check visorchannel_signalinsert/remove
    failures
  staging: unisys: visornic - consolidate+simplify xmit watermark checks
  staging: unisys: visornic - prevent NETDEV WATCHDOG timeouts after IO
    recovery

 drivers/staging/unisys/visornic/visornic_main.c | 314 ++++++++++++++----------
 kernel/kthread.c                                |   4 +
 2 files changed, 182 insertions(+), 136 deletions(-)

-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/7] staging: unisys: visornic: fix serialization mechanism around usage atomic

[Benjamin Romer]

From: Neil Horman <nhorman@redhat.com>

Missed this in my initial review.  The usage counter that guards against
kthread task is horribly racy.  The atomic is self consistent, but theres
nothing that prevents the service_resp_queue function from re-incrementing
it immediately after the check in disable_with_timeout is complete.  Its
just a improper usage of atomics as a serialization mechanism.

Instead use kthread_park to pause the thread in its activity so that
buffers can be properly freed without racing against usage in
service_resp_queue

Signed-off-by: Neil Horman <nhorman@redhat.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 23 ++++++-----------------
 kernel/kthread.c                                |  4 ++++
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 4d49937..aeecb14 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -126,7 +126,6 @@ struct visornic_devdata {
 	unsigned short old_flags;	/* flags as they were prior to
 					 * set_multicast_list
 					 */
-	atomic_t usage;			/* count of users */
 	int num_rcv_bufs;		/* indicates how many rcv buffers
 					 * the vnic will post
 					 */
@@ -565,19 +564,7 @@ visornic_disable_with_timeout(struct net_device *netdev, const int timeout)
 		spin_lock_irqsave(&devdata->priv_lock, flags);
 	}
 
-	/* Wait for usage to go to 1 (no other users) before freeing
-	 * rcv buffers
-	 */
-	if (atomic_read(&devdata->usage) > 1) {
-		while (1) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			spin_unlock_irqrestore(&devdata->priv_lock, flags);
-			schedule_timeout(msecs_to_jiffies(10));
-			spin_lock_irqsave(&devdata->priv_lock, flags);
-			if (atomic_read(&devdata->usage))
-				break;
-		}
-	}
+	kthread_park(devdata->threadinfo.task);
 
 	/* we've set enabled to 0, so we can give up the lock. */
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
@@ -594,6 +581,7 @@ visornic_disable_with_timeout(struct net_device *netdev, const int timeout)
 		}
 	}
 
+	kthread_unpark(devdata->threadinfo.task);
 	return 0;
 }
 
@@ -1622,7 +1610,7 @@ send_rcv_posts_if_needed(struct visornic_devdata *devdata)
  *	Returns when response queue is empty or when the threadd stops.
  */
 static void
-drain_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
+service_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
 {
 	unsigned long flags;
 	struct net_device *netdev;
@@ -1742,6 +1730,8 @@ process_incoming_rsps(void *v)
 			devdata->rsp_queue, (atomic_read(
 					     &devdata->interrupt_rcvd) == 1),
 				msecs_to_jiffies(devdata->thread_wait_ms));
+		if (kthread_should_park())
+			kthread_parkme();
 
 		/* periodically check to see if there are any rcf bufs which
 		 * need to get sent to the IOSP. This can only happen if
@@ -1749,7 +1739,7 @@ process_incoming_rsps(void *v)
 		 */
 		atomic_set(&devdata->interrupt_rcvd, 0);
 		send_rcv_posts_if_needed(devdata);
-		drain_queue(cmdrsp, devdata);
+		service_resp_queue(cmdrsp, devdata);
 	}
 
 	kfree(cmdrsp);
@@ -1809,7 +1799,6 @@ static int visornic_probe(struct visor_device *dev)
 	init_waitqueue_head(&devdata->rsp_queue);
 	spin_lock_init(&devdata->priv_lock);
 	devdata->enabled = 0; /* not yet */
-	atomic_set(&devdata->usage, 1);
 
 	/* Setup rcv bufs */
 	channel_offset = offsetof(struct spar_io_channel_protocol,
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c..bad80c1 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -97,6 +97,7 @@ bool kthread_should_park(void)
 {
 	return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
 }
+EXPORT_SYMBOL(kthread_should_park);
 
 /**
  * kthread_freezable_should_stop - should this freezable kthread return now?
@@ -171,6 +172,7 @@ void kthread_parkme(void)
 {
 	__kthread_parkme(to_kthread(current));
 }
+EXPORT_SYMBOL(kthread_parkme);
 
 static int kthread(void *_create)
 {
@@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
 	if (kthread)
 		__kthread_unpark(k, kthread);
 }
+EXPORT_SYMBOL(kthread_unpark);
 
 /**
  * kthread_park - park a thread created by kthread_create().
@@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
 	}
 	return ret;
 }
+EXPORT_SYMBOL(kthread_park);
 
 /**
  * kthread_stop - stop a thread created by kthread_create().
-- 
2.1.4

Re: [PATCH 1/7] staging: unisys: visornic: fix serialization mechanism around usage atomic

[Greg KH]

On Fri, Jul 24, 2015 at 12:00:19PM -0400, Benjamin Romer wrote:
> From: Neil Horman <nhorman@redhat.com>
> 
> Missed this in my initial review.  The usage counter that guards against
> kthread task is horribly racy.  The atomic is self consistent, but theres
> nothing that prevents the service_resp_queue function from re-incrementing
> it immediately after the check in disable_with_timeout is complete.  Its
> just a improper usage of atomics as a serialization mechanism.
> 
> Instead use kthread_park to pause the thread in its activity so that
> buffers can be properly freed without racing against usage in
> service_resp_queue
> 
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> ---
>  drivers/staging/unisys/visornic/visornic_main.c | 23 ++++++-----------------
>  kernel/kthread.c                                |  4 ++++

Sorry, staging drivers can not modify other parts of the kernel.  Adding
exports for these kthread functions needs to happen as a "real" patch
that the kthread maintainer accepts, I can't take this.

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/7] staging: unisys: visornic: fix serialization mechanism around usage atomic

[Neil Horman]

On Fri, Jul 24, 2015 at 01:35:51PM -0700, Greg KH wrote:
> On Fri, Jul 24, 2015 at 12:00:19PM -0400, Benjamin Romer wrote:
> > From: Neil Horman <nhorman@redhat.com>
> > 
> > Missed this in my initial review.  The usage counter that guards against
> > kthread task is horribly racy.  The atomic is self consistent, but theres
> > nothing that prevents the service_resp_queue function from re-incrementing
> > it immediately after the check in disable_with_timeout is complete.  Its
> > just a improper usage of atomics as a serialization mechanism.
> > 
> > Instead use kthread_park to pause the thread in its activity so that
> > buffers can be properly freed without racing against usage in
> > service_resp_queue
> > 
> > Signed-off-by: Neil Horman <nhorman@redhat.com>
> > Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> > ---
> >  drivers/staging/unisys/visornic/visornic_main.c | 23 ++++++-----------------
> >  kernel/kthread.c                                |  4 ++++
> 
> Sorry, staging drivers can not modify other parts of the kernel.  Adding
> exports for these kthread functions needs to happen as a "real" patch
> that the kthread maintainer accepts, I can't take this.
> 
> greg k-h

Thats fine, we can split out the kthread_park exports and propose them on lkml
separately. 

Thanks
Neil

[PATCH 2/7] staging: unisys: add simple error-check into visornic receive path

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Add an error check here for use with dynamic debugging.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index aeecb14..6ce3fc2 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1253,13 +1253,13 @@ visornic_rx(struct uiscmdrsp *cmdrsp)
 			curr->data_len = 0;
 			off += currsize;
 		}
-#ifdef DEBUG
 		/* assert skb->len == off */
 		if (skb->len != off) {
 			dev_err(&devdata->netdev->dev,
-				"%s something wrong; skb->len:%d != off:%d\n",
-				netdev->name, skb->len, off);
+				"something wrong; skb->len:%d != off:%d\n",
+				skb->len, off);
 		}
+#ifdef DEBUG
 		/* test code */
 		cc = util_copy_fragsinfo_from_skb("rcvchaintest", skb,
 						  RCVPOST_BUF_SIZE,
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/7] staging: unisys: add simple error-check into visornic receive path

[Greg KH]

On Fri, Jul 24, 2015 at 12:00:20PM -0400, Benjamin Romer wrote:
> From: Tim Sell <Timothy.Sell@unisys.com>
> 
> Add an error check here for use with dynamic debugging.
> 
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> ---
>  drivers/staging/unisys/visornic/visornic_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index aeecb14..6ce3fc2 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1253,13 +1253,13 @@ visornic_rx(struct uiscmdrsp *cmdrsp)
>  			curr->data_len = 0;
>  			off += currsize;
>  		}
> -#ifdef DEBUG
>  		/* assert skb->len == off */
>  		if (skb->len != off) {
>  			dev_err(&devdata->netdev->dev,
> -				"%s something wrong; skb->len:%d != off:%d\n",
> -				netdev->name, skb->len, off);
> +				"something wrong; skb->len:%d != off:%d\n",
> +				skb->len, off);

The changelog comment doesn't match what you are doing here.

Also, shouldn't this be netdev_err() instead?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/7] staging: unisys: Process more than one response per check

[Benjamin Romer]

From: David Kershner <david.kershner@unisys.com>

When s-Par is in polling mode it checks every 2 ms to see if there is
a response from the IO service partition in the queue. Currently it
just reads one entry per 2 ms, this needs to be changed so it drains
the queue on each check.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 164 ++++++++++++------------
 1 file changed, 83 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 6ce3fc2..8d600fa 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1617,93 +1617,95 @@ service_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
 
 	/* TODO: CLIENT ACQUIRE -- Don't really need this at the
 	 * moment */
-	if (!visorchannel_signalremove(devdata->dev->visorchannel,
-				       IOCHAN_FROM_IOPART,
-				       cmdrsp))
-		return; /* queue empty */
-
-	switch (cmdrsp->net.type) {
-	case NET_RCV:
-		devdata->chstat.got_rcv++;
-		/* process incoming packet */
-		visornic_rx(cmdrsp);
-		break;
-	case NET_XMIT_DONE:
-		spin_lock_irqsave(&devdata->priv_lock, flags);
-		devdata->chstat.got_xmit_done++;
-		if (cmdrsp->net.xmtdone.xmt_done_result)
-			devdata->chstat.xmit_fail++;
-		/* only call queue wake if we stopped it */
-		netdev = ((struct sk_buff *)cmdrsp->net.buf)->dev;
-		/* ASSERT netdev == vnicinfo->netdev; */
-		if ((netdev == devdata->netdev) &&
-		    netif_queue_stopped(netdev)) {
-			/* check to see if we have crossed
-			 * the lower watermark for
-			 * netif_wake_queue()
-			 */
-			if (((devdata->chstat.sent_xmit >=
-			    devdata->chstat.got_xmit_done) &&
-			    (devdata->chstat.sent_xmit -
-			    devdata->chstat.got_xmit_done <=
-			    devdata->lower_threshold_net_xmits)) ||
-			    ((devdata->chstat.sent_xmit <
-			    devdata->chstat.got_xmit_done) &&
-			    (ULONG_MAX - devdata->chstat.got_xmit_done
-			    + devdata->chstat.sent_xmit <=
-			    devdata->lower_threshold_net_xmits))) {
-				/* enough NET_XMITs completed
-				 * so can restart netif queue
+	for (;;) {
+		if (!visorchannel_signalremove(devdata->dev->visorchannel,
+					       IOCHAN_FROM_IOPART,
+					       cmdrsp))
+			break; /* queue empty */
+
+		switch (cmdrsp->net.type) {
+		case NET_RCV:
+			devdata->chstat.got_rcv++;
+			/* process incoming packet */
+			visornic_rx(cmdrsp);
+			break;
+		case NET_XMIT_DONE:
+			spin_lock_irqsave(&devdata->priv_lock, flags);
+			devdata->chstat.got_xmit_done++;
+			if (cmdrsp->net.xmtdone.xmt_done_result)
+				devdata->chstat.xmit_fail++;
+			/* only call queue wake if we stopped it */
+			netdev = ((struct sk_buff *)cmdrsp->net.buf)->dev;
+			/* ASSERT netdev == vnicinfo->netdev; */
+			if ((netdev == devdata->netdev) &&
+			    netif_queue_stopped(netdev)) {
+				/* check to see if we have crossed
+				 * the lower watermark for
+				 * netif_wake_queue()
 				 */
-				netif_wake_queue(netdev);
-				devdata->flow_control_lower_hits++;
+				if (((devdata->chstat.sent_xmit >=
+				    devdata->chstat.got_xmit_done) &&
+				    (devdata->chstat.sent_xmit -
+				    devdata->chstat.got_xmit_done <=
+				    devdata->lower_threshold_net_xmits)) ||
+				    ((devdata->chstat.sent_xmit <
+				    devdata->chstat.got_xmit_done) &&
+				    (ULONG_MAX - devdata->chstat.got_xmit_done
+				    + devdata->chstat.sent_xmit <=
+				    devdata->lower_threshold_net_xmits))) {
+					/* enough NET_XMITs completed
+					 * so can restart netif queue
+					 */
+					netif_wake_queue(netdev);
+					devdata->flow_control_lower_hits++;
+				}
 			}
-		}
-		skb_unlink(cmdrsp->net.buf, &devdata->xmitbufhead);
-		spin_unlock_irqrestore(&devdata->priv_lock, flags);
-		kfree_skb(cmdrsp->net.buf);
-		break;
-	case NET_RCV_ENBDIS_ACK:
-		devdata->chstat.got_enbdisack++;
-		netdev = (struct net_device *)
-		cmdrsp->net.enbdis.context;
-		spin_lock_irqsave(&devdata->priv_lock, flags);
-		devdata->enab_dis_acked = 1;
-		spin_unlock_irqrestore(&devdata->priv_lock, flags);
-
-		if (kthread_should_stop())
+			skb_unlink(cmdrsp->net.buf, &devdata->xmitbufhead);
+			spin_unlock_irqrestore(&devdata->priv_lock, flags);
+			kfree_skb(cmdrsp->net.buf);
 			break;
-		if (devdata->server_down &&
-		    devdata->server_change_state) {
-			/* Inform Linux that the link is up */
-			devdata->server_down = false;
-			devdata->server_change_state = false;
-			netif_wake_queue(netdev);
-			netif_carrier_on(netdev);
-		}
-		break;
-	case NET_CONNECT_STATUS:
-		netdev = devdata->netdev;
-		if (cmdrsp->net.enbdis.enable == 1) {
-			spin_lock_irqsave(&devdata->priv_lock, flags);
-			devdata->enabled = cmdrsp->net.enbdis.enable;
-			spin_unlock_irqrestore(&devdata->priv_lock,
-					       flags);
-			netif_wake_queue(netdev);
-			netif_carrier_on(netdev);
-		} else {
-			netif_stop_queue(netdev);
-			netif_carrier_off(netdev);
+		case NET_RCV_ENBDIS_ACK:
+			devdata->chstat.got_enbdisack++;
+			netdev = (struct net_device *)
+			cmdrsp->net.enbdis.context;
 			spin_lock_irqsave(&devdata->priv_lock, flags);
-			devdata->enabled = cmdrsp->net.enbdis.enable;
-			spin_unlock_irqrestore(&devdata->priv_lock,
-					       flags);
+			devdata->enab_dis_acked = 1;
+			spin_unlock_irqrestore(&devdata->priv_lock, flags);
+
+			if (kthread_should_stop())
+				break;
+			if (devdata->server_down &&
+			    devdata->server_change_state) {
+				/* Inform Linux that the link is up */
+				devdata->server_down = false;
+				devdata->server_change_state = false;
+				netif_wake_queue(netdev);
+				netif_carrier_on(netdev);
+			}
+			break;
+		case NET_CONNECT_STATUS:
+			netdev = devdata->netdev;
+			if (cmdrsp->net.enbdis.enable == 1) {
+				spin_lock_irqsave(&devdata->priv_lock, flags);
+				devdata->enabled = cmdrsp->net.enbdis.enable;
+				spin_unlock_irqrestore(&devdata->priv_lock,
+						       flags);
+				netif_wake_queue(netdev);
+				netif_carrier_on(netdev);
+			} else {
+				netif_stop_queue(netdev);
+				netif_carrier_off(netdev);
+				spin_lock_irqsave(&devdata->priv_lock, flags);
+				devdata->enabled = cmdrsp->net.enbdis.enable;
+				spin_unlock_irqrestore(&devdata->priv_lock,
+						       flags);
+			}
+			break;
+		default:
+			break;
 		}
-		break;
-	default:
-		break;
+		/* cmdrsp is now available for reuse  */
 	}
-	/* cmdrsp is now available for reuse  */
 }
 
 /**
-- 
2.1.4

[PATCH 4/7] staging: unisys: visornic - ensure proper net locking in tx reset logic

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

visornic tx reset handling is done asynchronously via a workqueue in
visornic_timeout_reset().  As a result, it needs to use rtnl_lock() /
rtnl_unlock() to lock against possible simultaneous close() of the network
device.

(I consulted the bnx2 driver as a model here, as that driver also does
its tx reset handling asynchronously, just like visornic does.  See
bnx2_tx_timeout() and bnx2_reset_task().)

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 8d600fa..7541c49 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -719,6 +719,12 @@ visornic_timeout_reset(struct work_struct *work)
 	devdata = container_of(work, struct visornic_devdata, timeout_reset);
 	netdev = devdata->netdev;
 
+	rtnl_lock();
+	if (!netif_running(netdev)) {
+		rtnl_unlock();
+		return;
+	}
+
 	response = visornic_disable_with_timeout(netdev,
 						 VISORNIC_INFINITE_RSP_WAIT);
 	if (response)
@@ -729,10 +735,13 @@ visornic_timeout_reset(struct work_struct *work)
 	if (response)
 		goto call_serverdown;
 
+	rtnl_unlock();
+
 	return;
 
 call_serverdown:
 	visornic_serverdown(devdata, NULL);
+	rtnl_unlock();
 }
 
 /**
-- 
2.1.4

[PATCH 5/7] staging: unisys: visornic - check visorchannel_signalinsert/remove failures

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Logic to check for failures of visorchannel_signalinsert() and
visorchannel_signalremove() were added, and a new sent_post_failed counter
tracks the number of times we failed to post a rcv buffer to the IO
partition.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 7541c49..baa137e 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -102,6 +102,7 @@ struct chanstat {
 	unsigned long sent_enbdis;
 	unsigned long sent_promisc;
 	unsigned long sent_post;
+	unsigned long sent_post_failed;
 	unsigned long sent_xmit;
 	unsigned long reject_count;
 	unsigned long extra_rcvbufs_sent;
@@ -478,11 +479,14 @@ post_skb(struct uiscmdrsp *cmdrsp,
 	if ((cmdrsp->net.rcvpost.frag.pi_off + skb->len) <= PI_PAGE_SIZE) {
 		cmdrsp->net.type = NET_RCV_POST;
 		cmdrsp->cmdtype = CMD_NET_TYPE;
-		visorchannel_signalinsert(devdata->dev->visorchannel,
+		if (visorchannel_signalinsert(devdata->dev->visorchannel,
 					  IOCHAN_TO_IOPART,
-					  cmdrsp);
-		atomic_inc(&devdata->num_rcvbuf_in_iovm);
-		devdata->chstat.sent_post++;
+					  cmdrsp)) {
+			atomic_inc(&devdata->num_rcvbuf_in_iovm);
+			devdata->chstat.sent_post++;
+		} else {
+			devdata->chstat.sent_post_failed++;
+		}
 	}
 }
 
@@ -504,10 +508,10 @@ send_enbdis(struct net_device *netdev, int state,
 	devdata->cmdrsp_rcv->net.enbdis.context = netdev;
 	devdata->cmdrsp_rcv->net.type = NET_RCV_ENBDIS;
 	devdata->cmdrsp_rcv->cmdtype = CMD_NET_TYPE;
-	visorchannel_signalinsert(devdata->dev->visorchannel,
+	if (visorchannel_signalinsert(devdata->dev->visorchannel,
 				  IOCHAN_TO_IOPART,
-				  devdata->cmdrsp_rcv);
-	devdata->chstat.sent_enbdis++;
+				  devdata->cmdrsp_rcv))
+		devdata->chstat.sent_enbdis++;
 }
 
 /**
@@ -1490,6 +1494,9 @@ static ssize_t info_debugfs_read(struct file *file, char __user *buf,
 				     " chstat.sent_post = %lu\n",
 				     devdata->chstat.sent_post);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
+				     " chstat.sent_post_failed = %lu\n",
+				     devdata->chstat.sent_post_failed);
+		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
 				     " chstat.sent_xmit = %lu\n",
 				     devdata->chstat.sent_xmit);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
-- 
2.1.4

[PATCH 6/7] staging: unisys: visornic - consolidate+simplify xmit watermark checks

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

The code that tests for high and low xmit watermarks was consolidated and
simplified. The output behavior should be identical, with the exception
of an off-by-one error being corrected in the tests done when the counters
overflowed.

Note that the literals passed to max() macros need to be explicitly
specified as unsigned long (UL), in order to prevent gcc warnings:

    warning: comparison of distinct pointer types lacks a cast

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 109 +++++++++++++++---------
 1 file changed, 71 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index baa137e..bd58e15 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -134,15 +134,16 @@ struct visornic_devdata {
 	atomic_t num_rcvbuf_in_iovm;
 	unsigned long alloc_failed_in_if_needed_cnt;
 	unsigned long alloc_failed_in_repost_rtn_cnt;
-	int max_outstanding_net_xmits;   /* absolute max number of outstanding
-					  * xmits - should never hit this
-					  */
-	int upper_threshold_net_xmits;   /* high water mark for calling
-					  * netif_stop_queue()
-					  */
-	int lower_threshold_net_xmits;	 /* high water mark for calling
-					  * netif_wake_queue()
-					  */
+	unsigned long max_outstanding_net_xmits; /* absolute max number of
+						  * outstanding xmits - should
+						  * never hit this
+						  */
+	unsigned long upper_threshold_net_xmits;  /* high water mark for
+						   * calling netif_stop_queue()
+						   */
+	unsigned long lower_threshold_net_xmits; /* high water mark for calling
+						  * netif_wake_queue()
+						  */
 	struct sk_buff_head xmitbufhead; /* xmitbufhead is the head of the
 					  * xmit buffer list that have been
 					  * sent to the IOPART end
@@ -779,6 +780,53 @@ visornic_close(struct net_device *netdev)
 }
 
 /**
+ *	devdata_xmits_outstanding - compute outstanding xmits
+ *	@devdata: visornic_devdata for device
+ *
+ *	Return value is the number of outstanding xmits.
+ */
+static unsigned long devdata_xmits_outstanding(struct visornic_devdata *devdata)
+{
+	if (devdata->chstat.sent_xmit >= devdata->chstat.got_xmit_done)
+		return devdata->chstat.sent_xmit -
+			devdata->chstat.got_xmit_done;
+	else
+		return (ULONG_MAX - devdata->chstat.got_xmit_done
+			+ devdata->chstat.sent_xmit + 1);
+}
+
+/**
+ *	vnic_hit_high_watermark
+ *	@devdata: indicates visornic device we are checking
+ *	@high_watermark: max num of unacked xmits we will tolerate,
+ *                       before we will start throttling
+ *
+ *      Returns true iff the number of unacked xmits sent to
+ *      the IO partition is >= high_watermark.
+ */
+static inline bool vnic_hit_high_watermark(struct visornic_devdata *devdata,
+					   ulong high_watermark)
+{
+	return (devdata_xmits_outstanding(devdata) >= high_watermark);
+}
+
+/**
+ *	vnic_hit_low_watermark
+ *	@devdata: indicates visornic device we are checking
+ *	@low_watermark: we will wait until the num of unacked xmits
+ *                      drops to this value or lower before we start
+ *                      transmitting again
+ *
+ *      Returns true iff the number of unacked xmits sent to
+ *      the IO partition is <= low_watermark.
+ */
+static inline bool vnic_hit_low_watermark(struct visornic_devdata *devdata,
+					  ulong low_watermark)
+{
+	return (devdata_xmits_outstanding(devdata) <= low_watermark);
+}
+
+/**
  *	visornic_xmit - send a packet to the IO Partition
  *	@skb: Packet to be sent
  *	@netdev: net device the packet is being sent from
@@ -853,13 +901,8 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	/* save the pointer to skb -- we'll need it for completion */
 	cmdrsp->net.buf = skb;
 
-	if (((devdata->chstat.sent_xmit >= devdata->chstat.got_xmit_done) &&
-	     (devdata->chstat.sent_xmit - devdata->chstat.got_xmit_done >=
-	     devdata->max_outstanding_net_xmits)) ||
-	     ((devdata->chstat.sent_xmit < devdata->chstat.got_xmit_done) &&
-	     (ULONG_MAX - devdata->chstat.got_xmit_done +
-	      devdata->chstat.sent_xmit >=
-	      devdata->max_outstanding_net_xmits))) {
+	if (vnic_hit_high_watermark(devdata,
+				    devdata->max_outstanding_net_xmits)) {
 		/* too many NET_XMITs queued over to IOVM - need to wait
 		 */
 		devdata->chstat.reject_count++;
@@ -941,13 +984,8 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	/* check to see if we have hit the high watermark for
 	 * netif_stop_queue()
 	 */
-	if (((devdata->chstat.sent_xmit >= devdata->chstat.got_xmit_done) &&
-	     (devdata->chstat.sent_xmit - devdata->chstat.got_xmit_done >=
-	      devdata->upper_threshold_net_xmits)) ||
-	    ((devdata->chstat.sent_xmit < devdata->chstat.got_xmit_done) &&
-	     (ULONG_MAX - devdata->chstat.got_xmit_done +
-	      devdata->chstat.sent_xmit >=
-	      devdata->upper_threshold_net_xmits))) {
+	if (vnic_hit_high_watermark(devdata,
+				    devdata->upper_threshold_net_xmits)) {
 		/* too many NET_XMITs queued over to IOVM - need to wait */
 		netif_stop_queue(netdev); /* calling stop queue - call
 					   * netif_wake_queue() after lower
@@ -1461,13 +1499,13 @@ static ssize_t info_debugfs_read(struct file *file, char __user *buf,
 				     " num_rcv_bufs = %d\n",
 				     devdata->num_rcv_bufs);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
-				     " max_oustanding_next_xmits = %d\n",
+				     " max_oustanding_next_xmits = %lu\n",
 				    devdata->max_outstanding_net_xmits);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
-				     " upper_threshold_net_xmits = %d\n",
+				     " upper_threshold_net_xmits = %lu\n",
 				     devdata->upper_threshold_net_xmits);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
-				     " lower_threshold_net_xmits = %d\n",
+				     " lower_threshold_net_xmits = %lu\n",
 				     devdata->lower_threshold_net_xmits);
 		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
 				     " queuefullmsg_logged = %d\n",
@@ -1566,6 +1604,9 @@ static ssize_t info_debugfs_read(struct file *file, char __user *buf,
 				     " netif_queue = %s\n",
 				     netif_queue_stopped(devdata->netdev) ?
 				     "stopped" : "running");
+		str_pos += scnprintf(vbuf + str_pos, len - str_pos,
+				     " xmits_outstanding = %lu\n",
+				     devdata_xmits_outstanding(devdata));
 	}
 	rcu_read_unlock();
 	bytes_read = simple_read_from_buffer(buf, len, offset, vbuf, str_pos);
@@ -1659,16 +1700,8 @@ service_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
 				 * the lower watermark for
 				 * netif_wake_queue()
 				 */
-				if (((devdata->chstat.sent_xmit >=
-				    devdata->chstat.got_xmit_done) &&
-				    (devdata->chstat.sent_xmit -
-				    devdata->chstat.got_xmit_done <=
-				    devdata->lower_threshold_net_xmits)) ||
-				    ((devdata->chstat.sent_xmit <
-				    devdata->chstat.got_xmit_done) &&
-				    (ULONG_MAX - devdata->chstat.got_xmit_done
-				    + devdata->chstat.sent_xmit <=
-				    devdata->lower_threshold_net_xmits))) {
+				if (vnic_hit_low_watermark(devdata,
+					devdata->lower_threshold_net_xmits)) {
 					/* enough NET_XMITs completed
 					 * so can restart netif queue
 					 */
@@ -1842,9 +1875,9 @@ static int visornic_probe(struct visor_device *dev)
 	devdata->max_outstanding_net_xmits =
 		max(3, ((devdata->num_rcv_bufs / 3) - 2));
 	devdata->upper_threshold_net_xmits =
-		max(2, devdata->max_outstanding_net_xmits - 1);
+		max(2UL, (devdata->max_outstanding_net_xmits - 1));
 	devdata->lower_threshold_net_xmits =
-		max(1, devdata->max_outstanding_net_xmits / 2);
+		max(1UL, (devdata->max_outstanding_net_xmits / 2));
 
 	skb_queue_head_init(&devdata->xmitbufhead);
 
-- 
2.1.4

Re: [PATCH 6/7] staging: unisys: visornic - consolidate+simplify xmit watermark checks

[Greg KH]

On Fri, Jul 24, 2015 at 12:00:24PM -0400, Benjamin Romer wrote:
> From: Tim Sell <Timothy.Sell@unisys.com>
> 
> The code that tests for high and low xmit watermarks was consolidated and
> simplified. The output behavior should be identical, with the exception
> of an off-by-one error being corrected in the tests done when the counters
> overflowed.
> 
> Note that the literals passed to max() macros need to be explicitly
> specified as unsigned long (UL), in order to prevent gcc warnings:
> 
>     warning: comparison of distinct pointer types lacks a cast

You do know about max_t(), right?  Please redo this with that call
instead of the odd casting.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/7] staging: unisys: visornic - consolidate+simplify xmit watermark checks

[Dan Carpenter]

On Fri, Jul 24, 2015 at 12:00:24PM -0400, Benjamin Romer wrote:
> -		max(2, devdata->max_outstanding_net_xmits - 1);
> +		max(2UL, (devdata->max_outstanding_net_xmits - 1));

It's weird here to treat ->max_outstanding_net_xmits equals zero as a
ULONG_MAX but it looks deliberate...  Perhaps add a comment?

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 7/7] staging: unisys: visornic - prevent NETDEV WATCHDOG timeouts after IO recovery

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

After IO partition recovery, it was possible to get into a situation where
a visornic device would repeatedly report:

    NETDEV WATCHDOG: eth0 (): transmit queue 0 timed out

The actual problem would affect any visornic device that was rapidly
transmitting at the same time the IO partition was being recovered. Once
you hit the problem, the only way to resume use of the nic would be to
reboot the Linux client partition.

The problem was caused by chstat.sent_xmit and chstat.got_xmit_done NOT
getting cleared during IO partition recovery. This is necessary because
outstanding xmits would essentially be "abandoned" during such recovery.
These fields are now cleared in virtnic_serverdown_complete().

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index bd58e15..4b5fc07 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -380,6 +380,8 @@ visornic_serverdown_complete(struct visornic_devdata *devdata)
 	rtnl_unlock();
 
 	atomic_set(&devdata->num_rcvbuf_in_iovm, 0);
+	devdata->chstat.sent_xmit = 0;
+	devdata->chstat.got_xmit_done = 0;
 
 	if (devdata->server_down_complete_func)
 		(*devdata->server_down_complete_func)(devdata->dev, 0);
-- 
2.1.4