This patch adds functionality to validate and convert either a device
name or 'ser' member of synth into dev_t. Subsequent patch in this set
will call it to convert user-specified device into device number. For
device name, this patch does some basic sanity checks on the string
passed in. It currently supports ttyS*, ttyUSB* and, for selected
synths, lp*.
The patch also introduces a string member variable named 'dev_name' to
struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
which needs conversion to dev_t.
Signed-off-by: Okash Khawaja <[email protected]>
---
drivers/staging/speakup/spk_priv.h | 2 +
drivers/staging/speakup/spk_ttyio.c | 46 ++++++++++++++++++++++++++++++++++++
drivers/staging/speakup/spk_types.h | 1
3 files changed, 49 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -40,6 +40,8 @@
#define KT_SPKUP 15
#define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
const struct old_serial_port *spk_serial_init(int index);
void spk_stop_serial_interrupt(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,10 @@
#include "spk_types.h"
#include "spk_priv.h"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -16,6 +20,48 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+ if (ser < 0 || ser > (255 - 64)) {
+ pr_err("speakup: Invalid ser param. \
+ Must be between 0 and 191 inclusive.\n");
+
+ return -EINVAL;
+ }
+
+ *dev_no = MKDEV(4, (64 + ser));
+ return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+ /* use ser only when dev is not specified */
+ if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) {
+ /* for /dev/lp* check if synth is supported */
+ if (strncmp(synth->dev_name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
+ if (strcmp(synth->name, lp_supported[i]) == 0)
+ break;
+ }
+
+ if (i >= ARRAY_SIZE(lp_supported)) {
+ pr_err("speakup: lp* is only supported on:");
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
+ pr_cont(" %s", lp_supported[i]);
+ pr_cont("\n");
+
+ return -ENOTSUPP;
+ }
+ }
+
+ return tty_dev_name_to_number(synth->dev_name, dev_no);
+ }
+
+ return ser_to_dev(synth->ser, dev_no);
+}
+
static int spk_ttyio_ldisc_open(struct tty_struct *tty)
{
struct spk_ldisc_data *ldisc_data;
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -169,6 +169,7 @@ struct spk_synth {
int jiffies;
int full;
int ser;
+ char *dev_name;
short flags;
short startup;
const int checkval; /* for validating a proper synth module */
On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <[email protected]> wrote:
> This patch adds functionality to validate and convert either a device
> name or 'ser' member of synth into dev_t. Subsequent patch in this set
> will call it to convert user-specified device into device number. For
> device name, this patch does some basic sanity checks on the string
> passed in. It currently supports ttyS*, ttyUSB* and, for selected
> synths, lp*.
>
> The patch also introduces a string member variable named 'dev_name' to
> struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
> which needs conversion to dev_t.
Looks fine. Few minor comments below, and take my
Reviewed-by: Andy Shevchenko <[email protected]>
>
> Signed-off-by: Okash Khawaja <[email protected]>
>
> ---
> drivers/staging/speakup/spk_priv.h | 2 +
> drivers/staging/speakup/spk_ttyio.c | 46 ++++++++++++++++++++++++++++++++++++
> drivers/staging/speakup/spk_types.h | 1
> 3 files changed, 49 insertions(+)
>
> --- a/drivers/staging/speakup/spk_priv.h
> +++ b/drivers/staging/speakup/spk_priv.h
> @@ -40,6 +40,8 @@
>
> #define KT_SPKUP 15
> #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
> +#define SYNTH_DEFAULT_DEV "ttyS0"
> +#define SYNTH_DEFAULT_SER 0
>
> const struct old_serial_port *spk_serial_init(int index);
> void spk_stop_serial_interrupt(void);
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -7,6 +7,10 @@
> #include "spk_types.h"
> #include "spk_priv.h"
>
> +#define DEV_PREFIX_LP "lp"
> +
> +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
static ?
> +
> struct spk_ldisc_data {
> char buf;
> struct semaphore sem;
> @@ -16,6 +20,48 @@ struct spk_ldisc_data {
> static struct spk_synth *spk_ttyio_synth;
> static struct tty_struct *speakup_tty;
>
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> + if (ser < 0 || ser > (255 - 64)) {
> + pr_err("speakup: Invalid ser param. \
> + Must be between 0 and 191 inclusive.\n");
Just make it one line.
> +
> + return -EINVAL;
> + }
> +
> + *dev_no = MKDEV(4, (64 + ser));
> + return 0;
> +}
> +
> +static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
> +{
> + /* use ser only when dev is not specified */
> + if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) {
> + /* for /dev/lp* check if synth is supported */
> + if (strncmp(synth->dev_name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
> + if (strcmp(synth->name, lp_supported[i]) == 0)
> + break;
> + }
> +
> + if (i >= ARRAY_SIZE(lp_supported)) {
match_string()
> + pr_err("speakup: lp* is only supported on:");
> + for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
> + pr_cont(" %s", lp_supported[i]);
> + pr_cont("\n");
pr_cont() is not the best idea, though I think it will be rare cases
when it might be broken in pieces.
> +
> + return -ENOTSUPP;
> + }
> + }
> +
> + return tty_dev_name_to_number(synth->dev_name, dev_no);
> + }
> +
> + return ser_to_dev(synth->ser, dev_no);
> +}
> +
> static int spk_ttyio_ldisc_open(struct tty_struct *tty)
> {
> struct spk_ldisc_data *ldisc_data;
> --- a/drivers/staging/speakup/spk_types.h
> +++ b/drivers/staging/speakup/spk_types.h
> @@ -169,6 +169,7 @@ struct spk_synth {
> int jiffies;
> int full;
> int ser;
> + char *dev_name;
const ?
> short flags;
> short startup;
> const int checkval; /* for validating a proper synth module */
>
--
With Best Regards,
Andy Shevchenko
Hi,
Thanks for the reviews. Couple of things inlined below.
On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
>
> > +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
>
> static ?
Sure!
> > + if (ser < 0 || ser > (255 - 64)) {
>
> > + pr_err("speakup: Invalid ser param. \
> > + Must be between 0 and 191 inclusive.\n");
>
> Just make it one line.
Is it okay if it becomes larger than 80 chars?
> > +
> > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
> > + if (strcmp(synth->name, lp_supported[i]) == 0)
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(lp_supported)) {
>
> match_string()
Cool, didn't know about it
>
> > + pr_err("speakup: lp* is only supported on:");
>
> > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
> > + pr_cont(" %s", lp_supported[i]);
> > + pr_cont("\n");
>
> pr_cont() is not the best idea, though I think it will be rare cases
> when it might be broken in pieces.
Hmm... I would like to keep it if it doesn't incur an overhead. It also
indicates to the reader that this all part of same output line. Let me
know what you think.
>
> > +
> > + return -ENOTSUPP;
> > + }
> > + }
> > +
> > + return tty_dev_name_to_number(synth->dev_name, dev_no);
> > + }
> > +
> > + return ser_to_dev(synth->ser, dev_no);
> > +}
> > +
> > static int spk_ttyio_ldisc_open(struct tty_struct *tty)
> > {
> > struct spk_ldisc_data *ldisc_data;
> > --- a/drivers/staging/speakup/spk_types.h
> > +++ b/drivers/staging/speakup/spk_types.h
> > @@ -169,6 +169,7 @@ struct spk_synth {
> > int jiffies;
> > int full;
> > int ser;
>
> > + char *dev_name;
>
> const ?
This becomes the target of module_param in following patch. It complains
when set to const.
Thanks!
Okash
On Sun, Jun 18, 2017 at 8:22 PM, Okash Khawaja <[email protected]> wrote:
> On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
>> > + if (ser < 0 || ser > (255 - 64)) {
>>
>> > + pr_err("speakup: Invalid ser param. \
>> > + Must be between 0 and 191 inclusive.\n");
>>
>> Just make it one line.
> Is it okay if it becomes larger than 80 chars?
Yes. Even checkpatch will not complain in this case.
>> > + pr_err("speakup: lp* is only supported on:");
>>
>> > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
>> > + pr_cont(" %s", lp_supported[i]);
>> > + pr_cont("\n");
>>
>> pr_cont() is not the best idea, though I think it will be rare cases
>> when it might be broken in pieces.
> Hmm... I would like to keep it if it doesn't incur an overhead. It also
> indicates to the reader that this all part of same output line. Let me
> know what you think.
For better user experience you need something like a temporary buffer
and one pr_err(); line at the end.
But as I said here is quite a chance that no one will see this message
broken apart.
>> > --- a/drivers/staging/speakup/spk_types.h
>> > +++ b/drivers/staging/speakup/spk_types.h
>> > @@ -169,6 +169,7 @@ struct spk_synth {
>> > int jiffies;
>> > int full;
>> > int ser;
>>
>> > + char *dev_name;
>>
>> const ?
> This becomes the target of module_param in following patch. It complains
> when set to const.
Fair enough.
--
With Best Regards,
Andy Shevchenko
On Sun, 2017-06-18 at 22:54 +0300, Andy Shevchenko wrote:
> On Sun, Jun 18, 2017 at 8:22 PM, Okash Khawaja <[email protected]> wrote:
> > On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
> > > > + if (ser < 0 || ser > (255 - 64)) {
> > > > + pr_err("speakup: Invalid ser param. \
> > > > + Must be between 0 and 191 inclusive.\n");
> > >
> > > Just make it one line.
> >
> > Is it okay if it becomes larger than 80 chars?
>
> Yes. Even checkpatch will not complain in this case.
And even if it didn't, as written it's a defect
because line continuations don't act like
string concatenations. You've added a space
before the line continuation \ and a bunch of
whitespace before the word Must
On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> This patch adds functionality to validate and convert either a device
> name or 'ser' member of synth into dev_t. Subsequent patch in this set
> will call it to convert user-specified device into device number. For
> device name, this patch does some basic sanity checks on the string
> passed in. It currently supports ttyS*, ttyUSB* and, for selected
> synths, lp*.
>
> The patch also introduces a string member variable named 'dev_name' to
> struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
> which needs conversion to dev_t.
>
> Signed-off-by: Okash Khawaja <[email protected]>
>
> ---
> drivers/staging/speakup/spk_priv.h | 2 +
> drivers/staging/speakup/spk_ttyio.c | 46 ++++++++++++++++++++++++++++++++++++
> drivers/staging/speakup/spk_types.h | 1
> 3 files changed, 49 insertions(+)
>
> --- a/drivers/staging/speakup/spk_priv.h
> +++ b/drivers/staging/speakup/spk_priv.h
> @@ -40,6 +40,8 @@
>
> #define KT_SPKUP 15
> #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
> +#define SYNTH_DEFAULT_DEV "ttyS0"
> +#define SYNTH_DEFAULT_SER 0
>
> const struct old_serial_port *spk_serial_init(int index);
> void spk_stop_serial_interrupt(void);
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -7,6 +7,10 @@
> #include "spk_types.h"
> #include "spk_priv.h"
>
> +#define DEV_PREFIX_LP "lp"
> +
> +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
> +
> struct spk_ldisc_data {
> char buf;
> struct semaphore sem;
> @@ -16,6 +20,48 @@ struct spk_ldisc_data {
> static struct spk_synth *spk_ttyio_synth;
> static struct tty_struct *speakup_tty;
>
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> + if (ser < 0 || ser > (255 - 64)) {
> + pr_err("speakup: Invalid ser param. \
> + Must be between 0 and 191 inclusive.\n");
As Andy pointed out, never do this for a C string, it's not doing what
you think it is :)
Worse case, do this like the following:
pr_err("speakup: Invalid ser param."
"Must be between 0 and 191 inclusive.\n");
Also note, you are using spaces here in the patch, always run
checkpatch.pl on your patches, so you don't get a grumpy maintainer
telling you to run checkpatch.pl on your patches :)
Please fix up and resend the series.
thanks,
greg k-h
On Mon, 2017-06-19 at 09:15 +0800, Greg Kroah-Hartman wrote:
> On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> > This patch adds functionality to validate and convert either a device
> > name or 'ser' member of synth into dev_t.
[]
> > --- a/drivers/staging/speakup/spk_ttyio.c
[]
> > +int ser_to_dev(int ser, dev_t *dev_no)
> > +{
> > + if (ser < 0 || ser > (255 - 64)) {
> > + pr_err("speakup: Invalid ser param. \
> > + Must be between 0 and 191 inclusive.\n");
>
> As Andy pointed out, never do this for a C string, it's not doing what
> you think it is :)
Well, some guy.
> Worse case, do this like the following:
> pr_err("speakup: Invalid ser param."
> "Must be between 0 and 191 inclusive.\n");
Nope, now there's no space between param and Must.
Using string concatenation on multiple lines is error prone.
On Sun, Jun 18, 2017 at 10:33:35PM -0700, Joe Perches wrote:
> On Mon, 2017-06-19 at 09:15 +0800, Greg Kroah-Hartman wrote:
> > On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> > > This patch adds functionality to validate and convert either a device
> > > name or 'ser' member of synth into dev_t.
> []
> > > --- a/drivers/staging/speakup/spk_ttyio.c
> []
> > > +int ser_to_dev(int ser, dev_t *dev_no)
> > > +{
> > > + if (ser < 0 || ser > (255 - 64)) {
> > > + pr_err("speakup: Invalid ser param. \
> > > + Must be between 0 and 191 inclusive.\n");
> >
> > As Andy pointed out, never do this for a C string, it's not doing what
> > you think it is :)
>
> Well, some guy.
>
> > Worse case, do this like the following:
> > pr_err("speakup: Invalid ser param."
> > "Must be between 0 and 191 inclusive.\n");
>
> Nope, now there's no space between param and Must.
>
> Using string concatenation on multiple lines is error prone.
Ah, yes it is, see, I messed it up! :)
On Mon, Jun 19, 2017 at 09:15:33AM +0800, Greg Kroah-Hartman wrote:
> > +int ser_to_dev(int ser, dev_t *dev_no)
> > +{
> > + if (ser < 0 || ser > (255 - 64)) {
> > + pr_err("speakup: Invalid ser param. \
> > + Must be between 0 and 191 inclusive.\n");
>
> As Andy pointed out, never do this for a C string, it's not doing what
> you think it is :)
Of course! I am sorry I should address such issues before submitting.
Will watch out more carefully next time.
>
> Worse case, do this like the following:
> pr_err("speakup: Invalid ser param."
> "Must be between 0 and 191 inclusive.\n");
>
> Also note, you are using spaces here in the patch, always run
> checkpatch.pl on your patches, so you don't get a grumpy maintainer
> telling you to run checkpatch.pl on your patches :)
Sure.
Thanks for the patience.
Okash
On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> + if (ser < 0 || ser > (255 - 64)) {
> + pr_err("speakup: Invalid ser param. \
> + Must be between 0 and 191 inclusive.\n");
I pointed out that all these strings are wrong in the first version of
the patch. You're going to end up with a whole bunch of tabs in your
dmesg output.
regards,
dan carpenter