Date: Wed, 14 Jun 2017 12:15:41 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: okash.khawaja@gmail.com
Cc: Jiri Slaby <jslaby@suse.com>,
        Samuel Thibault <samuel.thibault@ens-lyon.org>,
        linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
        Kirk Reiser <kirk@reisers.ca>, speakup@linux-speakup.org,
        Chris Brannon <chris@the-brannons.com>
Subject: Re: [patch 1/2] staging: speakup: add function to convert dev name
 to number
Message-ID: <20170614101541.GA8065@kroah.com>
References: <20170613223702.534373529@gmail.com>
 <20170613224050.646277124@gmail.com>
 <20170614101326.GA7606@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170614101326.GA7606@kroah.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4742
Lines: 121

On Wed, Jun 14, 2017 at 12:13:26PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 13, 2017 at 11:37:03PM +0100, okash.khawaja@gmail.com wrote:
> > The function converts strings like ttyS0 and ttyUSB0 to dev_t like
> > (4, 64) and (188, 0). Subsequent patch in this set will call it to
> > convert user-supplied device into device number. The function does
> > some basic sanity checks on the string passed in. It currently supports
> > ttyS*, ttyUSB* and, for selected synths, lp*.
> > 
> > In order to do this, the patch also introduces a string member variable
> > named 'dev' to struct spk_synth. 'dev' represents the device name -
> > ttyUSB0 etc - which needs conversion to dev_t.
> > 
> > Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> > Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > 
> > ---
> >  drivers/staging/speakup/spk_priv.h  |    2 
> >  drivers/staging/speakup/spk_ttyio.c |  105 ++++++++++++++++++++++++++++++++++++
> >  drivers/staging/speakup/spk_types.h |    1 
> >  3 files changed, 108 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,13 @@
> >  #include "spk_types.h"
> >  #include "spk_priv.h"
> >  
> > +/* Supported device types */
> > +#define DEV_PREFIX_TTYS "ttyS"
> > +#define DEV_PREFIX_TTYUSB "ttyUSB"
> > +#define DEV_PREFIX_LP "lp"
> > +
> > +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
> > +
> >  struct spk_ldisc_data {
> >  	char buf;
> >  	struct semaphore sem;
> > @@ -16,6 +23,104 @@ struct spk_ldisc_data {
> >  static struct spk_synth *spk_ttyio_synth;
> >  static struct tty_struct *speakup_tty;
> >  
> > +static int name_to_dev(const char *name, dev_t *dev_no)
> > +{
> > +        int maj = -1, min = -1;
> > +
> > +        if (strncmp(name, DEV_PREFIX_TTYS, strlen(DEV_PREFIX_TTYS)) == 0) {
> > +                if (kstrtoint(name + strlen(DEV_PREFIX_TTYS), 10, &min)) {
> > +			pr_err("speakup: Invalid ser param. Must be \
> > +					between 0 and 191 inclusive.\n");
> > +                        return -EINVAL;
> > +                }
> > +                maj = 4;
> > +
> > +                if (min < 0 || min > 191) {
> > +			pr_err("speakup: Invalid ser param. Must be \
> > +					between 0 and 191 inclusive.\n");
> > +                        return -EINVAL;
> > +                }
> > +                min = min + 64;
> > +        } else if (strncmp(name, DEV_PREFIX_TTYUSB, strlen(DEV_PREFIX_TTYUSB))
> > +			== 0) {
> > +                if (kstrtoint(name + strlen(DEV_PREFIX_TTYUSB), 10, &min)) {
> > +                        pr_err("speakup: Invalid ttyUSB number. \
> > +					Must be a number from 0 onwards\n");
> > +                        return -EINVAL;
> > +                }
> > +                maj = 188;
> > +
> > +                if (min < 0) {
> > +                        pr_err("speakup: Invalid ttyUSB number. \
> > +					Must be a number from 0 onwards\n");
> > +                        return -EINVAL;
> > +                }
> > +        } else if (strncmp(name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
> > +                if (kstrtoint(name + strlen(DEV_PREFIX_LP), 10, &min)) {
> > +                        pr_warn("speakup: Invalid lp number. \
> > +					Must be a number from 0 onwards\n");
> > +                        return -EINVAL;
> > +                }
> > +                maj = 6;
> > +
> > +                if (min < 0) {
> > +                        pr_warn("speakup: Invalid lp number. \
> > +					Must be a number from 0 onwards\n");
> > +                        return -EINVAL;
> > +                }
> > +        }
> > +
> > +        if (maj == -1 || min == -1)
> > +                return -EINVAL;
> > +
> > +        /* if here, maj and min must be valid */
> > +        *dev_no = MKDEV(maj, min);
> > +
> > +        return 0;
> > +}
> 
> Eeek, no, let's never try to parse strings like this and "figure out"
> what major/minor number it is.  That's madness and will break if we ever
> make all char majors dynamic (there's a thread on lkml about that.)
> 
> Why would the kernel need to know major/minor?  Is it going to open a
> device node?  If so, again, crazy stuff, that's not good...

Ah, no, nevermind, you just need the major/minor.  So why not just take
that as the input here, and not a string?

thanks,

greg k-h