[PATCH] HVCS fix to replace yield with tty_wait_until_sent in hvcs_close

Following the same advice you gave in a recent hvc_console patch I have
modified HVCS to remove a while() { yield(); } from hvcs_close() which may
cause problems where real time scheduling is concerned and replaced it with
tty_wait_until_sent() which uses a real wait queue and is the proper method
for blocking a tty operation while waiting for data to be sent.  This patch
has been tested to verify that all the paths of code that were changed were
hit during the code run and performed as expected including hotplug remove
of hvcs adapters and hangup of ttys.

- Replaced yield() in hvcs_close() with tty_wait_until_sent() to prevent
  possible lockup with realtime scheduling.

- Removed hvcs_final_close() and reordered cleanup operations to prevent
  discarding of pending data during an hvcs_close() call.

- Removed spinlock protection of hvcs_struct data members in
  hvcs_write_room() and hvcs_chars_in_buffer() because they aren't needed.
Signed-off-by: Ryan S. Arnold <rsa@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

drivers/char/hvcs.c

@@ -119,14 +119,28 @@
  * Rearranged hvcs_close().  Cleaned up some printks and did some housekeeping
  * on the changelog.  Removed local CLC_LENGTH and used HVCS_CLC_LENGTH from
  * arch/ppc64/hvcserver.h.
+ *
+ * 1.3.2 -> 1.3.3 Replaced yield() in hvcs_close() with tty_wait_until_sent() to
+ * prevent possible lockup with realtime scheduling as similarily pointed out by
+ * akpm in hvc_console.  Changed resulted in the removal of hvcs_final_close()
+ * to reorder cleanup operations and prevent discarding of pending data during
+ * an hvcs_close().  Removed spinlock protection of hvcs_struct data members in
+ * hvcs_write_room() and hvcs_chars_in_buffer() because they aren't needed.
  */
