Date: Mon, 19 Jun 2017 09:09:26 +0100
From: Okash Khawaja <okash.khawaja@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Samuel Thibault <samuel.thibault@ens-lyon.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        William Hubbs <w.d.hubbs@gmail.com>,
        Chris Brannon <chris@the-brannons.com>, Kirk Reiser <kirk@reisers.ca>,
        speakup@linux-speakup.org, devel@driverdev.osuosl.org
Subject: Re: [patch v2 1/3] tty: add function to convert device name to number
Message-ID: <20170619080926.GA1374@sanghar>
References: <20170618085825.601359240@gmail.com>
 <20170618093533.134956303@gmail.com>
 <CAHp75VdrS=aSZvg2LwxwV0sR03SQb6do=aG5KGmgs1WKyvDirg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VdrS=aSZvg2LwxwV0sR03SQb6do=aG5KGmgs1WKyvDirg@mail.gmail.com>
User-Agent: Mutt/1.8.2 (2017-04-18)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2296
Lines: 69

On Sun, Jun 18, 2017 at 04:27:42PM +0300, Andy Shevchenko wrote: 
> This doesn't have actual parameter name.
> Btw, I would drop dev_ suffix completely from parameter (you have it
> in function name).
Good call, thanks.

> > + *     Locking: this acquires tty_mutex
> 
> ...and releases.
> 
> Perhaps it makes sense to describe what it protects (like it's done in
> some functions around).
Yes, will add the description

> > + */
> > +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
> 
> const char *name, right?
Yes :)

> > +       int rv, index, prefix_length = 0;
> 
> I would keep returned variable on a separate line and name it like
> other functions do in this file, i.e. ret.
> 
> int ret;
Sure

> > +       while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
> > +                       strlen(dev_name) )
> > +               prefix_length++;
> > +
> > +       if (prefix_length == strlen(dev_name))
> > +               return -EINVAL;
> 
> Basically, what you need is to get tailing digits, right?
> 
> Moreover, there is quite similar piece of code in
> tty_find_polling_driver() you may share.
tty_find_polling_driver does something slightly different. It looks for
digits embedded in a string, so something like kgdboc=ttyS0,115200 where
the digit being sought is 0 after ttyS. So the sanity checks aren't the
same. It also looks for a comma in addition to looking for a number,
which doesn't apply here. Little bit of functionality may be factored
out but that will be too trivial.

While skimming through tty_find_polling_driver, I also noticed that, in
a strict sense, the following check may not be sufficient when name is
prefix of p->name.

if (strncmp(name, p->name, len) != 0)

> > +                       if (rv) {
> 
> > +                               mutex_unlock(&tty_mutex);
> > +                               return rv;
> 
> I would go with goto style in this function (since it has locking involved).
Sure
> 
> > +                       }
> 
> All together kstrtoint() is invariant here as far as I can see and can
> be done out of locking. Also see above comment how to get line index.
Good call, thanks. Think I changed the code afterwards but didn't
notice that kstrtoint no longer needs to be inside the loop and lock.

Best regards,
Okash