From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AB867C11D05 for ; Thu, 20 Feb 2020 14:01:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 80A5020722 for ; Thu, 20 Feb 2020 14:01:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727868AbgBTOBH (ORCPT ); Thu, 20 Feb 2020 09:01:07 -0500 Received: from metis.ext.pengutronix.de ([85.220.165.71]:47407 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727298AbgBTOBG (ORCPT ); Thu, 20 Feb 2020 09:01:06 -0500 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1j4mNz-0006Ks-Ot; Thu, 20 Feb 2020 15:01:03 +0100 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1j4mNx-0003lv-HS; Thu, 20 Feb 2020 15:01:01 +0100 Date: Thu, 20 Feb 2020 15:01:01 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Andy Shevchenko Cc: Pengutronix Kernel Team , "open list:SERIAL DRIVERS" , Greg Kroah-Hartman , Linux Kernel Mailing List , Jacek Anaszewski , Pavel Machek , Jiri Slaby , Linux LED Subsystem , Dan Murphy Subject: Re: [PATCH v6 1/4] lib: new helper kstrtodev_t() Message-ID: <20200220140101.frlxklnv6x3uhzow@pengutronix.de> References: <20200213091600.554-1-uwe@kleine-koenig.org> <20200213091600.554-2-uwe@kleine-koenig.org> <20200220074901.ohcrisjgd26555ya@pengutronix.de> <20200220105718.eoevd3kb63zzrotu@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-serial@vger.kernel.org Sender: linux-serial-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-serial@vger.kernel.org Hello, On Thu, Feb 20, 2020 at 01:46:00PM +0200, Andy Shevchenko wrote (with the additions written in a later mail inserted as if he said them already then): > On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König > wrote: > > On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote: > > > On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König > > > wrote: > > > > On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote: > > > > > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König wrote: > > > > > > > > > > > > This function is in the same spirit as the other kstrto* functions and > > > > > > uses the same calling convention. It expects the input string to be in > > > > > > the format %u:%u and implements stricter parsing than sscanf as it > > > > > > returns an error on trailing data (other than the usual \n). > > > > > > ... > > > > > > > > On top of that, why kstrtodev_t is so important? How many users are > > > > > already in the kernel to get an advantage out of it? > > > > > > > > Does it need to be important? It matches the other kstrto* functions and > > > > so it seemed more natural to me to put it near the other functions. I'm > > > > not aware of other potential users and surprised you seem to suggest > > > > this as a requirement. > > > > > > Yes it does. The kstrtox() are quite generic, what you are proposing > > > is rather one particular case with blurry understanding how many users > > > will be out of it. > > > > In my understanding one user is a hard requirement. > > Yes. But looking at the LOCs you introduce to entire kernel in such > generic area (I wouldn't tell you anything if, for instance, you > introduced a support for hypothetical S2P bus with one host controller > driver) like lib/. Why are added LOCs bad? Increased compile time? Increased binary size? More memory usage when the file is opened in an editor? > > > If you had told "look, we have 1234 users which may benefit out of > > > it", I would have given no comment against. > > > > Sure, having >1000 potential users would be a good argument pro this > > function. But having only one isn't a good contra IMHO. > > For lib/ is a good argument in my opinion. So while I agree (as I wrote) that many users would be great, not having many users isn't bad enough to justify putting this function somewhere separated from the other kstrto*() functions. I assume we won't be able to sort out this difference of opinion. > > > > > What to do with all other possible variants ("%d:%d", "%dx%d" and its > > > > > %u variant, etc)? > > > > > > > > I don't see how %d:%d is relevant, major and minor cannot be negative > > > > can they? I never saw 'x' as separator between major and minor. I > > > > considered shortly parsing %u, but given that (I think) this is an > > > > internal representation only I chose to not make it more visible than it > > > > already is. > > > > > > See above, if we are going to make it generic, perhaps better to cover > > > more possible users, right? > > > Otherwise your change provokes pile of (replaced) > > > kstrto_resolution() /* %ux:%u */ > > > kstrto_range() /* %d:%d */ > > > kstrto_you_name_it() > > > > Given there are respective types that this can be stored to, I don't > > object more functions of this type and don't see a good reason to not > > add such a function. And in my eyes I prefer to have such a function in > > a visible place (i.e. where all the other kstrto* functions are) to > > prevent code duplication. > > You can easily satisfy above by adding a function parameter 'char > *delim', right? Well, not completely as major and minor have a different domain range compared to unsigned ints (or any other integer type). > > Also I don't understand yet, what you want me to do. > > I have issues with kstrto() not playing with simple and single numbers > (boolean is a special case, but still a number at the end). A dev_t is also a number in the end. > I also don't feel good with too narrow usage of the newly introduced helper I don't see how a helper that should provide a valid dev_t and other types use a generic function. The specification for parsing a dev_t is: "A non-negative number that fits in 20 bits, followed by a colon, followed by a non-negative number that fits in 12 bits." So even if we had a generic function that could parse (in scanf-semantics) "%u:%u", we'd still need a more specialized helper that ensures the range of the two integers. (And I suggest to call this helper kstrtodev_t. :-) > > Assume I'd be > > willing to use simple_strtoul, I'd still want to have a function that > > gives me a dev_t from a given string. Should I put this directly in my > > led-trigger driver? > > I see the following possibilities: > a) put it inside the caller and forget about generic helper > b) do a generic helper, but 1/ in string_*() namespace, 2/ with a > delimiter parameter and 3/ possibility to take negative numbers I wonder about 1/. Are there already other (and similar) functions in the string_* namespace? IMHO kstrto* is fine. These all take a string and provide a value of a given type with strict parsing. As pointed out above, 2/ isn't enough. I don't care much about 3/. > In b) case, add to the commit message how many potential _existing_ > users may be converted to this. Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good example how to do that. > Also it would be good to have two versions strict (only \n at the end > is allowed) and non-strict (based on the amount of users for each > group). And don't forget to extend lib/test_string.c accordingly. > > > > > And given that I was asked for strict > > > > parsing (i.e. not accepting 2:4:something) I'd say using simple_strto* > > > > is a step backwards. Also simple_strtoul() has "This function is obsolete. > > > > Please use kstrtoul instead." in its docstring which seems to apply to > > > > the other simple_strto*() functions, too. > > > > > > I specifically fixed a doc string to approve its use in the precisely > > > cases you have here. > > > > Can you please be a bit more constructive here and point to the change > > you talk about? I didn't find a commit in next. > > https://elixir.bootlin.com/linux/v5.6-rc2/source/include/linux/kernel.h#L446 > > Note, there is no more word 'obsolete' there. I talked about https://elixir.bootlin.com/linux/v5.6-rc2/source/lib/vsprintf.c#L61 which still tells to not use it. I think what is needed here to satisfy us both is a set of functions like: int buftoul(const char *buf, char **endp, unsigned long *result) which does proper range checking (by not consuming chars that are too much) and still provides an endp pointer that allows to make use of this function to parse types that are represented by more than a plain integer. Currently this functionality is provided by _parse_integer (with a different API and slightly different semantic). For my purpose _parse_integer is good enough, so I'd like to leave introduction and identification plus conversion of already existing potential users to you. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |