[PATCH 0/2] staging: speakup: factor out selection code

[PATCH 0/2] staging: speakup: factor out selection code

[Okash Khawaja]

Hi,

Speakup's selection functionality parallels that of
drivers/tty/vt/selection.c. This patch set replaces speakup's
implementation with calls to vt's selection code. This is one of the
remaining items in our TODO file and it's needed for moving speakup out
of staging.

Please note that in speakup selection is set inside interrupt context of
keyboard handler. Set selection code in vt happens in process context
and hence expects ability to sleep. To address this, there were two
options: farm out speakup's set selection into a work_struct thread, or
create atomic version of vt's set_selection. These patches implement
the former option.

Here's a summary:

Patch 1 re-arranges code in vt and exports some functions.
Patch 2 replaces speakup's selection code with calls to vt's functions
exported in patch 1.

Thanks,
Okash

[PATCH 1/2] vt: selection: allow functions to be called from inside kernel

[Okash Khawaja]

This patch breaks set_selection() into two functions so that when
called from kernel, copy_from_user() can be avoided. It also exports
set_selection() and paste_selection().

These changes are used the following patch where speakup's selection
functionality calls into the above functions, thereby doing away with
parallel implementation.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Gregory Nowak <greg@gregn.net>
---
 drivers/tty/vt/selection.c | 37 ++++++++++++++++++++++++-------------
 include/linux/selection.h  |  3 +--
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 07496c711d7d..a43f9cd9bdd6 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -80,6 +80,7 @@ void clear_selection(void)
 		sel_start = -1;
 	}
 }
+EXPORT_SYMBOL_GPL(clear_selection);
 
 /*
  * User settable table: what characters are to be considered alphabetic?
@@ -164,34 +165,42 @@ static int store_utf8(u32 c, char *p)
  *	 a lot under the lock but its hardly a performance path
  */
 int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty)
