staging: speakup: refactor synths array to use a list

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 <[email protected]>

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

drivers/staging/speakup/spk_types.h +2 -0

index 3e082dc3d45c..a2fc72c29894 100644
@@ -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;

drivers/staging/speakup/synth.c +13 -27

index 7deeb7061018..25f259ee4ffc 100644
@@ -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);
 }

Signature · 6 lines

2.17.1

_______________________________________________
Speakup mailing list
[email protected]
http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup

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.

Hello,

Justin Skists, le lun. 04 juin 2018 10:52:12 +0100, a ecrit:

Quoted 4 lines

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 <[email protected]>

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.

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.

Quoted 4 lines

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).

/sys/accessibility/speakup/synth, I get no speech at all from either

Quoted 3 lines · Samuel Thibault

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

Quoted 1 line

/sys/accessibility/speakup/synth, I get no speech at all from either

synthesizer.

Quoted 6 lines · On 18 June 2018 at 06:34 Gregory Nowak <[email protected]> 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

Quoted 1 line

/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 Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit:

Quoted 4 lines

Quoted 4 lines · On 18 June 2018 at 06:34 Gregory Nowak <[email protected]> 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

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

/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.

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.

[1 <text/plain; us-ascii (7bit)>]
Hello,

Samuel Thibault, le mer. 06 juin 2018 15:26:28 +0200, a ecrit:

Quoted 3 lines

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(-)

drivers/staging/speakup/spk_types.h +2 -0

index 3e082dc3d45c..a2fc72c29894 100644
@@ -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;

drivers/staging/speakup/synth.c +13 -27

index 7deeb7061018..25f259ee4ffc 100644
@@ -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);
 }

Signature · 6 lines

2.17.1
[3  <text/plain; utf-8 (base64)>]
_______________________________________________
Speakup mailing list
[email protected]
http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup