Re: [PATCH] staging: speakup: refactor synths array to use a list
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
> On 18 June 2018 at 09:46 Samuel Thibault wrote: > > > Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit: > > > On 18 June 2018 at 06:34 Gregory Nowak 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
Justin Skists, le lun. 18 juin 2018 09:41:44 +0100, a ecrit: > > On 18 June 2018 at 06:34 Gregory Nowak 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
> On 18 June 2018 at 06:34 Gregory Nowak 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_bns16384 0 > speakup_soft 16384 1 > speakup94208 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
Thanks for the tests! Samuel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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_bns16384 0 speakup_soft 16384 1 speakup94208 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-mana...@eu.org ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 ] > 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 ] > --- > 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(_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, , 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(_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, , node) { > + if (tmp == in_synth) { > mutex_unlock(_mutex); > return 0; > } > - if (i == MAXSYNTHS) { > - pr_warn("Error: attempting to add a synth past end of array\n"); > - mutex_unlock(_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(_synth->node, ); > > mutex_unlock(_mutex); > return status; > @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add); > > void synth_remove(struct spk_synth *in_synth) > { > - int i; > - > mutex_lock(_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(_synth->node); > module_status = 0; > mutex_unlock(_mutex); > } > -- > 2.17.1 > [3 ] > ___ > Speakup mailing list > spea...@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 cov...@ccs.covici.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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-mana...@eu.org ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 --- 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(_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, , 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(_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, , node) { + if (tmp == in_synth) { mutex_unlock(_mutex); return 0; } - if (i == MAXSYNTHS) { - pr_warn("Error: attempting to add a synth past end of array\n"); - mutex_unlock(_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(_synth->node, ); mutex_unlock(_mutex); return status; @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add); void synth_remove(struct spk_synth *in_synth) { - int i; - mutex_lock(_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(_synth->node); module_status = 0; mutex_unlock(_mutex); } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 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 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 byTainted: 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 byTainted: 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 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 --- 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(_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, , 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(_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, , node) { + if (tmp == in_synth) { mutex_unlock(_mutex); return 0; } - if (i == MAXSYNTHS) { - pr_warn("Error: attempting to add a synth past end of array\n"); - mutex_unlock(_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(_synth->node, ); mutex_unlock(_mutex); return status; @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add); void synth_remove(struct spk_synth *in_synth) { - int i; - mutex_lock(_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(_synth->node); module_status = 0; mutex_unlock(_mutex); } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: refactor synths array to use a list
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 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(_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, , 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(_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, , node) { > + if (tmp == in_synth) { > mutex_unlock(_mutex); > return 0; > } > - if (i == MAXSYNTHS) { > - pr_warn("Error: attempting to add a synth past end of array\n"); > - mutex_unlock(_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(_synth->node, ); > > mutex_unlock(_mutex); > return status; > @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add); > > void synth_remove(struct spk_synth *in_synth) > { > - int i; > - > mutex_lock(_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(_synth->node); > module_status = 0; > mutex_unlock(_mutex); > } > -- > 2.17.1 > > ___ > Speakup mailing list > spea...@linux-speakup.org > http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup -- Samuel un driver qui fait quoi, alors ? ben pour les bips pour passer les oops en morse -+- #ens-mim - vive les rapports de bug -+- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] 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 --- 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(_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, , 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(_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, , node) { + if (tmp == in_synth) { mutex_unlock(_mutex); return 0; } - if (i == MAXSYNTHS) { - pr_warn("Error: attempting to add a synth past end of array\n"); - mutex_unlock(_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(_synth->node, ); mutex_unlock(_mutex); return status; @@ -479,17 +472,10 @@ EXPORT_SYMBOL_GPL(synth_add); void synth_remove(struct spk_synth *in_synth) { - int i; - mutex_lock(_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(_synth->node); module_status = 0; mutex_unlock(_mutex); } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel