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(-)
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
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 <[email protected]>
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
> [email protected]
> 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 -+-
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 <[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. Could
somebody on the speakup mailing list test the patch? (which I have
re-attached as a file for conveniency).
Samuel
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 <[email protected]>
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
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
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 [email protected]
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
> [email protected]
> 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
[email protected]
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
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 [email protected]
Thanks for the tests!
Samuel
> On 18 June 2018 at 06:34 Gregory Nowak <[email protected]> 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.
Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit:
> > 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
> > >/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
> On 18 June 2018 at 09:46 Samuel Thibault <[email protected]> wrote:
>
>
> Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit:
> > > 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
> > > >/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.
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