Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753107AbdFRN1p (ORCPT ); Sun, 18 Jun 2017 09:27:45 -0400 Received: from mail-qt0-f194.google.com ([209.85.216.194]:36409 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752524AbdFRN1o (ORCPT ); Sun, 18 Jun 2017 09:27:44 -0400 MIME-Version: 1.0 In-Reply-To: <20170618093533.134956303@gmail.com> References: <20170618085825.601359240@gmail.com> <20170618093533.134956303@gmail.com> From: Andy Shevchenko Date: Sun, 18 Jun 2017 16:27:42 +0300 Message-ID: Subject: Re: [patch v2 1/3] tty: add function to convert device name to number To: Okash Khawaja Cc: Greg Kroah-Hartman , Jiri Slaby , Samuel Thibault , "linux-kernel@vger.kernel.org" , William Hubbs , Chris Brannon , Kirk Reiser , speakup@linux-speakup.org, devel@driverdev.osuosl.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3007 Lines: 96 On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja wrote: > The function converts strings like ttyS0 and ttyUSB0 to dev_t like > (4, 64) and (188, 0). It does this by scanning tty_drivers list for > corresponding device name and index. If the driver is not registered, > this function returns -ENODEV. It also acquires tty_mutex. Nice! Few comments below. > +/** > + * tty_dev_name_to_number - return dev_t for device name > + * @device_name: user space name of device under /dev This doesn't have actual parameter name. Btw, I would drop dev_ suffix completely from parameter (you have it in function name). > + * @dev_no: pointer to dev_t that this function will populate Here I'm not sure if dev_ prefix is good to have or not. > + * > + * This function converts device names like ttyS0 or ttyUSB1 into dev_t > + * like (4, 64) or (188, 1). If no corresponding driver is registered then > + * the function returns -ENODEV. > + * > + * Locking: this acquires tty_mutex ...and releases. Perhaps it makes sense to describe what it protects (like it's done in some functions around). > + */ > +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no) const char *name, right? > +{ > + struct tty_driver *p; > + 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; > + 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. > + mutex_lock(&tty_mutex); > + > + list_for_each_entry(p, &tty_drivers, tty_drivers) > + if (prefix_length == strlen(p->name) && strncmp(dev_name, > + p->name, prefix_length) == 0) { > + rv = kstrtoint(dev_name + prefix_length, 10, &index); > + if (rv) { > + mutex_unlock(&tty_mutex); > + return rv; I would go with goto style in this function (since it has locking involved). > + } 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. > + if (index < p->num) { > + *dev_no = MKDEV(p->major, p->minor_start + index); > + mutex_unlock(&tty_mutex); > + return 0; > + } > + } > + > + mutex_unlock(&tty_mutex); > + return -ENODEV; > +} > +EXPORT_SYMBOL_GPL(tty_dev_name_to_number); -- With Best Regards, Andy Shevchenko