Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	Linux LED Subsystem <linux-leds@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Sascha Hauer <kernel@pengutronix.de>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>
Subject: Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
Date: Thu, 20 Feb 2020 12:22:36 +0200
Message-ID: <CAHp75VcxXWputX1y90t8f-c0a3dw2CHU6=ebQ+o6e8Z1GymiDw@mail.gmail.com> (raw)
In-Reply-To: <20200220074901.ohcrisjgd26555ya@pengutronix.de>

On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> 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 <uwe@kleine-koenig.org> 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.
If you had told "look, we have 1234 users which may benefit out of
it", I would have given no comment against.

> > 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()

> > Why simple_strto*() can't be used?
>
> I didn't really consider it, but looking in more detail I don't like it
> much. Without having tried it I think simple_strtoull accepts
> "1000000000000000000000000000000000000000000" returning some arbitrary
> value without an error indication.

So what? User has a lot of possibilities to shoot into the foot.
Since you interpret this as device major:minor, not founding a device
will be first level of error, next one when your code will try to do
something out of it. It shouldn't be a problem of kstrtox generic
helpers.

> 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.

-- 
With Best Regards,
Andy Shevchenko

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
2020-02-19 19:50   ` Andy Shevchenko
2020-02-20  7:49     ` Uwe Kleine-König
2020-02-20 10:22       ` Andy Shevchenko [this message]
2020-02-20 10:57         ` Uwe Kleine-König
2020-02-20 11:46           ` Andy Shevchenko
2020-02-20 11:57             ` Andy Shevchenko
2020-02-20 14:01             ` Uwe Kleine-König
2020-02-21  8:42               ` Andy Shevchenko
2020-02-21 10:53                 ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
2020-02-19 13:21   ` Johan Hovold
2020-02-19 16:37     ` Uwe Kleine-König
2020-02-19 17:17       ` Johan Hovold
2020-02-20 11:04         ` Uwe Kleine-König
2020-02-25  8:55           ` Johan Hovold
2020-02-25  9:05             ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 3/4] tty: new helper function tty_get_icount() Uwe Kleine-König
2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-19 10:52   ` Johan Hovold
2020-02-19 11:03     ` Uwe Kleine-König
2020-02-19 11:19       ` Johan Hovold
2020-02-19 12:48         ` Johan Hovold
2020-02-19 10:40 ` [PATCH v6 0/4] " Greg Kroah-Hartman
2020-02-26 14:02   ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHp75VcxXWputX1y90t8f-c0a3dw2CHU6=ebQ+o6e8Z1GymiDw@mail.gmail.com' \
    --to=andy.shevchenko@gmail.com \
    --cc=dmurphy@ti.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=jslaby@suse.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git