[PATCH] staging: speakup: refactor synths array to use a list

[PATCH] staging: speakup: refactor synths array to use a list

[Justin Skists]

The synths[] array is a collection of synths acting like a list.
There is no need for synths to be an array, so refactor synths[] to use
standard kernel list_head API, instead, and modify the usages to suit.
As a side-effect, the maximum number of synths has also become redundant.

Signed-off-by: Justin Skists <justin.skists@juzza.co.uk>
---
 drivers/staging/speakup/spk_types.h |  2 ++
 drivers/staging/speakup/synth.c     | 40 ++++++++++-------------------
 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 3e082dc3d45c..a2fc72c29894 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -160,6 +160,8 @@ struct spk_io_ops {
 };
 
 struct spk_synth {
+	struct list_head node;
+
 	const char *name;
 	const char *version;
 	const char *long_name;
diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
index 7deeb7061018..25f259ee4ffc 100644
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -18,8 +18,7 @@
 #include "speakup.h"
 #include "serialio.h"
 
-#define MAXSYNTHS       16      /* Max number of synths in array. */
-static struct spk_synth *synths[MAXSYNTHS + 1];
+static LIST_HEAD(synths);
 struct spk_synth *synth;
 char spk_pitch_buff[32] = "";
 static int module_status;
@@ -355,9 +354,8 @@ struct var_t synth_time_vars[] = {
 /* called by: speakup_init() */
 int synth_init(char *synth_name)
 {
-	int i;
 	int ret = 0;
-	struct spk_synth *synth = NULL;
+	struct spk_synth *tmp, *synth = NULL;
 
 	if (!synth_name)
 		return 0;
@@ -371,9 +369,10 @@ int synth_init(char *synth_name)
 
 	mutex_lock(&spk_mutex);
 	/* First, check if we already have it loaded. */
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		if (strcmp(synths[i]->name, synth_name) == 0)
-			synth = synths[i];
+	list_for_each_entry(tmp, &synths, node) {
+		if (strcmp(tmp->name, synth_name) == 0)
+			synth = tmp;
+	}
 
 	/* If we got one, initialize it now. */
 	if (synth)
@@ -448,29 +447,23 @@ void synth_release(void)
 /* called by: all_driver_init() */
 int synth_add(struct spk_synth *in_synth)
 {
-	int i;
 	int status = 0;
+	struct spk_synth *tmp;
 
 	mutex_lock(&spk_mutex);
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		/* synth_remove() is responsible for rotating the array down */
-		if (in_synth == synths[i]) {
+
+	list_for_each_entry(tmp, &synths, node) {
+		if (tmp == in_synth) {
 			mutex_unlock(&spk_mutex);
 			return 0;
 		}
-	if (i == MAXSYNTHS) {
-		pr_warn("Error: attempting to add a synth past end of array\n");
-		mutex_unlock(&spk_mutex);
-		return -1;
 	}
 
 	if (in_synth->startup)
 		status = do_synth_init(in_synth);
 
-	if (!status) {
-		synths[i++] = in_synth;
-		synths[i] = NULL;
-	}
+	if (!status)
+		list_add_tail(&in_synth->node, &synths);
 
 	mutex_unlock(&spk_mutex);
 	return status;
@@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add);
 
 void synth_remove(struct spk_synth *in_synth)
 {
-	int i;
-
 	mutex_lock(&spk_mutex);
 	if (synth == in_synth)
 		synth_release();
-	for (i = 0; synths[i]; i++) {
-		if (in_synth == synths[i])
-			break;
-	}
-	for ( ; synths[i]; i++) /* compress table */
-		synths[i] = synths[i + 1];
+	list_del(&in_synth->node);
 	module_status = 0;
 	mutex_unlock(&spk_mutex);
 }
-- 
2.17.1

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

Hello,

Justin Skists, le lun. 04 juin 2018 10:52:12 +0100, a ecrit:
> The synths[] array is a collection of synths acting like a list.
> There is no need for synths to be an array, so refactor synths[] to use
> standard kernel list_head API, instead, and modify the usages to suit.
> As a side-effect, the maximum number of synths has also become redundant.
> 
> Signed-off-by: Justin Skists <justin.skists@juzza.co.uk>

This needs a review, but the principle looks sound to me :)

Thanks,
Samuel

> ---
>  drivers/staging/speakup/spk_types.h |  2 ++
>  drivers/staging/speakup/synth.c     | 40 ++++++++++-------------------
>  2 files changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
> index 3e082dc3d45c..a2fc72c29894 100644
> --- a/drivers/staging/speakup/spk_types.h
> +++ b/drivers/staging/speakup/spk_types.h
> @@ -160,6 +160,8 @@ struct spk_io_ops {
>  };
>  
>  struct spk_synth {
> +	struct list_head node;
> +
>  	const char *name;
>  	const char *version;
>  	const char *long_name;
> diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
> index 7deeb7061018..25f259ee4ffc 100644
> --- a/drivers/staging/speakup/synth.c
> +++ b/drivers/staging/speakup/synth.c
> @@ -18,8 +18,7 @@
>  #include "speakup.h"
>  #include "serialio.h"
>  
> -#define MAXSYNTHS       16      /* Max number of synths in array. */
> -static struct spk_synth *synths[MAXSYNTHS + 1];
> +static LIST_HEAD(synths);
>  struct spk_synth *synth;
>  char spk_pitch_buff[32] = "";
>  static int module_status;
> @@ -355,9 +354,8 @@ struct var_t synth_time_vars[] = {
>  /* called by: speakup_init() */
>  int synth_init(char *synth_name)
>  {
> -	int i;
>  	int ret = 0;
> -	struct spk_synth *synth = NULL;
> +	struct spk_synth *tmp, *synth = NULL;
>  
>  	if (!synth_name)
>  		return 0;
> @@ -371,9 +369,10 @@ int synth_init(char *synth_name)
>  
>  	mutex_lock(&spk_mutex);
>  	/* First, check if we already have it loaded. */
> -	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
> -		if (strcmp(synths[i]->name, synth_name) == 0)
> -			synth = synths[i];
> +	list_for_each_entry(tmp, &synths, node) {
> +		if (strcmp(tmp->name, synth_name) == 0)
> +			synth = tmp;
> +	}
>  
>  	/* If we got one, initialize it now. */
>  	if (synth)
> @@ -448,29 +447,23 @@ void synth_release(void)
>  /* called by: all_driver_init() */
>  int synth_add(struct spk_synth *in_synth)
>  {
> -	int i;
>  	int status = 0;
> +	struct spk_synth *tmp;
>  
>  	mutex_lock(&spk_mutex);
> -	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
> -		/* synth_remove() is responsible for rotating the array down */
> -		if (in_synth == synths[i]) {
> +
> +	list_for_each_entry(tmp, &synths, node) {
> +		if (tmp == in_synth) {
>  			mutex_unlock(&spk_mutex);
>  			return 0;
>  		}
> -	if (i == MAXSYNTHS) {
> -		pr_warn("Error: attempting to add a synth past end of array\n");
> -		mutex_unlock(&spk_mutex);
> -		return -1;
>  	}
>  
>  	if (in_synth->startup)
>  		status = do_synth_init(in_synth);
>  
> -	if (!status) {
> -		synths[i++] = in_synth;
> -		synths[i] = NULL;
> -	}
> +	if (!status)
> +		list_add_tail(&in_synth->node, &synths);
>  
>  	mutex_unlock(&spk_mutex);
>  	return status;
> @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add);
>  
>  void synth_remove(struct spk_synth *in_synth)
>  {
> -	int i;
> -
>  	mutex_lock(&spk_mutex);
>  	if (synth == in_synth)
>  		synth_release();
> -	for (i = 0; synths[i]; i++) {
> -		if (in_synth == synths[i])
> -			break;
> -	}
> -	for ( ; synths[i]; i++) /* compress table */
> -		synths[i] = synths[i + 1];
> +	list_del(&in_synth->node);
>  	module_status = 0;
>  	mutex_unlock(&spk_mutex);
>  }
> -- 
> 2.17.1
> 
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup

-- 
Samuel
<N> un driver qui fait quoi, alors ?
<y> ben pour les bips
<s> pour passer les oops en morse
 -+- #ens-mim - vive les rapports de bug -+-

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

Hello,

Justin Skists, le lun. 04 juin 2018 10:52:12 +0100, a ecrit:
> The synths[] array is a collection of synths acting like a list.
> There is no need for synths to be an array, so refactor synths[] to use
> standard kernel list_head API, instead, and modify the usages to suit.
> As a side-effect, the maximum number of synths has also become redundant.

This looks good to me,

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Did you test to e.g. insmod speakup_soft ; insmod speakup_dummy ; rmmod
speakup_soft ; rmmod speakup_dummy

to make sure it did work correctly?

I'd also rather see it tested in the real wild before committing. Could
somebody on the speakup mailing list test the patch? (which I have
re-attached as a file for conveniency).

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2998 bytes --]

---
 drivers/staging/speakup/spk_types.h |  2 ++
 drivers/staging/speakup/synth.c     | 40 ++++++++++-------------------
 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 3e082dc3d45c..a2fc72c29894 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -160,6 +160,8 @@ struct spk_io_ops {
 };
 
 struct spk_synth {
+	struct list_head node;
+
 	const char *name;
 	const char *version;
 	const char *long_name;
diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
index 7deeb7061018..25f259ee4ffc 100644
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -18,8 +18,7 @@
 #include "speakup.h"
 #include "serialio.h"
 
-#define MAXSYNTHS       16      /* Max number of synths in array. */
-static struct spk_synth *synths[MAXSYNTHS + 1];
+static LIST_HEAD(synths);
 struct spk_synth *synth;
 char spk_pitch_buff[32] = "";
 static int module_status;
@@ -355,9 +354,8 @@ struct var_t synth_time_vars[] = {
 /* called by: speakup_init() */
 int synth_init(char *synth_name)
 {
-	int i;
 	int ret = 0;
-	struct spk_synth *synth = NULL;
+	struct spk_synth *tmp, *synth = NULL;
 
 	if (!synth_name)
 		return 0;
@@ -371,9 +369,10 @@ int synth_init(char *synth_name)
 
 	mutex_lock(&spk_mutex);
 	/* First, check if we already have it loaded. */
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		if (strcmp(synths[i]->name, synth_name) == 0)
-			synth = synths[i];
+	list_for_each_entry(tmp, &synths, node) {
+		if (strcmp(tmp->name, synth_name) == 0)
+			synth = tmp;
+	}
 
 	/* If we got one, initialize it now. */
 	if (synth)
@@ -448,29 +447,23 @@ void synth_release(void)
 /* called by: all_driver_init() */
 int synth_add(struct spk_synth *in_synth)
 {
-	int i;
 	int status = 0;
+	struct spk_synth *tmp;
 
 	mutex_lock(&spk_mutex);
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		/* synth_remove() is responsible for rotating the array down */
-		if (in_synth == synths[i]) {
+
+	list_for_each_entry(tmp, &synths, node) {
+		if (tmp == in_synth) {
 			mutex_unlock(&spk_mutex);
 			return 0;
 		}
-	if (i == MAXSYNTHS) {
-		pr_warn("Error: attempting to add a synth past end of array\n");
-		mutex_unlock(&spk_mutex);
-		return -1;
 	}
 
 	if (in_synth->startup)
 		status = do_synth_init(in_synth);
 
-	if (!status) {
-		synths[i++] = in_synth;
-		synths[i] = NULL;
-	}
+	if (!status)
+		list_add_tail(&in_synth->node, &synths);
 
 	mutex_unlock(&spk_mutex);
 	return status;
@@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add);
 
 void synth_remove(struct spk_synth *in_synth)
 {
-	int i;
-
 	mutex_lock(&spk_mutex);
 	if (synth == in_synth)
 		synth_release();
-	for (i = 0; synths[i]; i++) {
-		if (in_synth == synths[i])
-			break;
-	}
-	for ( ; synths[i]; i++) /* compress table */
-		synths[i] = synths[i + 1];
+	list_del(&in_synth->node);
 	module_status = 0;
 	mutex_unlock(&spk_mutex);
 }
-- 
2.17.1

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Justin Skists]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On Wed, Jun 06, 2018 at 03:26:28PM +0200, Samuel Thibault wrote:
> Hello,
> 
> Justin Skists, le lun. 04 juin 2018 10:52:12 +0100, a ecrit:
> > The synths[] array is a collection of synths acting like a list.
> > There is no need for synths to be an array, so refactor synths[] to use
> > standard kernel list_head API, instead, and modify the usages to suit.
> > As a side-effect, the maximum number of synths has also become redundant.
> 
> This looks good to me,
> 
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thank you.


> Did you test to e.g. insmod speakup_soft ; insmod speakup_dummy ; rmmod
> speakup_soft ; rmmod speakup_dummy
> 
> to make sure it did work correctly?

I did. And I swapped synths via the sysfs interface.

As always, it's always good to double-check. So, I've scripted the test
sequence that I used and attached the output.

> I'd also rather see it tested in the real wild before committing.

As it should be. :)

Justin

[-- Attachment #2: evidence.txt --]
[-- Type: text/plain, Size: 3109 bytes --]


Kernel info
-----------
# uname -a
Linux buildroot 4.17.0-rc7-next-20180601 #1 SMP Mon Jun 4 09:31:05 BST 2018 x86_64 GNU/Linux

insert modules
--------------
# modprobe speakup
# modprobe speakup_dummy dev=ttyS1 ser=1 start=1
# modprobe speakup_soft
# lsmod
Module                  Size  Used by    Tainted: G  
speakup_soft           16384  0 
speakup_dummy          16384  0 
speakup               118784  2 speakup_soft,speakup_dummy

switching to soft
-----------------
# echo 'soft' > /sys/accessibility/speakup/synth
# cat /sys/accessibility/speakup/synth
soft

switching to dummy
------------------
# echo 'dummy' > /sys/accessibility/speakup/synth
# cat /sys/accessibility/speakup/synth
dummy

Removing modules
----------------
# rmmod speakup_dummy
# rmmod speakup_soft
# lsmod
Module                  Size  Used by    Tainted: G  
speakup               118784  0 

view message log
----------------
# tail -25 /var/log/messages
Jun  6 20:06:57 buildroot kern.notice kernel: random: ssh-keygen: uninitialized urandom read (32 bytes read)
Jun  6 20:06:57 buildroot kern.notice kernel: random: sshd: uninitialized urandom read (32 bytes read)
Jun  6 20:06:57 buildroot auth.info sshd[105]: Server listening on :: port 22.
Jun  6 20:06:57 buildroot auth.info sshd[105]: Server listening on 0.0.0.0 port 22.
Jun  6 20:06:57 buildroot daemon.info : starting pid 107, tty '/dev/tty1': '/sbin/getty -L  tty1 0 vt100 '
Jun  6 20:07:00 buildroot auth.info login[107]: root login on 'tty1'
Jun  6 20:07:08 buildroot kern.notice kernel: random: crng init done
Jun  6 20:07:12 buildroot kern.warn kernel: speakup: module is from the staging directory, the quality is unknown, you have been warned.
Jun  6 20:07:13 buildroot kern.info kernel: input: Speakup as /devices/virtual/input/input4
Jun  6 20:07:13 buildroot kern.info kernel: initialized device: /dev/synth, node (MAJOR 10, MINOR 25)
Jun  6 20:07:13 buildroot kern.info kernel: speakup 3.1.6: initialized
Jun  6 20:07:13 buildroot kern.info kernel: synth name on entry is: (null)
Jun  6 20:07:13 buildroot kern.warn kernel: speakup_dummy: module is from the staging directory, the quality is unknown, you have been warned.
Jun  6 20:07:13 buildroot kern.warn kernel: synth probe
Jun  6 20:07:13 buildroot kern.warn kernel: speakup_soft: module is from the staging directory, the quality is unknown, you have been warned.
Jun  6 20:07:13 buildroot kern.info kernel: releasing synth dummy
Jun  6 20:07:13 buildroot kern.warn kernel: synth probe
Jun  6 20:07:13 buildroot kern.info kernel: initialized device: /dev/softsynth, node (MAJOR 10, MINOR 26)
Jun  6 20:07:13 buildroot kern.info kernel: initialized device: /dev/softsynthu, node (MAJOR 10, MINOR 27)
Jun  6 20:07:13 buildroot kern.warn kernel: soft already in use
Jun  6 20:07:13 buildroot kern.info kernel: releasing synth soft
Jun  6 20:07:13 buildroot kern.info kernel: unregistered /dev/softsynth
Jun  6 20:07:13 buildroot kern.info kernel: unregistered /dev/softsynthu
Jun  6 20:07:13 buildroot kern.warn kernel: synth probe
Jun  6 20:07:13 buildroot kern.info kernel: releasing synth dummy

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

Hello,

Samuel Thibault, le mer. 06 juin 2018 15:26:28 +0200, a ecrit:
> I'd also rather see it tested in the real wild before committing. Could
> somebody on the speakup mailing list test the patch? (which I have
> re-attached as a file for conveniency).

Anybody up for testing please?

If people want to see speakup get mainlined instead of staging, please
help.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2998 bytes --]

---
 drivers/staging/speakup/spk_types.h |  2 ++
 drivers/staging/speakup/synth.c     | 40 ++++++++++-------------------
 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 3e082dc3d45c..a2fc72c29894 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -160,6 +160,8 @@ struct spk_io_ops {
 };
 
 struct spk_synth {
+	struct list_head node;
+
 	const char *name;
 	const char *version;
 	const char *long_name;
diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
index 7deeb7061018..25f259ee4ffc 100644
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -18,8 +18,7 @@
 #include "speakup.h"
 #include "serialio.h"
 
-#define MAXSYNTHS       16      /* Max number of synths in array. */
-static struct spk_synth *synths[MAXSYNTHS + 1];
+static LIST_HEAD(synths);
 struct spk_synth *synth;
 char spk_pitch_buff[32] = "";
 static int module_status;
@@ -355,9 +354,8 @@ struct var_t synth_time_vars[] = {
 /* called by: speakup_init() */
 int synth_init(char *synth_name)
 {
-	int i;
 	int ret = 0;
-	struct spk_synth *synth = NULL;
+	struct spk_synth *tmp, *synth = NULL;
 
 	if (!synth_name)
 		return 0;
@@ -371,9 +369,10 @@ int synth_init(char *synth_name)
 
 	mutex_lock(&spk_mutex);
 	/* First, check if we already have it loaded. */
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		if (strcmp(synths[i]->name, synth_name) == 0)
-			synth = synths[i];
+	list_for_each_entry(tmp, &synths, node) {
+		if (strcmp(tmp->name, synth_name) == 0)
+			synth = tmp;
+	}
 
 	/* If we got one, initialize it now. */
 	if (synth)
@@ -448,29 +447,23 @@ void synth_release(void)
 /* called by: all_driver_init() */
 int synth_add(struct spk_synth *in_synth)
 {
-	int i;
 	int status = 0;
+	struct spk_synth *tmp;
 
 	mutex_lock(&spk_mutex);
-	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
-		/* synth_remove() is responsible for rotating the array down */
-		if (in_synth == synths[i]) {
+
+	list_for_each_entry(tmp, &synths, node) {
+		if (tmp == in_synth) {
 			mutex_unlock(&spk_mutex);
 			return 0;
 		}
-	if (i == MAXSYNTHS) {
-		pr_warn("Error: attempting to add a synth past end of array\n");
-		mutex_unlock(&spk_mutex);
-		return -1;
 	}
 
 	if (in_synth->startup)
 		status = do_synth_init(in_synth);
 
-	if (!status) {
-		synths[i++] = in_synth;
-		synths[i] = NULL;
-	}
+	if (!status)
+		list_add_tail(&in_synth->node, &synths);
 
 	mutex_unlock(&spk_mutex);
 	return status;
@@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add);
 
 void synth_remove(struct spk_synth *in_synth)
 {
-	int i;
-
 	mutex_lock(&spk_mutex);
 	if (synth == in_synth)
 		synth_release();
-	for (i = 0; synths[i]; i++) {
-		if (in_synth == synths[i])
-			break;
-	}
-	for ( ; synths[i]; i++) /* compress table */
-		synths[i] = synths[i + 1];
+	list_del(&in_synth->node);
 	module_status = 0;
 	mutex_unlock(&spk_mutex);
 }
-- 
2.17.1

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Gregory Nowak]

On Tue, Jun 12, 2018 at 12:57:03AM +0200, Samuel Thibault wrote:
> Anybody up for testing please?
> 
> If people want to see speakup get mainlined instead of staging, please
> help.

If I understand right, this patch changes how synthesizers are loaded
and unloaded through /sys/accessibility/speakup/synth, correct? If
yes, then would for example verifying that

echo bns >/sys/accessibility/speakup/synth
echo soft >/sys/accessibility/speakup/synth

does what it should be good enough of a test? If this would not be
good enough, please describe exactly what needs testing.

I likely won't be able to do a kernel build until this weekend, but
should be able to report back next Monday on the 18th.

Greg


-- 
web site: http://www.gregn.net
gpg public key: http://www.gregn.net/pubkey.asc
skype: gregn1
(authorization required, add me to your contacts list first)
If we haven't been in touch before, e-mail me before adding me to your contacts.

--
Free domains: http://www.eu.org/ or mail dns-manager@EU.org

Re: [PATCH] staging: speakup: refactor synths array to use a list

[John Covici]

Maybe I can do it, I have a kernel with speakup, using 4.9.43.  Will
the patch fit there?

On Mon, 11 Jun 2018 18:57:03 -0400,
Samuel Thibault wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> Hello,
> 
> Samuel Thibault, le mer. 06 juin 2018 15:26:28 +0200, a ecrit:
> > I'd also rather see it tested in the real wild before committing. Could
> > somebody on the speakup mailing list test the patch? (which I have
> > re-attached as a file for conveniency).
> 
> Anybody up for testing please?
> 
> If people want to see speakup get mainlined instead of staging, please
> help.
> 
> Samuel
> [2 patch <text/plain; us-ascii (7bit)>]
> ---
>  drivers/staging/speakup/spk_types.h |  2 ++
>  drivers/staging/speakup/synth.c     | 40 ++++++++++-------------------
>  2 files changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
> index 3e082dc3d45c..a2fc72c29894 100644
> --- a/drivers/staging/speakup/spk_types.h
> +++ b/drivers/staging/speakup/spk_types.h
> @@ -160,6 +160,8 @@ struct spk_io_ops {
>  };
>  
>  struct spk_synth {
> +	struct list_head node;
> +
>  	const char *name;
>  	const char *version;
>  	const char *long_name;
> diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
> index 7deeb7061018..25f259ee4ffc 100644
> --- a/drivers/staging/speakup/synth.c
> +++ b/drivers/staging/speakup/synth.c
> @@ -18,8 +18,7 @@
>  #include "speakup.h"
>  #include "serialio.h"
>  
> -#define MAXSYNTHS       16      /* Max number of synths in array. */
> -static struct spk_synth *synths[MAXSYNTHS + 1];
> +static LIST_HEAD(synths);
>  struct spk_synth *synth;
>  char spk_pitch_buff[32] = "";
>  static int module_status;
> @@ -355,9 +354,8 @@ struct var_t synth_time_vars[] = {
>  /* called by: speakup_init() */
>  int synth_init(char *synth_name)
>  {
> -	int i;
>  	int ret = 0;
> -	struct spk_synth *synth = NULL;
> +	struct spk_synth *tmp, *synth = NULL;
>  
>  	if (!synth_name)
>  		return 0;
> @@ -371,9 +369,10 @@ int synth_init(char *synth_name)
>  
>  	mutex_lock(&spk_mutex);
>  	/* First, check if we already have it loaded. */
> -	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
> -		if (strcmp(synths[i]->name, synth_name) == 0)
> -			synth = synths[i];
> +	list_for_each_entry(tmp, &synths, node) {
> +		if (strcmp(tmp->name, synth_name) == 0)
> +			synth = tmp;
> +	}
>  
>  	/* If we got one, initialize it now. */
>  	if (synth)
> @@ -448,29 +447,23 @@ void synth_release(void)
>  /* called by: all_driver_init() */
>  int synth_add(struct spk_synth *in_synth)
>  {
> -	int i;
>  	int status = 0;
> +	struct spk_synth *tmp;
>  
>  	mutex_lock(&spk_mutex);
> -	for (i = 0; i < MAXSYNTHS && synths[i]; i++)
> -		/* synth_remove() is responsible for rotating the array down */
> -		if (in_synth == synths[i]) {
> +
> +	list_for_each_entry(tmp, &synths, node) {
> +		if (tmp == in_synth) {
>  			mutex_unlock(&spk_mutex);
>  			return 0;
>  		}
> -	if (i == MAXSYNTHS) {
> -		pr_warn("Error: attempting to add a synth past end of array\n");
> -		mutex_unlock(&spk_mutex);
> -		return -1;
>  	}
>  
>  	if (in_synth->startup)
>  		status = do_synth_init(in_synth);
>  
> -	if (!status) {
> -		synths[i++] = in_synth;
> -		synths[i] = NULL;
> -	}
> +	if (!status)
> +		list_add_tail(&in_synth->node, &synths);
>  
>  	mutex_unlock(&spk_mutex);
>  	return status;
> @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add);
>  
>  void synth_remove(struct spk_synth *in_synth)
>  {
> -	int i;
> -
>  	mutex_lock(&spk_mutex);
>  	if (synth == in_synth)
>  		synth_release();
> -	for (i = 0; synths[i]; i++) {
> -		if (in_synth == synths[i])
> -			break;
> -	}
> -	for ( ; synths[i]; i++) /* compress table */
> -		synths[i] = synths[i + 1];
> +	list_del(&in_synth->node);
>  	module_status = 0;
>  	mutex_unlock(&spk_mutex);
>  }
> -- 
> 2.17.1
> [3  <text/plain; utf-8 (base64)>]
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup

-- 
Your life is like a penny.  You're going to lose it.  The question is:
How do
you spend it?

         John Covici wb2una
         covici@ccs.covici.com

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

Gregory Nowak, le lun. 11 juin 2018 16:51:22 -0700, a ecrit:
> On Tue, Jun 12, 2018 at 12:57:03AM +0200, Samuel Thibault wrote:
> > Anybody up for testing please?
> > 
> > If people want to see speakup get mainlined instead of staging, please
> > help.
> 
> If I understand right, this patch changes how synthesizers are loaded
> and unloaded through /sys/accessibility/speakup/synth, correct?

The load/unload is about the module itself, i.e. modprobe speakup_bns ;
modprobe speakup_soft, switch between them, then rmmod speakup_bns ;
speakup_soft or the converse (to exercise both orders).

Thanks!
Samuel

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Gregory Nowak]

On Tue, Jun 12, 2018 at 08:31:06AM +0200, Samuel Thibault wrote:
> The load/unload is about the module itself, i.e. modprobe speakup_bns ;
> modprobe speakup_soft, switch between them, then rmmod speakup_bns ;
> speakup_soft or the converse (to exercise both orders).

# uname -a
Linux p41box 4.17.1 #1 SMP Sat Jun 16 11:19:57 MST 2018 i686 GNU/Linux
# lsmod |grep "speakup"
speakup_bns            16384  0
speakup_soft           16384  1
speakup                94208  3 speakup_bns,speakup_soft

With /sys/accessibility/speakup/synth set to bns, I am getting output
alternately from the bns and from soft. It's as if speakup can't make
up its mind which synthesizer is being used. When I echo soft
>/sys/accessibility/speakup/synth, I get no speech at all from either
synthesizer. Doing rmmod of all three speakup modules comes back with
no errors. There is also no unusual output in dmesg, I can see both
synthesizers being registered and unregistered as I switch between
them.

I can also reproduce this behavior with speakup_soft, and speakup_dummy
specifically:

1. modprobe speakup_soft and modprobe speakup_dummy

2. The synthesizer should now be set to dummy in
   /sys/accessibility/speakup/synth.

3. Use the speakup review keys, press enter a number of times. You
   should observe output from both the software speech, and from the
   serial port alternating between each other.

4. echo soft >/sys/accessibility/speakup/synth

5. You should observe no output from either software speech or the
   serial port as you use speakup review keys, or press enter
   repeatedly.

6. echo dummy >/sys/accessibility/speakup/synth

7. You should alternately get speech from the software synthesizer and
   from the serial port.

I built my kernel from the 4.17.1 kernel.org sources, and the patch
that Samuel reposted applied cleanly with no errors.

Greg


-- 
web site: http://www.gregn.net
gpg public key: http://www.gregn.net/pubkey.asc
skype: gregn1
(authorization required, add me to your contacts list first)
If we haven't been in touch before, e-mail me before adding me to your contacts.

--
Free domains: http://www.eu.org/ or mail dns-manager@EU.org

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

Thanks for the tests!

Samuel

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Justin Skists]

> On 18 June 2018 at 06:34 Gregory Nowak <greg@gregn.net> wrote:
> 
> 
> On Tue, Jun 12, 2018 at 08:31:06AM +0200, Samuel Thibault wrote:
> > The load/unload is about the module itself, i.e. modprobe speakup_bns ;
> > modprobe speakup_soft, switch between them, then rmmod speakup_bns ;
> > speakup_soft or the converse (to exercise both orders).
> 
> # uname -a
> Linux p41box 4.17.1 #1 SMP Sat Jun 16 11:19:57 MST 2018 i686 GNU/Linux
> # lsmod |grep "speakup"
> speakup_bns            16384  0
> speakup_soft           16384  1
> speakup                94208  3 speakup_bns,speakup_soft
> 
> With /sys/accessibility/speakup/synth set to bns, I am getting output
> alternately from the bns and from soft. It's as if speakup can't make
> up its mind which synthesizer is being used. When I echo soft
> >/sys/accessibility/speakup/synth, I get no speech at all from either
> synthesizer.

Is this a known issue, or a regression due to the patch?


Justin.

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit:
> > On 18 June 2018 at 06:34 Gregory Nowak <greg@gregn.net> wrote:
> > With /sys/accessibility/speakup/synth set to bns, I am getting output
> > alternately from the bns and from soft. It's as if speakup can't make
> > up its mind which synthesizer is being used. When I echo soft
> > >/sys/accessibility/speakup/synth, I get no speech at all from either
> > synthesizer.
> 
> Is this a known issue, or a regression due to the patch?

I don't think it was a known issue, but I don't think it can be a
regression due to the patch, since none of that is handled by the
array/list at stake.

Samuel

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Justin Skists]

> On 18 June 2018 at 09:46 Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> 
> Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit:
> > > On 18 June 2018 at 06:34 Gregory Nowak <greg@gregn.net> wrote:
> > > With /sys/accessibility/speakup/synth set to bns, I am getting output
> > > alternately from the bns and from soft. It's as if speakup can't make
> > > up its mind which synthesizer is being used. When I echo soft
> > > >/sys/accessibility/speakup/synth, I get no speech at all from either
> > > synthesizer.
> > 
> > Is this a known issue, or a regression due to the patch?
> 
> I don't think it was a known issue, but I don't think it can be a
> regression due to the patch, since none of that is handled by the
> array/list at stake.

OK, thanks. That's what I thought, too.

When I was going through the driver code, to become familiar with it, there
were a few places that I thought needed a closer look. But I need to finish
setting up a regression testing system before I do.

Justin.

Re: [PATCH] staging: speakup: refactor synths array to use a list

[Samuel Thibault]

Justin Skists, le lun. 18 juin 2018 09:55:32 +0100, a ecrit:
> When I was going through the driver code, to become familiar with it, there
> were a few places that I thought needed a closer look.

Yes, cleanup work is probably needed in various places.

Samuel