-#define HVCS_DRIVER_VERSION "1.3.2"
+
+#define HVCS_DRIVER_VERSION "1.3.3"
 
 MODULE_AUTHOR("Ryan S. Arnold <rsa@us.ibm.com>");
 MODULE_DESCRIPTION("IBM hvcs (Hypervisor Virtual Console Server) Driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HVCS_DRIVER_VERSION);
 
+/*
+ * Wait this long per iteration while trying to push buffered data to the
+ * hypervisor before allowing the tty to complete a close operation.
+ */
+#define HVCS_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
+
 /*
  * Since the Linux TTY code does not currently (2-04-2004) support dynamic
  * addition of tty derived devices and we shouldn't allocate thousands of
@@ -317,7 +331,6 @@ static void hvcs_partner_free(struct hvcs_struct *hvcsd);
 
 static int hvcs_enable_device(struct hvcs_struct *hvcsd,
 		uint32_t unit_address, unsigned int irq, struct vio_dev *dev);
-static void hvcs_final_close(struct hvcs_struct *hvcsd);
 
 static void destroy_hvcs_struct(struct kobject *kobj);
 static int hvcs_open(struct tty_struct *tty, struct file *filp);
@@ -574,28 +587,6 @@ static void destroy_hvcs_struct(struct kobject *kobj)
 	kfree(hvcsd);
 }
 
-/*
- * This function must be called with hvcsd->lock held.  Do not free the irq in
- * this function since it is called with the spinlock held.
- */
-static void hvcs_final_close(struct hvcs_struct *hvcsd)
-{
-	vio_disable_interrupts(hvcsd->vdev);
-
-	hvcsd->todo_mask = 0;
-
-	/* These two may be redundant if the operation was a close. */
-	if (hvcsd->tty) {
-		hvcsd->tty->driver_data = NULL;
-		hvcsd->tty = NULL;
-	}
-
-	hvcsd->open_count = 0;
-
-	memset(&hvcsd->buffer[0], 0x00, HVCS_BUFF_LEN);
-	hvcsd->chars_in_buffer = 0;
-}
-
 static struct kobj_type hvcs_kobj_type = {
 	.release = destroy_hvcs_struct,
 };
@@ -692,8 +683,6 @@ static int __devinit hvcs_probe(
 	return 0;
 }
 
-
-
 static int __devexit hvcs_remove(struct vio_dev *dev)
 {
 	struct hvcs_struct *hvcsd = dev->dev.driver_data;
@@ -1099,12 +1088,7 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
 	kobjp = &hvcsd->kobj;
 	if (--hvcsd->open_count == 0) {
 
-		/*
-		 * This line is important because it tells hvcs_open that this
-		 * device needs to be re-configured the next time hvcs_open is
-		 * called.
-		 */
-		hvcsd->tty->driver_data = NULL;
+		vio_disable_interrupts(hvcsd->vdev);
 
 		/*
 		 * NULL this early so that the kernel_thread doesn't try to
@@ -1113,26 +1097,18 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
 		 */
 		hvcsd->tty = NULL;
 
+		irq = hvcsd->vdev->irq;
+		spin_unlock_irqrestore(&hvcsd->lock, flags);
+
+		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
+
 		/*
-		 * Block the close until all the buffered data has been
-		 * delivered.
+		 * This line is important because it tells hvcs_open that this
+		 * device needs to be re-configured the next time hvcs_open is
+		 * called.
 		 */
-		while(hvcsd->chars_in_buffer) {
-			spin_unlock_irqrestore(&hvcsd->lock, flags);
-
-			/*
-			 * Give the kernel thread the hvcs_struct so that it can
-			 * try to deliver the remaining data but block the close
-			 * operation by spinning in this function so that other
-			 * tty operations have to wait.
-			 */
-			yield();
-			spin_lock_irqsave(&hvcsd->lock, flags);
-		}
+		tty->driver_data = NULL;
 
-		hvcs_final_close(hvcsd);
-		irq = hvcsd->vdev->irq;
-		spin_unlock_irqrestore(&hvcsd->lock, flags);
 		free_irq(irq, hvcsd);
 		kobject_put(kobjp);
 		return;
@@ -1162,12 +1138,25 @@ static void hvcs_hangup(struct tty_struct * tty)
 	 * Don't kobject put inside the spinlock because the destruction
 	 * callback may use the spinlock and it may get called before the
 	 * spinlock has been released.  Get a pointer to the kobject and
-	 * kobject_put on that instead.
+	 * kobject_put on that after releasing the spinlock.
 	 */
 	kobjp = &hvcsd->kobj;
 
-	/* Calling this will drop any buffered data on the floor. */
-	hvcs_final_close(hvcsd);
+	vio_disable_interrupts(hvcsd->vdev);
+
+	hvcsd->todo_mask = 0;
+
+	/* I don't think the tty needs the hvcs_struct pointer after a hangup */
+	hvcsd->tty->driver_data = NULL;
+	hvcsd->tty = NULL;
+
+	hvcsd->open_count = 0;
+
+	/* This will drop any buffered data on the floor which is OK in a hangup
+	 * scenario. */
+	memset(&hvcsd->buffer[0], 0x00, HVCS_BUFF_LEN);
+	hvcsd->chars_in_buffer = 0;
+
 	irq = hvcsd->vdev->irq;
 
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
@@ -1323,28 +1312,18 @@ static int hvcs_write(struct tty_struct *tty, int from_user,
 static int hvcs_write_room(struct tty_struct *tty)
 {
 	struct hvcs_struct *hvcsd = tty->driver_data;
-	unsigned long flags;
-	int retval;
 
 	if (!hvcsd || hvcsd->open_count <= 0)
 		return 0;
 
-	spin_lock_irqsave(&hvcsd->lock, flags);
-	retval = HVCS_BUFF_LEN - hvcsd->chars_in_buffer;
-	spin_unlock_irqrestore(&hvcsd->lock, flags);
-	return retval;
+	return HVCS_BUFF_LEN - hvcsd->chars_in_buffer;
 }
 
 static int hvcs_chars_in_buffer(struct tty_struct *tty)
 {
 	struct hvcs_struct *hvcsd = tty->driver_data;
-	unsigned long flags;
-	int retval;
 
-	spin_lock_irqsave(&hvcsd->lock, flags);
-	retval = hvcsd->chars_in_buffer;
-	spin_unlock_irqrestore(&hvcsd->lock, flags);
-	return retval;
+	return hvcsd->chars_in_buffer;
 }
 
 static struct tty_operations hvcs_ops = {