Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Andy Shevchenko]

On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@xxxxxxxxx> 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 <andy.shevchenko@xxxxxxxxx>

>
> Signed-off-by: Okash Khawaja <okash.khawaja@xxxxxxxxx>
>
> ---
>  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