+{
+	struct tiocl_selection v;
+
+	if (copy_from_user(&v, sel, sizeof(*sel)))
+		return -EFAULT;
+
+	return do_set_selection(&v, tty);
+}
+
+int do_set_selection(struct tiocl_selection *v, struct tty_struct *tty)
 {
 	struct vc_data *vc = vc_cons[fg_console].d;
 	int new_sel_start, new_sel_end, spc;
-	struct tiocl_selection v;
 	char *bp, *obp;
 	int i, ps, pe, multiplier;
 	u32 c;
 	int mode;
 
 	poke_blanked_console();
-	if (copy_from_user(&v, sel, sizeof(*sel)))
-		return -EFAULT;
 
-	v.xs = min_t(u16, v.xs - 1, vc->vc_cols - 1);
-	v.ys = min_t(u16, v.ys - 1, vc->vc_rows - 1);
-	v.xe = min_t(u16, v.xe - 1, vc->vc_cols - 1);
-	v.ye = min_t(u16, v.ye - 1, vc->vc_rows - 1);
-	ps = v.ys * vc->vc_size_row + (v.xs << 1);
-	pe = v.ye * vc->vc_size_row + (v.xe << 1);
+	v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
+	v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
+	v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
+	v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+	ps = v->ys * vc->vc_size_row + (v->xs << 1);
+	pe = v->ye * vc->vc_size_row + (v->xe << 1);
 
-	if (v.sel_mode == TIOCL_SELCLEAR) {
+	if (v->sel_mode == TIOCL_SELCLEAR) {
 		/* useful for screendump without selection highlights */
 		clear_selection();
 		return 0;
 	}
 
-	if (mouse_reporting() && (v.sel_mode & TIOCL_SELMOUSEREPORT)) {
-		mouse_report(tty, v.sel_mode & TIOCL_SELBUTTONMASK, v.xs, v.ys);
+	if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
+		mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
+			     v->ys);
 		return 0;
 	}
 
@@ -208,7 +217,7 @@ int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *t
 	else
 		use_unicode = 0;
 
-	switch (v.sel_mode)
+	switch (v->sel_mode)
 	{
 		case TIOCL_SELCHAR:	/* character-by-character selection */
 			new_sel_start = ps;
@@ -322,6 +331,7 @@ int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *t
 	sel_buffer_lth = bp - sel_buffer;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(do_set_selection);
 
 /* Insert the contents of the selection buffer into the
  * queue of the tty associated with the current console.
@@ -367,3 +377,4 @@ int paste_selection(struct tty_struct *tty)
 	tty_ldisc_deref(ld);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(paste_selection);
diff --git a/include/linux/selection.h b/include/linux/selection.h
index a8f5b97b216f..171d77dfc825 100644
--- a/include/linux/selection.h
+++ b/include/linux/selection.h
@@ -11,13 +11,12 @@
 #include <linux/tiocl.h>
 #include <linux/vt_buffer.h>
 
-struct tty_struct;
-
 extern struct vc_data *sel_cons;
 struct tty_struct;
 
 extern void clear_selection(void);
 extern int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty);
+extern int do_set_selection(struct tiocl_selection *v, struct tty_struct *tty);
 extern int paste_selection(struct tty_struct *tty);
 extern int sel_loadlut(char __user *p);
 extern int mouse_reporting(void);
-- 
2.21.0

Re: [PATCH 1/2] vt: selection: allow functions to be called from inside kernel

[Greg Kroah-Hartman]

On Thu, Apr 04, 2019 at 08:45:29PM +0100, Okash Khawaja wrote:
> diff --git a/include/linux/selection.h b/include/linux/selection.h
> index a8f5b97b216f..171d77dfc825 100644
> --- a/include/linux/selection.h
> +++ b/include/linux/selection.h
> @@ -11,13 +11,12 @@
>  #include <linux/tiocl.h>
>  #include <linux/vt_buffer.h>
>  
> -struct tty_struct;
> -

Nit, why remove these lines?

thanks,

greg k-h

Re: [PATCH 1/2] vt: selection: allow functions to be called from inside kernel

[Samuel Thibault]

Greg KH, le jeu. 04 avril 2019 22:35:49 +0200, a ecrit:
> On Thu, Apr 04, 2019 at 08:45:29PM +0100, Okash Khawaja wrote:
> > diff --git a/include/linux/selection.h b/include/linux/selection.h
> > index a8f5b97b216f..171d77dfc825 100644
> > --- a/include/linux/selection.h
> > +++ b/include/linux/selection.h
> > @@ -11,13 +11,12 @@
> >  #include <linux/tiocl.h>
> >  #include <linux/vt_buffer.h>
> >  
> > -struct tty_struct;
> > -
> 
> Nit, why remove these lines?

The same line shows up already just a couple of lines below, so it's
just a clean-up along the way.

Samuel

Re: [PATCH 1/2] vt: selection: allow functions to be called from inside kernel

[Greg Kroah-Hartman]

On Thu, Apr 04, 2019 at 08:45:29PM +0100, Okash Khawaja wrote:
> This patch breaks set_selection() into two functions so that when
> called from kernel, copy_from_user() can be avoided. It also exports
> set_selection() and paste_selection().
> 
> These changes are used the following patch where speakup's selection
> functionality calls into the above functions, thereby doing away with
> parallel implementation.
> 
> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Tested-by: Gregory Nowak <greg@gregn.net>
> ---
>  drivers/tty/vt/selection.c | 37 ++++++++++++++++++++++++-------------
>  include/linux/selection.h  |  3 +--
>  2 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> index 07496c711d7d..a43f9cd9bdd6 100644
> --- a/drivers/tty/vt/selection.c
> +++ b/drivers/tty/vt/selection.c
> @@ -80,6 +80,7 @@ void clear_selection(void)
>  		sel_start = -1;
>  	}
>  }
> +EXPORT_SYMBOL_GPL(clear_selection);
>  
>  /*
>   * User settable table: what characters are to be considered alphabetic?
> @@ -164,34 +165,42 @@ static int store_utf8(u32 c, char *p)
>   *	 a lot under the lock but its hardly a performance path
>   */
>  int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty)
> +{
> +	struct tiocl_selection v;
> +
> +	if (copy_from_user(&v, sel, sizeof(*sel)))
> +		return -EFAULT;
> +
> +	return do_set_selection(&v, tty);
> +}
> +
> +int do_set_selection(struct tiocl_selection *v, struct tty_struct *tty)

I have no idea what the difference is between set_selection() and
do_set_selection() is now.  Naming is hard, I know :(

How about set_selection_kernel()?  set_selection_tiocl()?

Something to show that one takes a userspace pointer, and the other a
kernel pointer, how about:
	set_selection_user()
	set_selection_kernel()
making it more obvious?

thanks,

greg k-h

[PATCH 2/2] staging: speakup: refactor to use existing code in vt

[Okash Khawaja]

This patch replaces speakup's implementations with calls to functions
in tty/vt/selection.c. Those functions are:

cancel_selection()
do_set_selection()
paste_selection()

Currently setting selection is done in interrupt context. However,
do_set_selection() can sleep - for instance, it requires console_lock
which can sleep. Therefore we offload that work to a work_struct thread,
following the same pattern which was already set for paste_selection().
When setting selection, we also get a reference to tty and make sure to
release the reference before returning.

struct speakup_paste_work has been renamed to the more generic
speakup_selection_work because it is now used for both pasting as well
as setting selection. When paste work is cancelled, the code wasn't
setting tty to NULL. This patch also fixes that by setting tty to NULL
so that in case of failure we don't get EBUSY forever.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Gregory Nowak <greg@gregn.net>
---
 drivers/staging/speakup/main.c      |   1 +
 drivers/staging/speakup/selection.c | 212 +++++++++++-----------------
 drivers/staging/speakup/speakup.h   |   1 +
 3 files changed, 88 insertions(+), 126 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index b6a65b0c1896..488f2539aa9a 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2319,6 +2319,7 @@ static void __exit speakup_exit(void)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	speakup_unregister_devsynth();
+	speakup_cancel_selection();
 	speakup_cancel_paste();
 	del_timer_sync(&cursor_timer);
 	kthread_stop(speakup_task);
diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index 0ed1fefee0e9..6c6d77f8bc24 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -9,178 +9,138 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/atomic.h>
+#include <linux/console.h>
 
 #include "speakup.h"
 
-/* ------ cut and paste ----- */
-/* Don't take this from <ctype.h>: 011-015 on the screen aren't spaces */
-#define ishardspace(c)      ((c) == ' ')
-
 unsigned short spk_xs, spk_ys, spk_xe, spk_ye; /* our region points */
-
-/* Variables for selection control. */
-/* must not be deallocated */
 struct vc_data *spk_sel_cons;
-/* cleared by clear_selection */
-static int sel_start = -1;
-static int sel_end;
-static int sel_buffer_lth;
-static char *sel_buffer;
 
-static unsigned char sel_pos(int n)
-{
-	return inverse_translate(spk_sel_cons,
-		screen_glyph(spk_sel_cons, n), 0);
-}
+struct speakup_selection_work {
+	struct work_struct work;
+	struct tiocl_selection sel;
+	struct tty_struct *tty;
+};
 
 void speakup_clear_selection(void)
 {
-	sel_start = -1;
+	console_lock();
+	clear_selection();
+	console_unlock();
 }
 
-/* does screen address p correspond to character at LH/RH edge of screen? */
-static int atedge(const int p, int size_row)
+static void __speakup_set_selection(struct work_struct *work)
 {
-	return !(p % size_row) || !((p + 2) % size_row);
-}
+	struct speakup_selection_work *ssw =
+		container_of(work, struct speakup_selection_work, work);
 
-/* constrain v such that v <= u */
-static unsigned short limit(const unsigned short v, const unsigned short u)
-{
-	return (v > u) ? u : v;
-}
+	struct tty_struct *tty;
+	struct tiocl_selection sel;
 
-int speakup_set_selection(struct tty_struct *tty)
-{
-	int new_sel_start, new_sel_end;
-	char *bp, *obp;
-	int i, ps, pe;
-	struct vc_data *vc = vc_cons[fg_console].d;
+	sel = ssw->sel;
 
-	spk_xs = limit(spk_xs, vc->vc_cols - 1);
-	spk_ys = limit(spk_ys, vc->vc_rows - 1);
-	spk_xe = limit(spk_xe, vc->vc_cols - 1);
-	spk_ye = limit(spk_ye, vc->vc_rows - 1);
-	ps = spk_ys * vc->vc_size_row + (spk_xs << 1);
-	pe = spk_ye * vc->vc_size_row + (spk_xe << 1);
+	/* this ensures we copy sel before releasing the lock below */
+	rmb();
 
-	if (ps > pe)	/* make sel_start <= sel_end */
-		swap(ps, pe);
+	/* release the lock by setting tty of the struct to NULL */
+	tty = xchg(&ssw->tty, NULL);
 
 	if (spk_sel_cons != vc_cons[fg_console].d) {
-		speakup_clear_selection();
 		spk_sel_cons = vc_cons[fg_console].d;
-		dev_warn(tty->dev,
-			 "Selection: mark console not the same as cut\n");
-		return -EINVAL;
+		pr_warn("Selection: mark console not the same as cut\n");
+		goto unref;
 	}
 
-	new_sel_start = ps;
-	new_sel_end = pe;
-
-	/* select to end of line if on trailing space */
-	if (new_sel_end > new_sel_start &&
-	    !atedge(new_sel_end, vc->vc_size_row) &&
-	    ishardspace(sel_pos(new_sel_end))) {
-		for (pe = new_sel_end + 2; ; pe += 2)
-			if (!ishardspace(sel_pos(pe)) ||
-			    atedge(pe, vc->vc_size_row))
-				break;
-		if (ishardspace(sel_pos(pe)))
-			new_sel_end = pe;
-	}
-	if ((new_sel_start == sel_start) && (new_sel_end == sel_end))
-		return 0; /* no action required */
-
-	sel_start = new_sel_start;
-	sel_end = new_sel_end;
-	/* Allocate a new buffer before freeing the old one ... */
-	bp = kmalloc((sel_end - sel_start) / 2 + 1, GFP_ATOMIC);
-	if (!bp) {
-		speakup_clear_selection();
-		return -ENOMEM;
-	}
-	kfree(sel_buffer);
-	sel_buffer = bp;
-
-	obp = bp;
-	for (i = sel_start; i <= sel_end; i += 2) {
-		*bp = sel_pos(i);
-		if (!ishardspace(*bp++))
-			obp = bp;
-		if (!((i + 2) % vc->vc_size_row)) {
-			/* strip trailing blanks from line and add newline,
-			 * unless non-space at end of line.
-			 */
-			if (obp != bp) {
-				bp = obp;
-				*bp++ = '\r';
-			}
-			obp = bp;
-		}
+	console_lock();
+	do_set_selection(&sel, tty);
+	console_unlock();
+
+unref:
+	tty_kref_put(tty);
+}
+
+static struct speakup_selection_work speakup_sel_work = {
+	.work = __WORK_INITIALIZER(speakup_sel_work.work,
+				   __speakup_set_selection)
+};
+
+int speakup_set_selection(struct tty_struct *tty)
+{
+	/* we get kref here first in order to avoid a subtle race when
+	 * cancelling selection work. getting kref first establishes the
+	 * invariant that if speakup_sel_work.tty is not NULL when
+	 * speakup_cancel_selection() is called, it must be the case that a put
+	 * kref is pending.
+	 */
+	tty_kref_get(tty);
+	if (cmpxchg(&speakup_sel_work.tty, NULL, tty)) {
+		tty_kref_put(tty);
+		return -EBUSY;
 	}
-	sel_buffer_lth = bp - sel_buffer;
+	/* now we have the 'lock' by setting tty member of
+	 * speakup_selection_work. wmb() ensures that writes to
+	 * speakup_sel_work don't happen before cmpxchg() above.
+	 */
+	wmb();
+
+	speakup_sel_work.sel.xs = spk_xs + 1;
+	speakup_sel_work.sel.ys = spk_ys + 1;
+	speakup_sel_work.sel.xe = spk_xe + 1;
+	speakup_sel_work.sel.ye = spk_ye + 1;
+	speakup_sel_work.sel.sel_mode = TIOCL_SELCHAR;
+
+	schedule_work_on(WORK_CPU_UNBOUND, &speakup_sel_work.work);
+
 	return 0;
 }
 
-struct speakup_paste_work {
-	struct work_struct work;
+void speakup_cancel_selection(void)
+{
 	struct tty_struct *tty;
-};
+
+	cancel_work_sync(&speakup_sel_work.work);
+	/* setting to null so that if work fails to run and we cancel it,
+	 * we can run it again without getting EBUSY forever from there on.
+	 * we need to use xchg here to avoid race with speakup_set_selection()
+	 */
+	tty = xchg(&speakup_sel_work.tty, NULL);
+	if (tty)
+		tty_kref_put(tty);
+}
 
 static void __speakup_paste_selection(struct work_struct *work)
 {
-	struct speakup_paste_work *spw =
-		container_of(work, struct speakup_paste_work, work);
-	struct tty_struct *tty = xchg(&spw->tty, NULL);
-	struct vc_data *vc = (struct vc_data *)tty->driver_data;
-	int pasted = 0, count;
-	struct tty_ldisc *ld;
-	DECLARE_WAITQUEUE(wait, current);
-
-	ld = tty_ldisc_ref(tty);
-	if (!ld)
-		goto tty_unref;
-	tty_buffer_lock_exclusive(&vc->port);
-
-	add_wait_queue(&vc->paste_wait, &wait);
-	while (sel_buffer && sel_buffer_lth > pasted) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (tty_throttled(tty)) {
-			schedule();
-			continue;
-		}
-		count = sel_buffer_lth - pasted;
-		count = tty_ldisc_receive_buf(ld, sel_buffer + pasted, NULL,
-					      count);
-		pasted += count;
-	}
-	remove_wait_queue(&vc->paste_wait, &wait);
-	__set_current_state(TASK_RUNNING);
+	struct speakup_selection_work *ssw =
+		container_of(work, struct speakup_selection_work, work);
+	struct tty_struct *tty = xchg(&ssw->tty, NULL);
 
-	tty_buffer_unlock_exclusive(&vc->port);
-	tty_ldisc_deref(ld);
-tty_unref:
+	paste_selection(tty);
 	tty_kref_put(tty);
 }
 
-static struct speakup_paste_work speakup_paste_work = {
+static struct speakup_selection_work speakup_paste_work = {
 	.work = __WORK_INITIALIZER(speakup_paste_work.work,
 				   __speakup_paste_selection)
 };
 
 int speakup_paste_selection(struct tty_struct *tty)
 {
-	if (cmpxchg(&speakup_paste_work.tty, NULL, tty))
+	tty_kref_get(tty);
+	if (cmpxchg(&speakup_paste_work.tty, NULL, tty)) {
+		tty_kref_put(tty);
 		return -EBUSY;
+	}
 
-	tty_kref_get(tty);
 	schedule_work_on(WORK_CPU_UNBOUND, &speakup_paste_work.work);
 	return 0;
 }
 
 void speakup_cancel_paste(void)
 {
+	struct tty_struct *tty;
+
 	cancel_work_sync(&speakup_paste_work.work);
-	tty_kref_put(speakup_paste_work.tty);
+	tty = xchg(&speakup_paste_work.tty, NULL);
+	if (tty)
+		tty_kref_put(tty);
 }
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index e4f4f00be2dc..74fe49c2c511 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -72,6 +72,7 @@ void synth_buffer_add(u16 ch);
 void synth_buffer_clear(void);
 void speakup_clear_selection(void);
 int speakup_set_selection(struct tty_struct *tty);
+void speakup_cancel_selection(void);
 int speakup_paste_selection(struct tty_struct *tty);
 void speakup_cancel_paste(void);
 void speakup_register_devsynth(void);
-- 
2.21.0

[PATCH v2 0/2] staging: speakup: factor out selection code

[Okash Khawaja]

Hi,

The v2 renames set_selection() and do_set_selection() to following 
more explicit names:

set_selection_user() /* includes copying data from user space */
set_selection_kernel() /* no copying from user space */

The patches also update references to set_selection() to be 
set_selection_user().

Original intro:

Speakup's selection functionality parallels that of  
drivers/tty/vt/selection.c. This patch set replaces speakup's
implementation with calls to vt's selection code. This is one of the
remaining items in our TODO file and it's needed for moving speakup out
of staging.

Please note that in speakup selection is set inside interrupt context of
keyboard handler. Set selection code in vt happens in process context
and hence expects ability to sleep. To address this, there were two
options: farm out speakup's set selection into a work_struct thread, or
create atomic version of vt's set_selection. These patches implement
the former option.

Here's a summary:

Patch 1 re-arranges code in vt and exports some functions.
Patch 2 replaces speakup's selection code with calls to vt's functions
exported in patch 1.

Thanks,
Okash 

[PATCH v2 1/2] vt: selection: allow functions to be called from inside kernel

[Okash Khawaja]

This patch breaks set_selection() into two functions so that when
called from kernel, copy_from_user() can be avoided. The two functions
are called set_selection_user() and set_selection_kernel() in order to
be explicit about their purposes. This also means updating any
references to set_selection() and fixing for name change. It also
exports set_selection_kernel() and paste_selection().

These changes are used the following patch where speakup's selection
functionality calls into the above functions, thereby doing away with
parallel implementation.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Gregory Nowak <greg@gregn.net>
---
 drivers/tty/vt/selection.c | 46 ++++++++++++++++++++++++++++++----------------
 drivers/tty/vt/vt.c        |  7 ++++---
 include/linux/selection.h  |  7 ++++---
 3 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 07496c7..78732fe 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -2,7 +2,9 @@
 /*
  * This module exports the functions:
  *
- *     'int set_selection(struct tiocl_selection __user *, struct tty_struct *)'
+ *     'int set_selection_user(struct tiocl_selection __user *,
+ *			       struct tty_struct *)'
+ *     'int set_selection_kernel(struct tiocl_selection *, struct tty_struct *)'
  *     'void clear_selection(void)'
  *     'int paste_selection(struct tty_struct *)'
  *     'int sel_loadlut(char __user *)'
@@ -80,6 +82,7 @@ void clear_selection(void)
 		sel_start = -1;
 	}
 }
+EXPORT_SYMBOL_GPL(clear_selection);
 
 /*
  * User settable table: what characters are to be considered alphabetic?
@@ -154,7 +157,7 @@ static int store_utf8(u32 c, char *p)
 }
 
 /**
- *	set_selection		- 	set the current selection.
+ *	set_selection_user	-	set the current selection.
  *	@sel: user selection info
  *	@tty: the console tty
  *
@@ -163,35 +166,44 @@ static int store_utf8(u32 c, char *p)
  *	The entire selection process is managed under the console_lock. It's
  *	 a lot under the lock but its hardly a performance path
  */
-int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty)
+int set_selection_user(const struct tiocl_selection __user *sel,
+		       struct tty_struct *tty)
+{
+	struct tiocl_selection v;
+
+	if (copy_from_user(&v, sel, sizeof(*sel)))
+		return -EFAULT;
+
+	return set_selection_kernel(&v, tty);
+}
+
+int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
 {
 	struct vc_data *vc = vc_cons[fg_console].d;
 	int new_sel_start, new_sel_end, spc;
-	struct tiocl_selection v;
 	char *bp, *obp;
 	int i, ps, pe, multiplier;
 	u32 c;
 	int mode;
 
 	poke_blanked_console();
-	if (copy_from_user(&v, sel, sizeof(*sel)))
-		return -EFAULT;
 
-	v.xs = min_t(u16, v.xs - 1, vc->vc_cols - 1);
-	v.ys = min_t(u16, v.ys - 1, vc->vc_rows - 1);
-	v.xe = min_t(u16, v.xe - 1, vc->vc_cols - 1);
-	v.ye = min_t(u16, v.ye - 1, vc->vc_rows - 1);
-	ps = v.ys * vc->vc_size_row + (v.xs << 1);
-	pe = v.ye * vc->vc_size_row + (v.xe << 1);
+	v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
+	v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
+	v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
+	v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+	ps = v->ys * vc->vc_size_row + (v->xs << 1);
+	pe = v->ye * vc->vc_size_row + (v->xe << 1);
 
-	if (v.sel_mode == TIOCL_SELCLEAR) {
+	if (v->sel_mode == TIOCL_SELCLEAR) {
 		/* useful for screendump without selection highlights */
 		clear_selection();
 		return 0;
 	}
 
-	if (mouse_reporting() && (v.sel_mode & TIOCL_SELMOUSEREPORT)) {
-		mouse_report(tty, v.sel_mode & TIOCL_SELBUTTONMASK, v.xs, v.ys);
+	if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
+		mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
+			     v->ys);
 		return 0;
 	}
 
@@ -208,7 +220,7 @@ int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *t
 	else
 		use_unicode = 0;
 
-	switch (v.sel_mode)
+	switch (v->sel_mode)
 	{
 		case TIOCL_SELCHAR:	/* character-by-character selection */
 			new_sel_start = ps;
@@ -322,6 +334,7 @@ int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *t
 	sel_buffer_lth = bp - sel_buffer;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(set_selection_kernel);
 
 /* Insert the contents of the selection buffer into the
  * queue of the tty associated with the current console.
@@ -367,3 +380,4 @@ int paste_selection(struct tty_struct *tty)
 	tty_ldisc_deref(ld);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(paste_selection);
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index d34984a..f3c369a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1804,7 +1804,7 @@ void mouse_report(struct tty_struct *tty, int butt, int mrx, int mry)
 	respond_string(buf, tty->port);
 }
 
-/* invoked via ioctl(TIOCLINUX) and through set_selection */
+/* invoked via ioctl(TIOCLINUX) and through set_selection_user */
 int mouse_reporting(void)
 {
 	return vc_cons[fg_console].d->vc_report_mouse;
@@ -3008,7 +3008,7 @@ static struct tty_driver *vt_console_device(struct console *c, int *index)
  * There are some functions which can sleep for arbitrary periods
  * (paste_selection) but we don't need the lock there anyway.
  *
- * set_selection has locking, and definitely needs it
+ * set_selection_user has locking, and definitely needs it
  */
 
 int tioclinux(struct tty_struct *tty, unsigned long arg)
@@ -3028,7 +3028,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 	{
 		case TIOCL_SETSEL:
 			console_lock();
-			ret = set_selection((struct tiocl_selection __user *)(p+1), tty);
+			ret = set_selection_user((struct tiocl_selection
+						 __user *)(p+1), tty);
 			console_unlock();
 			break;
 		case TIOCL_PASTESEL:
diff --git a/include/linux/selection.h b/include/linux/selection.h
index a8f5b97..e2c1f96 100644
--- a/include/linux/selection.h
+++ b/include/linux/selection.h
@@ -11,13 +11,14 @@
 #include <linux/tiocl.h>
 #include <linux/vt_buffer.h>
 
-struct tty_struct;
-
 extern struct vc_data *sel_cons;
 struct tty_struct;
 
 extern void clear_selection(void);
-extern int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty);
+extern int set_selection_user(const struct tiocl_selection __user *sel,
+			      struct tty_struct *tty);
+extern int set_selection_kernel(struct tiocl_selection *v,
+				struct tty_struct *tty);
 extern int paste_selection(struct tty_struct *tty);
 extern int sel_loadlut(char __user *p);
 extern int mouse_reporting(void);
-- 
1.8.3.1

[PATCH v2 2/2] staging: speakup: refactor to use existing code in vt

[Okash Khawaja]

This patch replaces speakup's implementations with calls to functions
in tty/vt/selection.c. Those functions are:

cancel_selection()
set_selection_kernel()
paste_selection()

Currently setting selection is done in interrupt context. However,
set_selection_kernel() can sleep - for instance, it requires console_lock
which can sleep. Therefore we offload that work to a work_struct thread,
following the same pattern which was already set for paste_selection().
When setting selection, we also get a reference to tty and make sure to
release the reference before returning.

struct speakup_paste_work has been renamed to the more generic
speakup_selection_work because it is now used for both pasting as well
as setting selection. When paste work is cancelled, the code wasn't
setting tty to NULL. This patch also fixes that by setting tty to NULL
so that in case of failure we don't get EBUSY forever.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Gregory Nowak <greg@gregn.net>
---
 drivers/staging/speakup/main.c      |   1 +
 drivers/staging/speakup/selection.c | 212 +++++++++++++++---------------------
 drivers/staging/speakup/speakup.h   |   1 +
 3 files changed, 88 insertions(+), 126 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index b6a65b0..488f253 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2319,6 +2319,7 @@ static void __exit speakup_exit(void)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	speakup_unregister_devsynth();
+	speakup_cancel_selection();
 	speakup_cancel_paste();
 	del_timer_sync(&cursor_timer);
 	kthread_stop(speakup_task);
diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index 0ed1fef..a8b4d0c 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -9,178 +9,138 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/atomic.h>
+#include <linux/console.h>
 
 #include "speakup.h"
 
-/* ------ cut and paste ----- */
-/* Don't take this from <ctype.h>: 011-015 on the screen aren't spaces */
-#define ishardspace(c)      ((c) == ' ')
-
 unsigned short spk_xs, spk_ys, spk_xe, spk_ye; /* our region points */
-
-/* Variables for selection control. */
-/* must not be deallocated */
 struct vc_data *spk_sel_cons;
-/* cleared by clear_selection */
-static int sel_start = -1;
-static int sel_end;
-static int sel_buffer_lth;
-static char *sel_buffer;
 
-static unsigned char sel_pos(int n)
-{
-	return inverse_translate(spk_sel_cons,
-		screen_glyph(spk_sel_cons, n), 0);
-}
+struct speakup_selection_work {
+	struct work_struct work;
+	struct tiocl_selection sel;
+	struct tty_struct *tty;
+};
 
 void speakup_clear_selection(void)
 {
-	sel_start = -1;
+	console_lock();
+	clear_selection();
+	console_unlock();
 }
 
-/* does screen address p correspond to character at LH/RH edge of screen? */
-static int atedge(const int p, int size_row)
+static void __speakup_set_selection(struct work_struct *work)
 {
-	return !(p % size_row) || !((p + 2) % size_row);
-}
+	struct speakup_selection_work *ssw =
+		container_of(work, struct speakup_selection_work, work);
 
-/* constrain v such that v <= u */
-static unsigned short limit(const unsigned short v, const unsigned short u)
-{
-	return (v > u) ? u : v;
-}
+	struct tty_struct *tty;
+	struct tiocl_selection sel;
 
-int speakup_set_selection(struct tty_struct *tty)
-{
-	int new_sel_start, new_sel_end;
-	char *bp, *obp;
-	int i, ps, pe;
-	struct vc_data *vc = vc_cons[fg_console].d;
+	sel = ssw->sel;
 
-	spk_xs = limit(spk_xs, vc->vc_cols - 1);
-	spk_ys = limit(spk_ys, vc->vc_rows - 1);
-	spk_xe = limit(spk_xe, vc->vc_cols - 1);
-	spk_ye = limit(spk_ye, vc->vc_rows - 1);
-	ps = spk_ys * vc->vc_size_row + (spk_xs << 1);
-	pe = spk_ye * vc->vc_size_row + (spk_xe << 1);
+	/* this ensures we copy sel before releasing the lock below */
+	rmb();
 
-	if (ps > pe)	/* make sel_start <= sel_end */
-		swap(ps, pe);
+	/* release the lock by setting tty of the struct to NULL */
+	tty = xchg(&ssw->tty, NULL);
 
 	if (spk_sel_cons != vc_cons[fg_console].d) {
-		speakup_clear_selection();
 		spk_sel_cons = vc_cons[fg_console].d;
-		dev_warn(tty->dev,
-			 "Selection: mark console not the same as cut\n");
-		return -EINVAL;
+		pr_warn("Selection: mark console not the same as cut\n");
+		goto unref;
 	}
 
-	new_sel_start = ps;
-	new_sel_end = pe;
-
-	/* select to end of line if on trailing space */
-	if (new_sel_end > new_sel_start &&
-	    !atedge(new_sel_end, vc->vc_size_row) &&
-	    ishardspace(sel_pos(new_sel_end))) {
-		for (pe = new_sel_end + 2; ; pe += 2)
-			if (!ishardspace(sel_pos(pe)) ||
-			    atedge(pe, vc->vc_size_row))
-				break;
-		if (ishardspace(sel_pos(pe)))
-			new_sel_end = pe;
-	}
-	if ((new_sel_start == sel_start) && (new_sel_end == sel_end))
-		return 0; /* no action required */
-
-	sel_start = new_sel_start;
-	sel_end = new_sel_end;
-	/* Allocate a new buffer before freeing the old one ... */
-	bp = kmalloc((sel_end - sel_start) / 2 + 1, GFP_ATOMIC);
-	if (!bp) {
-		speakup_clear_selection();
-		return -ENOMEM;
-	}
-	kfree(sel_buffer);
-	sel_buffer = bp;
-
-	obp = bp;
-	for (i = sel_start; i <= sel_end; i += 2) {
-		*bp = sel_pos(i);
-		if (!ishardspace(*bp++))
-			obp = bp;
-		if (!((i + 2) % vc->vc_size_row)) {
-			/* strip trailing blanks from line and add newline,
-			 * unless non-space at end of line.
-			 */
-			if (obp != bp) {
-				bp = obp;
-				*bp++ = '\r';
-			}
-			obp = bp;
-		}
+	console_lock();
+	set_selection_kernel(&sel, tty);
+	console_unlock();
+
+unref:
+	tty_kref_put(tty);
+}
+
+static struct speakup_selection_work speakup_sel_work = {
+	.work = __WORK_INITIALIZER(speakup_sel_work.work,
+				   __speakup_set_selection)
+};
+
+int speakup_set_selection(struct tty_struct *tty)
+{
+	/* we get kref here first in order to avoid a subtle race when
+	 * cancelling selection work. getting kref first establishes the
+	 * invariant that if speakup_sel_work.tty is not NULL when
+	 * speakup_cancel_selection() is called, it must be the case that a put
+	 * kref is pending.
+	 */
+	tty_kref_get(tty);
+	if (cmpxchg(&speakup_sel_work.tty, NULL, tty)) {
+		tty_kref_put(tty);
+		return -EBUSY;
 	}
-	sel_buffer_lth = bp - sel_buffer;
+	/* now we have the 'lock' by setting tty member of
+	 * speakup_selection_work. wmb() ensures that writes to
+	 * speakup_sel_work don't happen before cmpxchg() above.
+	 */
+	wmb();
+
+	speakup_sel_work.sel.xs = spk_xs + 1;
+	speakup_sel_work.sel.ys = spk_ys + 1;
+	speakup_sel_work.sel.xe = spk_xe + 1;
+	speakup_sel_work.sel.ye = spk_ye + 1;
+	speakup_sel_work.sel.sel_mode = TIOCL_SELCHAR;
+
+	schedule_work_on(WORK_CPU_UNBOUND, &speakup_sel_work.work);
+
 	return 0;
 }
 
-struct speakup_paste_work {
-	struct work_struct work;
+void speakup_cancel_selection(void)
+{
 	struct tty_struct *tty;
-};
+
+	cancel_work_sync(&speakup_sel_work.work);
+	/* setting to null so that if work fails to run and we cancel it,
+	 * we can run it again without getting EBUSY forever from there on.
+	 * we need to use xchg here to avoid race with speakup_set_selection()
+	 */
+	tty = xchg(&speakup_sel_work.tty, NULL);
+	if (tty)
+		tty_kref_put(tty);
+}
 
 static void __speakup_paste_selection(struct work_struct *work)
 {
-	struct speakup_paste_work *spw =
-		container_of(work, struct speakup_paste_work, work);
-	struct tty_struct *tty = xchg(&spw->tty, NULL);
-	struct vc_data *vc = (struct vc_data *)tty->driver_data;
-	int pasted = 0, count;
-	struct tty_ldisc *ld;
-	DECLARE_WAITQUEUE(wait, current);
-
-	ld = tty_ldisc_ref(tty);
-	if (!ld)
-		goto tty_unref;
-	tty_buffer_lock_exclusive(&vc->port);
-
-	add_wait_queue(&vc->paste_wait, &wait);
-	while (sel_buffer && sel_buffer_lth > pasted) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (tty_throttled(tty)) {
-			schedule();
-			continue;
-		}
-		count = sel_buffer_lth - pasted;
-		count = tty_ldisc_receive_buf(ld, sel_buffer + pasted, NULL,
-					      count);
-		pasted += count;
-	}
-	remove_wait_queue(&vc->paste_wait, &wait);
-	__set_current_state(TASK_RUNNING);
+	struct speakup_selection_work *ssw =
+		container_of(work, struct speakup_selection_work, work);
+	struct tty_struct *tty = xchg(&ssw->tty, NULL);
 
-	tty_buffer_unlock_exclusive(&vc->port);
-	tty_ldisc_deref(ld);
-tty_unref:
+	paste_selection(tty);
 	tty_kref_put(tty);
 }
 
-static struct speakup_paste_work speakup_paste_work = {
+static struct speakup_selection_work speakup_paste_work = {
 	.work = __WORK_INITIALIZER(speakup_paste_work.work,
 				   __speakup_paste_selection)
 };
 
 int speakup_paste_selection(struct tty_struct *tty)
 {
-	if (cmpxchg(&speakup_paste_work.tty, NULL, tty))
+	tty_kref_get(tty);
+	if (cmpxchg(&speakup_paste_work.tty, NULL, tty)) {
+		tty_kref_put(tty);
 		return -EBUSY;
+	}
 
-	tty_kref_get(tty);
 	schedule_work_on(WORK_CPU_UNBOUND, &speakup_paste_work.work);
 	return 0;
 }
 
 void speakup_cancel_paste(void)
 {
+	struct tty_struct *tty;
+
 	cancel_work_sync(&speakup_paste_work.work);
-	tty_kref_put(speakup_paste_work.tty);
+	tty = xchg(&speakup_paste_work.tty, NULL);
+	if (tty)
+		tty_kref_put(tty);
 }
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index e4f4f00..74fe49c 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -72,6 +72,7 @@
 void synth_buffer_clear(void);
 void speakup_clear_selection(void);
 int speakup_set_selection(struct tty_struct *tty);
+void speakup_cancel_selection(void);
 int speakup_paste_selection(struct tty_struct *tty);
 void speakup_cancel_paste(void);
 void speakup_register_devsynth(void);
-- 
1.8.3.1

Re: [PATCH v2 0/2] staging: speakup: factor out selection code

[Greg Kroah-Hartman]

On Wed, Apr 17, 2019 at 01:21:12PM +0100, Okash Khawaja wrote:
> Hi,
> 
> The v2 renames set_selection() and do_set_selection() to following 
> more explicit names:
> 
> set_selection_user() /* includes copying data from user space */
> set_selection_kernel() /* no copying from user space */
> 
> The patches also update references to set_selection() to be 
> set_selection_user().
> 
> Original intro:
> 
> Speakup's selection functionality parallels that of  
> drivers/tty/vt/selection.c. This patch set replaces speakup's
> implementation with calls to vt's selection code. This is one of the
> remaining items in our TODO file and it's needed for moving speakup out
> of staging.
> 
> Please note that in speakup selection is set inside interrupt context of
> keyboard handler. Set selection code in vt happens in process context
> and hence expects ability to sleep. To address this, there were two
> options: farm out speakup's set selection into a work_struct thread, or
> create atomic version of vt's set_selection. These patches implement
> the former option.

Very nice, both now queued up!

thanks,

greg k-h