From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by befuddled.reisers.ca (Postfix, from userid 65534) id 6D5901F0382; Mon, 19 Jun 2017 04:09:37 -0400 (EDT) Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) by befuddled.reisers.ca (Postfix) with ESMTPS id 7C6211F037D for ; Mon, 19 Jun 2017 04:09:31 -0400 (EDT) Received: by mail-wm0-x242.google.com with SMTP id d64so15359200wmf.2 for ; Mon, 19 Jun 2017 01:09:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=GXkhk3vUGGwVZNo2cbtqW+zx4vc/G2NFzd43fs78RXk=; b=CdoA0ZkQflJuvqLgtbBQlD/V3y+MSt+cnpkWRbNV2W7BhyXmD3ExPQMZ9PXuFbP+II s4RR773mx0r3KRLWZvnoXwCnu4u/KnAWxPZpgWQwA4cX6LGiI0Lx+kD5yihozI6p85eR GzxalSAX7Epbnvzh2cHWDGd1RKONBvEWu36u0Tq1JsR2nZhJ1lIUn6Owzf+ITPHWbvGa UILafXly1TRlWIB+4InRCNGI3AX1qKjaQ5ogaYR+d38E3T2w+aqsZ370E0PI54WHmCPD vmGI5AcIq//Ko2x9A38v1Qu3oETIHqChwYYhj9YOk0oe6hPn5DwnP6IbybYF9a1C6du1 rGTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=GXkhk3vUGGwVZNo2cbtqW+zx4vc/G2NFzd43fs78RXk=; b=arfgkbu1o+unstFO+vx95fpBUxCLdhpK33VdaxAvClewyoMj9PmC6pJO0T5RgD7tCE QcQcijHzR4gQz7II8j+Sn0X0emnsz4h6naINbjnjoRhI4KKb5n2EwbVeMpXTvkBQeF5Q xX4ML4cgHOiACCbolHZf+zK3RDUo7fN0B6cSyTwCEyBFxUYfh4LVEpqwXmdZ41zdke0g TrogO60IpwQR3a1B17N/RsgBhxtGa9+P7vb1vfNmIl5XV808qphPLuIoS44HwDgv93XG 0mBUXZDi5bSIwTwciqNzss7i/tvPHOXv80apInaUmMzGwqyAVUYZ19y5OzirPkUTfpd/ V0MQ== X-Gm-Message-State: AKS2vOwT7EthAUnzkIxY4sJJmVulaDo+yr85iPjPYzO+VTH672npoQy5 LbFZe1ucHIR0/w== X-Received: by 10.28.109.210 with SMTP id b79mr13611711wmi.55.1497859769718; Mon, 19 Jun 2017 01:09:29 -0700 (PDT) Received: from sanghar ([2a00:23c4:7320:5900:224:d6ff:fe76:7136]) by smtp.gmail.com with ESMTPSA id 5sm1122147wrq.60.2017.06.19.01.09.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 19 Jun 2017 01:09:28 -0700 (PDT) Date: Mon, 19 Jun 2017 09:09:26 +0100 From: Okash Khawaja To: Andy Shevchenko 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 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.2 (2017-04-18) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 X-BeenThere: speakup@linux-speakup.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Speakup is a screen review system for Linux." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jun 2017 08:09:39 -0000 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