[PATCH v6 0/4] leds: trigger: implement a tty trigger

[PATCH v6 0/4] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Hello,

This is v6 of my quest to introduce ledtriggers for UARTs. The previous
series is available at

	http://lore.kernel.org/r/20191219093947.15502-1-u.kleine-koenig@pengutronix.de

The changes compared to that are that parsing of the dev parameter is
more strict and that I set brightness directly from the kworker instead
of using led_blink_set_oneshot which makes use of another kworker. (Both
requested by Pavel Machek.)

For the former I introduced a new helper kstrtodev_t() in the spirit of
kstrtoul() to implement the stricter parsing (instead of the lax one
using plain sscanf() in v5).

Best regards
Uwe

Uwe Kleine-König (4):
  lib: new helper kstrtodev_t()
  tty: rename tty_kopen() and add new function tty_kopen_shared()
  tty: new helper function tty_get_icount()
  leds: trigger: implement a tty trigger

 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   7 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 159 ++++++++++++++++++
 drivers/staging/speakup/spk_ttyio.c           |   2 +-
 drivers/tty/tty_io.c                          |  87 +++++++---
 include/linux/kdev_t.h                        |   2 +
 include/linux/kernel.h                        |   1 +
 include/linux/tty.h                           |   7 +-
 lib/kstrtox.c                                 |  46 +++++
 10 files changed, 293 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c


base-commit: 0bf999f9c5e74c7ecf9dafb527146601e5c848b9
-- 
2.24.0

[PATCH v6 1/4] lib: new helper kstrtodev_t()

[Uwe Kleine-König]

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

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 include/linux/kdev_t.h |  2 ++
 include/linux/kernel.h |  1 +
 lib/kstrtox.c          | 46 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/include/linux/kdev_t.h b/include/linux/kdev_t.h
index 85b5151911cf..3a5c24bd8fa4 100644
--- a/include/linux/kdev_t.h
+++ b/include/linux/kdev_t.h
@@ -4,8 +4,10 @@
 
 #include <uapi/linux/kdev_t.h>
 
+/* dev_t is 32 bit wide, 20 bits are used for MINOR, 12 for major */
 #define MINORBITS	20
 #define MINORMASK	((1U << MINORBITS) - 1)
+#define MAJORBITS	12
 
 #define MAJOR(dev)	((unsigned int) ((dev) >> MINORBITS))
 #define MINOR(dev)	((unsigned int) ((dev) & MINORMASK))
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0d9db2a14f44..9cf694c5d2c3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -410,6 +410,7 @@ int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
 int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
 int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
 int __must_check kstrtobool(const char *s, bool *res);
+int __must_check kstrtodev_t(const char *s, dev_t *res);
 
 int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
 int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..e1b896635c6a 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -19,6 +19,7 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/kdev_t.h>
 #include "kstrtox.h"
 
 const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
@@ -367,6 +368,51 @@ int kstrtobool(const char *s, bool *res)
 }
 EXPORT_SYMBOL(kstrtobool);
 
+/**
+ * kstrtodev_t - convert a string in format %u:%u to a dev_t
+ * @s: input string
+ * @res: result
+ *
+ * This is the reverse of print_dev_t. The first number is interpreted as major,
+ * the second as minor.
+ */
+int kstrtodev_t(const char *s, dev_t *res)
+{
+	unsigned long long _res;
+	unsigned int ma, mi;
+	int rv;
+
+	rv = _parse_integer(s, 10, &_res);
+	if (rv < 0)
+		return rv;
+	if (rv & KSTRTOX_OVERFLOW || _res >= (1U << MAJORBITS))
+		return -ERANGE;
+	ma = _res;
+
+	s += rv;
+
+	if (*s++ != ':')
+		return -EINVAL;
+
+	rv = _parse_integer(s, 10, &_res);
+	if (rv < 0)
+		return rv;
+	if (rv & KSTRTOX_OVERFLOW || _res >= (1U << MINORBITS))
+		return -ERANGE;
+	mi = _res;
+
+	s += rv;
+
+	if (*s == '\n')
+		s++;
+	if (*s)
+		return -EINVAL;
+
+	*res = MKDEV(ma, mi);
+	return 0;
+}
+EXPORT_SYMBOL(kstrtodev_t);
+
 /*
  * Since "base" would be a nonsense argument, this open-codes the
  * _from_user helper instead of using the helper macro below.
-- 
2.24.0

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Andy Shevchenko]

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

Can we first split the kstrotox* (and simple_strto*) to the separate
header first?

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?
What to do with all other possible variants ("%d:%d", "%dx%d" and its
%u variant, etc)?

Why simple_strto*() can't be used?

>  #include <linux/export.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>

> +#include <linux/kdev_t.h>

Perhaps preserve order? (It's for the future, since I doubt we will
get this in upstream anyway).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Uwe Kleine-König]

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).
> 
> Can we first split the kstrotox* (and simple_strto*) to the separate
> header first?

I don't feel strong here what is right. But I hesitate to create another
pre-condition for this patch set.

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

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

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

> >  #include <linux/export.h>
> >  #include <linux/types.h>
> >  #include <linux/uaccess.h>
> 
> > +#include <linux/kdev_t.h>
> 
> Perhaps preserve order?

Can do.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Andy Shevchenko]

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

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Uwe Kleine-König]

Hello Andy,

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

In my understanding one user is a hard requirement.

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

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

Also I don't understand yet, what you want me to do. 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?

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

I fail to follow your argument here. In my eyes if the user writes a
valid major:minor it should work, and if they write an invalid one this
should result in an error and not a usage of a device that just happens
to have the major:minor that simple_strtoull happens to return for the
two components.

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Andy Shevchenko]

On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> 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
> > <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.
>
> 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/.

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

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

> Also I don't understand yet, what you want me to do.

I have issues with kstrto() not playing with simple numbers (boolean
is a special case, but still a number at the end).
I also don't feel good with too narrow usage of the newly introduced helper

> 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

In b) case, add to the commit message how many potential _existing_
users may be converted to this.
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 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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Andy Shevchenko]

On Thu, Feb 20, 2020 at 1:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote:

...

> > Also I don't understand yet, what you want me to do.
>
> I have issues with kstrto() not playing with simple numbers (boolean

s/simple/simple and single/

> is a special case, but still a number at the end).
> I also don't feel good with too narrow usage of the newly introduced helper
>
> > 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:

(above doesn't imply the necessity of simple_strto*() use)

> 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
>
> In b) case, add to the commit message how many potential _existing_
> users may be converted to this.
> 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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Uwe Kleine-König]

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
> <u.kleine-koenig@pengutronix.de> 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
> > > <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.
> >
> > 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.

<sarcasm>Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good
example how to do that.</sarcasm>

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

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Andy Shevchenko]

On Thu, Feb 20, 2020 at 4:01 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> 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
> > <u.kleine-koenig@pengutronix.de> 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
> > > > <u.kleine-koenig@pengutronix.de> wrote:

...

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

Both.

> More memory usage when the file is opened in an editor?

Probably in some cases. :-)

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

If you put this like: okay, this is a new helper and new user, but we
have 100500 similar cases in the kernel which may benefit out of this,
it's one story, it you put it like: okay, let's do one helper per each
very particular case, it's another story.
You see the difference? I'm completely against the latter one.

...

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

Yes, it's also can be adjusted. You provide a better helper and then
you may do something like

static inline ... string_parse_dev_t(...)
{
  ret = string_parse_two_numbers(..., );
  if (ret)
    return ret;

  if (x or y is not in range)
    return -EOVERFLOW;
  ...
}

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

My point (when I added single) is 1:1 map. dev_t is not.

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

See above: 1/ provide a generic helper, 2/ provide its use with
certain dev_t validation.

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

Not exactly. There is a printing in human-readable format for sizes
and escape/unescape.

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

See above, I really don't see how this fits kstrtox(), to me it's
obvious layer violation.

> > In b) case, add to the commit message how many potential _existing_
> > users may be converted to this.
>
> <sarcasm>Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good
> example how to do that.</sarcasm>

I didn't get it. There are _existing_ users in the kernel for that
functionality, At least two are using it right now.
Funny that the original code has been rewritten to get rid of int_pow() call.
You may do the same, i.e. rewrite your code to get rid of whatever_dev_t() use.

> > 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 see. Thanks for report, I'll soon send a patch to fix it as well.

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

Yeah, https://xkcd.com/927/.

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

Nothing prevents you to use it from your parser which will be located
in string_*() namespace.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Uwe Kleine-König]

Hello Andy,

On Fri, Feb 21, 2020 at 10:42:29AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 20, 2020 at 4:01 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > > > 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.
> 
> My point (when I added single) is 1:1 map. dev_t is not.

I don't agree. The mapping is quite similar, the only difference is that
you cannot write 47:11 in C source code to get an instance of dev_t.
(But you can write MKDEV(47, 11) which is close but IMHO unsuitable for
the sysfs interface.)

Other than that it's just that kstrtoul does
"49283083" -> 10111100000000000000001011 while kstrtodev_t does
"47:11" -> 10111100000000000000001011.

(nitpick: it's both not 1:1 as "0x2f0000b" maps to the same as "49283083",
but ...)

> > > In b) case, add to the commit message how many potential _existing_
> > > users may be converted to this.
> >
> > <sarcasm>Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good
> > example how to do that.</sarcasm>
> 
> I didn't get it. There are _existing_ users in the kernel for that
> functionality, At least two are using it right now.

Yeah, this was just me being grumpy about "add to the commit message how
many potential _existing_ users may be converted". See the output of

	git grep '\<int_pow\>' 9f6158946987a5ce3f16da097d18f240a89db417^

.

> > [...]
> > 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.
> 
> Yeah, https://xkcd.com/927/.

With the difference that if we introduce a new standard we can
effectively kill the older ones. And that you now work towards
undeprecating simple_str* seems to confirm that we don't have the one
standard to rule them all yet.

=====

So, I'm trying to summarize the things we agree about and our
differences to maybe help finding an agreement. Trying to be objective
until the ==== below.

I think we agree about:

 - The dev_t specification provided by a user via sysfs should be parsed
   in a strict way.

 - A helper that takes a string as argument and yields a dev_t or an
   error is wanted.

The points we don't agree about yet are:

 a) naming of the function
    Uwe: It fits well into kstrto*(), so kstrtodev_t()
    Andy: It doesn't fit and feels like a layer violation

 b) Where to put the function
    Uwe: Put it into a global place for others to find
    Andy: Put it near the (for now) single user.
    (not sure this is really your position here)

 c) Helpers used to implement the str-to-dev_t helper
    Uwe: calling the already existing _parse_integer twice is fine
    Andy: let's create a helper that parses two integers with a given
          separator

====

I don't feel very strong about b), and could live with putting it near
the led trigger until a new user appears. Concerning a) I still think it
should have a name that should be obvious enough that a potential new
user finds it. And given that kstrto* already contains functions
converting strings to a given type this feels right for me. Andy
didn't suggest a definitive name, only string_* namespace. This is quite
crowded, the best representatives are probably the ones declared in
include/linux/string_helpers.h.

I looked a bit around for potential users of str-to-dev_t and
parse-two-integers. I found none for str-to-dev_t and only
dname_to_vma_addr() for the parse-two-integers helper. (But for
parse-two-integers the problem might be that I missed some as I don't
have a good idea how to grep for these.)

dname_to_vma_addr() takes a string in format "%lx-%lx", interprets the
numbers as base16 without 0x prefix and leading zeros and doesn't accept
a trailing \n. Sounds like a quest for someone being really motivated to
cover both (i.e. dname_to_vma_addr() and str-to-dev_t) in a single
versatile function.

Another way out would be to not take a dev_t from user-space to
determine the tty, but a name and use tty_dev_name_to_number() to get
the dev_t. (Which would add a second user to this globally exported
function. (The only other user is in staging.) :-)

I don't know how we can find an agreement, maybe we'd need some input
by someone else? There are quite some people who get this mail, maybe
someone read 'til here and cares enough to comment?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

Hi!

> > > > 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.
> 
> I fail to follow your argument here. In my eyes if the user writes a
> valid major:minor it should work, and if they write an invalid one this
> should result in an error and not a usage of a device that just happens
> to have the major:minor that simple_strtoull happens to return for the
> two components.

Yes please. We don't allow feet shooting where it is easy to prevent,
and that is the case here.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Introduce a new function tty_kopen_shared() that yields a struct
tty_struct. The semantic difference to tty_kopen() is that the tty is
expected to be used already. So rename tty_kopen() to
tty_kopen_exclusive() for clearness, adapt the single user and put the
common code in a new static helper function.

tty_kopen_shared is to be used to implement an LED trigger for tty
devices in one of the next patches.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/speakup/spk_ttyio.c |  2 +-
 drivers/tty/tty_io.c                | 58 ++++++++++++++++++++---------
 include/linux/tty.h                 |  5 ++-
 3 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c
index 5a9eff08cb96..e9db06eae875 100644
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -147,7 +147,7 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth)
 	if (ret)
 		return ret;
 
-	tty = tty_kopen(dev);
+	tty = tty_kopen_exclusive(dev);
 	if (IS_ERR(tty))
 		return PTR_ERR(tty);
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a1453fe10862..b718666ce73c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1875,22 +1875,7 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 	return driver;
 }
 
-/**
- *	tty_kopen	-	open a tty device for kernel
- *	@device: dev_t of device to open
- *
- *	Opens tty exclusively for kernel. Performs the driver lookup,
- *	makes sure it's not already opened and performs the first-time
- *	tty initialization.
- *
- *	Returns the locked initialized &tty_struct
- *
- *	Claims the global tty_mutex to serialize:
- *	  - concurrent first-time tty initialization
- *	  - concurrent tty driver removal w/ lookup
- *	  - concurrent tty removal from driver table
- */
-struct tty_struct *tty_kopen(dev_t device)
+static struct tty_struct *tty_kopen(dev_t device, int shared)
 {
 	struct tty_struct *tty;
 	struct tty_driver *driver;
@@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
 
 	/* check whether we're reopening an existing tty */
 	tty = tty_driver_lookup_tty(driver, NULL, index);
-	if (IS_ERR(tty))
+	if (IS_ERR(tty) || shared)
 		goto out;
 
 	if (tty) {
@@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
 	tty_driver_kref_put(driver);
 	return tty;
 }
-EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
+ *	tty_kopen_exclusive	-	open a tty device for kernel
+ *	@device: dev_t of device to open
+ *
+ *	Opens tty exclusively for kernel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Returns the locked initialized &tty_struct
+ *
+ *	Claims the global tty_mutex to serialize:
+ *	  - concurrent first-time tty initialization
+ *	  - concurrent tty driver removal w/ lookup
+ *	  - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen_exclusive(dev_t device)
+{
+	return tty_kopen(device, 0);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
+
+/**
+ *	tty_kopen_shared	-	open a tty device for shared in-kernel use
+ *	@device: dev_t of device to open
+ *
+ *	Opens an already existing tty
+ *	rnel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Locking is identical to tty_kopen() above.
+ */
+struct tty_struct *tty_kopen_shared(dev_t device)
+{
+	return tty_kopen(device, 1);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_shared);
 
 /**
  *	tty_open_by_driver	-	open a tty device
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bfa4e2ee94a9..d0bcf3226fb2 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -411,7 +411,8 @@ extern struct tty_struct *get_current_tty(void);
 /* tty_io.c */
 extern int __init tty_init(void);
 extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_kopen(dev_t device);
+extern struct tty_struct *tty_kopen_exclusive(dev_t device);
+extern struct tty_struct *tty_kopen_shared(dev_t device);
 extern void tty_kclose(struct tty_struct *tty);
 extern int tty_dev_name_to_number(const char *name, dev_t *number);
 extern int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout);
@@ -436,7 +437,7 @@ static inline int __init tty_init(void)
 { return 0; }
 static inline const char *tty_name(const struct tty_struct *tty)
 { return "(none)"; }
-static inline struct tty_struct *tty_kopen(dev_t device)
+static inline struct tty_struct *tty_kopen_exclusive(dev_t device)
 { return ERR_PTR(-ENODEV); }
 static inline void tty_kclose(struct tty_struct *tty)
 { }
-- 
2.24.0

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Johan Hovold]

On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Introduce a new function tty_kopen_shared() that yields a struct
> tty_struct. The semantic difference to tty_kopen() is that the tty is
> expected to be used already. So rename tty_kopen() to
> tty_kopen_exclusive() for clearness, adapt the single user and put the
> common code in a new static helper function.
> 
> tty_kopen_shared is to be used to implement an LED trigger for tty
> devices in one of the next patches.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
 
> -/**
> - *	tty_kopen	-	open a tty device for kernel
> - *	@device: dev_t of device to open
> - *
> - *	Opens tty exclusively for kernel. Performs the driver lookup,
> - *	makes sure it's not already opened and performs the first-time
> - *	tty initialization.
> - *
> - *	Returns the locked initialized &tty_struct
> - *
> - *	Claims the global tty_mutex to serialize:
> - *	  - concurrent first-time tty initialization
> - *	  - concurrent tty driver removal w/ lookup
> - *	  - concurrent tty removal from driver table
> - */
> -struct tty_struct *tty_kopen(dev_t device)
> +static struct tty_struct *tty_kopen(dev_t device, int shared)
>  {
>  	struct tty_struct *tty;
>  	struct tty_driver *driver;
> @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
>  
>  	/* check whether we're reopening an existing tty */
>  	tty = tty_driver_lookup_tty(driver, NULL, index);
> -	if (IS_ERR(tty))
> +	if (IS_ERR(tty) || shared)

So here you skip initialisation and return NULL if the tty isn't already
in use (e.g. is open) when shared is set.

>  		goto out;
>  
>  	if (tty) {
> @@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
>  	tty_driver_kref_put(driver);
>  	return tty;
>  }
> -EXPORT_SYMBOL_GPL(tty_kopen);
> +
> +/**
> + *	tty_kopen_exclusive	-	open a tty device for kernel
> + *	@device: dev_t of device to open
> + *
> + *	Opens tty exclusively for kernel. Performs the driver lookup,
> + *	makes sure it's not already opened and performs the first-time
> + *	tty initialization.
> + *
> + *	Returns the locked initialized &tty_struct
> + *
> + *	Claims the global tty_mutex to serialize:
> + *	  - concurrent first-time tty initialization
> + *	  - concurrent tty driver removal w/ lookup
> + *	  - concurrent tty removal from driver table
> + */
> +struct tty_struct *tty_kopen_exclusive(dev_t device)
> +{
> +	return tty_kopen(device, 0);
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
> +
> +/**
> + *	tty_kopen_shared	-	open a tty device for shared in-kernel use
> + *	@device: dev_t of device to open
> + *
> + *	Opens an already existing tty
> + *	rnel. Performs the driver lookup,

"rnel"?

> + *	makes sure it's not already opened and performs the first-time
> + *	tty initialization.

Yet, you claim to do initialisation here, which isn't the case.

> + *
> + *	Locking is identical to tty_kopen() above.
> + */
> +struct tty_struct *tty_kopen_shared(dev_t device)
> +{
> +	return tty_kopen(device, 1);
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen_shared);

This "kopen" naming is unfortunate as the tty isn't really opened by
either of these functions, but that's not something you introduced.

Johan

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Uwe Kleine-König]

On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Introduce a new function tty_kopen_shared() that yields a struct
> > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > expected to be used already. So rename tty_kopen() to
> > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > common code in a new static helper function.
> > 
> > tty_kopen_shared is to be used to implement an LED trigger for tty
> > devices in one of the next patches.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
>  
> > -/**
> > - *	tty_kopen	-	open a tty device for kernel
> > - *	@device: dev_t of device to open
> > - *
> > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > - *	makes sure it's not already opened and performs the first-time
> > - *	tty initialization.
> > - *
> > - *	Returns the locked initialized &tty_struct
> > - *
> > - *	Claims the global tty_mutex to serialize:
> > - *	  - concurrent first-time tty initialization
> > - *	  - concurrent tty driver removal w/ lookup
> > - *	  - concurrent tty removal from driver table
> > - */
> > -struct tty_struct *tty_kopen(dev_t device)
> > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> >  {
> >  	struct tty_struct *tty;
> >  	struct tty_driver *driver;
> > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> >  
> >  	/* check whether we're reopening an existing tty */
> >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > -	if (IS_ERR(tty))
> > +	if (IS_ERR(tty) || shared)
> 
> So here you skip initialisation and return NULL if the tty isn't already
> in use (e.g. is open) when shared is set.

Which is good, right? If I remember my tests correctly this even works
if the tty isn't opened but just "exists".

> >  		goto out;
> >  
> >  	if (tty) {
> > @@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
> >  	tty_driver_kref_put(driver);
> >  	return tty;
> >  }
> > -EXPORT_SYMBOL_GPL(tty_kopen);
> > +
> > +/**
> > + *	tty_kopen_exclusive	-	open a tty device for kernel
> > + *	@device: dev_t of device to open
> > + *
> > + *	Opens tty exclusively for kernel. Performs the driver lookup,
> > + *	makes sure it's not already opened and performs the first-time
> > + *	tty initialization.
> > + *
> > + *	Returns the locked initialized &tty_struct
> > + *
> > + *	Claims the global tty_mutex to serialize:
> > + *	  - concurrent first-time tty initialization
> > + *	  - concurrent tty driver removal w/ lookup
> > + *	  - concurrent tty removal from driver table
> > + */
> > +struct tty_struct *tty_kopen_exclusive(dev_t device)
> > +{
> > +	return tty_kopen(device, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
> > +
> > +/**
> > + *	tty_kopen_shared	-	open a tty device for shared in-kernel use
> > + *	@device: dev_t of device to open
> > + *
> > + *	Opens an already existing tty
> > + *	rnel. Performs the driver lookup,
> 
> "rnel"?
> 
> > + *	makes sure it's not already opened and performs the first-time
> > + *	tty initialization.
> 
> Yet, you claim to do initialisation here, which isn't the case.

Yeah, wrong (and incomplete) copy of the tty_kopen_exclusive docstring.

> > + *
> > + *	Locking is identical to tty_kopen() above.
> > + */
> > +struct tty_struct *tty_kopen_shared(dev_t device)
> > +{
> > +	return tty_kopen(device, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(tty_kopen_shared);
> 
> This "kopen" naming is unfortunate as the tty isn't really opened by
> either of these functions, but that's not something you introduced.

Ack, will send a v7 with the doc string fixed.

Thanks for taking the time to look over 
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Johan Hovold]

On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > Introduce a new function tty_kopen_shared() that yields a struct
> > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > expected to be used already. So rename tty_kopen() to
> > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > common code in a new static helper function.
> > > 
> > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > devices in one of the next patches.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> >  
> > > -/**
> > > - *	tty_kopen	-	open a tty device for kernel
> > > - *	@device: dev_t of device to open
> > > - *
> > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > - *	makes sure it's not already opened and performs the first-time
> > > - *	tty initialization.
> > > - *
> > > - *	Returns the locked initialized &tty_struct
> > > - *
> > > - *	Claims the global tty_mutex to serialize:
> > > - *	  - concurrent first-time tty initialization
> > > - *	  - concurrent tty driver removal w/ lookup
> > > - *	  - concurrent tty removal from driver table
> > > - */
> > > -struct tty_struct *tty_kopen(dev_t device)
> > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > >  {
> > >  	struct tty_struct *tty;
> > >  	struct tty_driver *driver;
> > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > >  
> > >  	/* check whether we're reopening an existing tty */
> > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > -	if (IS_ERR(tty))
> > > +	if (IS_ERR(tty) || shared)
> > 
> > So here you skip initialisation and return NULL if the tty isn't already
> > in use (e.g. is open) when shared is set.
> 
> Which is good, right? If I remember my tests correctly this even works
> if the tty isn't opened but just "exists".

No, this means that your trigger will never be installed unless the port
is already open, yet the sysfs interface still returns success (see
patch 4/4 dev_store()).

Note that the struct tty doesn't exist until the port is opened; it's
allocated in tty_init_dev() that you skip above when "shared" is set.

Johan

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Uwe Kleine-König]

On Wed, Feb 19, 2020 at 06:17:59PM +0100, Johan Hovold wrote:
> On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > 
> > > > Introduce a new function tty_kopen_shared() that yields a struct
> > > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > > expected to be used already. So rename tty_kopen() to
> > > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > > common code in a new static helper function.
> > > > 
> > > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > > devices in one of the next patches.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > >  
> > > > -/**
> > > > - *	tty_kopen	-	open a tty device for kernel
> > > > - *	@device: dev_t of device to open
> > > > - *
> > > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > > - *	makes sure it's not already opened and performs the first-time
> > > > - *	tty initialization.
> > > > - *
> > > > - *	Returns the locked initialized &tty_struct
> > > > - *
> > > > - *	Claims the global tty_mutex to serialize:
> > > > - *	  - concurrent first-time tty initialization
> > > > - *	  - concurrent tty driver removal w/ lookup
> > > > - *	  - concurrent tty removal from driver table
> > > > - */
> > > > -struct tty_struct *tty_kopen(dev_t device)
> > > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > > >  {
> > > >  	struct tty_struct *tty;
> > > >  	struct tty_driver *driver;
> > > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > > >  
> > > >  	/* check whether we're reopening an existing tty */
> > > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > > -	if (IS_ERR(tty))
> > > > +	if (IS_ERR(tty) || shared)
> > > 
> > > So here you skip initialisation and return NULL if the tty isn't already
> > > in use (e.g. is open) when shared is set.
> > 
> > Which is good, right? If I remember my tests correctly this even works
> > if the tty isn't opened but just "exists".
> 
> No, this means that your trigger will never be installed unless the port
> is already open, yet the sysfs interface still returns success (see
> patch 4/4 dev_store()).

Ah I think I see. tty_driver_lookup_tty() might return NULL which for
the trigger driver indicates that tty_kopen_shared should be retried,
right?

> Note that the struct tty doesn't exist until the port is opened; it's
> allocated in tty_init_dev() that you skip above when "shared" is set.

That needs fixing. I will try to resolve the dialog with Andy before
addressing that in the next iteration.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Johan Hovold]

On Thu, Feb 20, 2020 at 12:04:27PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 06:17:59PM +0100, Johan Hovold wrote:
> > On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > > > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > 
> > > > > Introduce a new function tty_kopen_shared() that yields a struct
> > > > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > > > expected to be used already. So rename tty_kopen() to
> > > > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > > > common code in a new static helper function.
> > > > > 
> > > > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > > > devices in one of the next patches.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > >  
> > > > > -/**
> > > > > - *	tty_kopen	-	open a tty device for kernel
> > > > > - *	@device: dev_t of device to open
> > > > > - *
> > > > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > > > - *	makes sure it's not already opened and performs the first-time
> > > > > - *	tty initialization.
> > > > > - *
> > > > > - *	Returns the locked initialized &tty_struct
> > > > > - *
> > > > > - *	Claims the global tty_mutex to serialize:
> > > > > - *	  - concurrent first-time tty initialization
> > > > > - *	  - concurrent tty driver removal w/ lookup
> > > > > - *	  - concurrent tty removal from driver table
> > > > > - */
> > > > > -struct tty_struct *tty_kopen(dev_t device)
> > > > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > > > >  {
> > > > >  	struct tty_struct *tty;
> > > > >  	struct tty_driver *driver;
> > > > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > > > >  
> > > > >  	/* check whether we're reopening an existing tty */
> > > > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > > > -	if (IS_ERR(tty))
> > > > > +	if (IS_ERR(tty) || shared)
> > > > 
> > > > So here you skip initialisation and return NULL if the tty isn't already
> > > > in use (e.g. is open) when shared is set.
> > > 
> > > Which is good, right? If I remember my tests correctly this even works
> > > if the tty isn't opened but just "exists".
> > 
> > No, this means that your trigger will never be installed unless the port
> > is already open, yet the sysfs interface still returns success (see
> > patch 4/4 dev_store()).
> 
> Ah I think I see. tty_driver_lookup_tty() might return NULL which for
> the trigger driver indicates that tty_kopen_shared should be retried,
> right?

I'm not sure how best to handle this, but yes, your trigger can only be
enabled while the port is open currently. And no error is returned to a
user trying to enable the trigger before it has been opened.

But regardless of the error reporting, I don't think failing when the
port isn't open is the right thing to do as as this makes the interface
rather useless since you cannot enable a trigger from for example a udev
rule.

If this approach is to be used at all, it seems you may need to allocate
the struct tty when the trigger is enabled. And make sure you don't mess
up some other assumption in tty core...
 
> > Note that the struct tty doesn't exist until the port is opened; it's
> > allocated in tty_init_dev() that you skip above when "shared" is set.
> 
> That needs fixing. I will try to resolve the dialog with Andy before
> addressing that in the next iteration.

Johan

Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Uwe Kleine-König]

On Tue, Feb 25, 2020 at 09:55:03AM +0100, Johan Hovold wrote:
> On Thu, Feb 20, 2020 at 12:04:27PM +0100, Uwe Kleine-König wrote:
> > Ah I think I see. tty_driver_lookup_tty() might return NULL which for
> > the trigger driver indicates that tty_kopen_shared should be retried,
> > right?
> 
> I'm not sure how best to handle this, but yes, your trigger can only be
> enabled while the port is open currently. And no error is returned to a
> user trying to enable the trigger before it has been opened.
> 
> But regardless of the error reporting, I don't think failing when the
> port isn't open is the right thing to do as as this makes the interface
> rather useless since you cannot enable a trigger from for example a udev
> rule.
> 
> If this approach is to be used at all, it seems you may need to allocate
> the struct tty when the trigger is enabled. And make sure you don't mess
> up some other assumption in tty core...

My idea is instead to retry opening the device later. At least this
sounds easier.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH v6 3/4] tty: new helper function tty_get_icount()

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

For a given struct tty_struct this yields the corresponding statistics
about sent and received characters (and some more) which is needed to
implement an LED trigger for tty devices.

The new function is then used to simplify tty_tiocgicount().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/tty_io.c | 29 +++++++++++++++++++++++++----
 include/linux/tty.h  |  2 ++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b718666ce73c..3e74e6cec1a6 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2492,15 +2492,36 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+/**
+ *	tty_get_icount		-	get tty statistics
+ *	@tty: tty device
+ *	@icount: output parameter
+ *
+ *	Gets a copy of the tty's icount statistics.
+ *
+ *	Locking: none (up to the driver)
+ */
+int tty_get_icount(struct tty_struct *tty,
+		   struct serial_icounter_struct *icount)
+{
+	memset(icount, 0, sizeof(*icount));
+
+	if (tty->ops->get_icount)
+		return tty->ops->get_icount(tty, icount);
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tty_get_icount);
+
 static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 {
-	int retval = -EINVAL;
 	struct serial_icounter_struct icount;
-	memset(&icount, 0, sizeof(icount));
-	if (tty->ops->get_icount)
-		retval = tty->ops->get_icount(tty, &icount);
+	int retval;
+
+	retval = tty_get_icount(tty, &icount);
 	if (retval != 0)
 		return retval;
+
 	if (copy_to_user(arg, &icount, sizeof(icount)))
 		return -EFAULT;
 	return 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index d0bcf3226fb2..40ea5c409619 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -495,6 +495,8 @@ extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_throttle_safe(struct tty_struct *tty);
 extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
+extern int tty_get_icount(struct tty_struct *tty,
+			  struct serial_icounter_struct *icount);
 extern int is_current_pgrp_orphaned(void);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
-- 
2.24.0

[PATCH v6 4/4] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Usage is as follows:

	myled=ledname
	tty=ttyS0

	echo tty > /sys/class/leds/$myled/trigger
	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev

. When this new trigger is active it periodically checks the tty's
statistics and when it changed since the last check the led is flashed
once.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   7 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 159 ++++++++++++++++++
 4 files changed, 173 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
new file mode 100644
index 000000000000..f56f9721c317
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -0,0 +1,6 @@
+What:		/sys/class/leds/<led>/dev
+Date:		Dec 2019
+KernelVersion:	5.6
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Specifies $major:$minor of the triggering tty
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429ca6dde..40ff08c93f56 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_TTY
+	tristate "LED Trigger for TTY devices"
+	depends on TTY
+	help
+	  This allows LEDs to be controlled by activity on ttys which includes
+	  serial devices like /dev/ttyS0.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e2a718..25c4db97cdd4 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
+obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
new file mode 100644
index 000000000000..e1be93c3026f
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <uapi/linux/serial.h>
+
+struct ledtrig_tty_data {
+	struct led_classdev *led_cdev;
+	struct delayed_work dwork;
+	struct tty_struct *tty;
+	dev_t device;
+	int rx, tx;
+};
+
+static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
+{
+	cancel_delayed_work_sync(&trigger_data->dwork);
+}
+
+static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
+{
+	schedule_delayed_work(&trigger_data->dwork, 0);
+}
+
+static ssize_t dev_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	ssize_t len = 0;
+
+	if (trigger_data->tty)
+		len = sprintf(buf, "%d:%d\n", MAJOR(trigger_data->device),
+			      MINOR(trigger_data->device));
+
+	return len;
+}
+
+static ssize_t dev_store(struct device *dev,
+			 struct device_attribute *attr, const char *buf,
+			 size_t size)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct tty_struct *tty;
+	dev_t d;
+	int ret;
+
+	if (size == 0 || (size == 1 && buf[0] == '\n')) {
+		tty = NULL;
+	} else {
+		ret = kstrtodev_t(buf, &d);
+		if (ret < 0)
+			return ret;
+
+		tty = tty_kopen_shared(d);
+		if (IS_ERR(tty))
+			return PTR_ERR(tty);
+	}
+
+	ledtrig_tty_halt(trigger_data);
+
+	tty_kref_put(trigger_data->tty);
+	trigger_data->tty = tty;
+	trigger_data->device = d;
+
+	if (tty) {
+		struct serial_icounter_struct icount;
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (!ret) {
+			trigger_data->tx = icount.tx;
+			trigger_data->rx = icount.rx;
+		}
+
+		ledtrig_tty_restart(trigger_data);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(dev);
+
+static void ledtrig_tty_work(struct work_struct *work)
+{
+	struct ledtrig_tty_data *trigger_data =
+		container_of(work, struct ledtrig_tty_data, dwork.work);
+	struct serial_icounter_struct icount;
+	int ret;
+
+	if (!trigger_data->tty) {
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+		return;
+	}
+
+	ret = tty_get_icount(trigger_data->tty, &icount);
+	if (ret)
+		return;
+
+	while (icount.rx != trigger_data->rx ||
+	       icount.tx != trigger_data->tx) {
+		led_set_brightness(trigger_data->led_cdev, LED_ON);
+
+		msleep(100);
+
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+
+		trigger_data->rx = icount.rx;
+		trigger_data->tx = icount.tx;
+
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (ret)
+			return;
+	}
+
+	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
+}
+
+static struct attribute *ledtrig_tty_attrs[] = {
+	&dev_attr_dev.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_tty);
+
+static int ledtrig_tty_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data;
+
+	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	if (!trigger_data)
+		return -ENOMEM;
+
+	led_set_trigger_data(led_cdev, trigger_data);
+
+	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
+	trigger_data->led_cdev = led_cdev;
+
+	return 0;
+}
+
+static void ledtrig_tty_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trigger_data->dwork);
+
+	kfree(trigger_data);
+}
+
+struct led_trigger ledtrig_tty = {
+	.name = "tty",
+	.activate = ledtrig_tty_activate,
+	.deactivate = ledtrig_tty_deactivate,
+	.groups = ledtrig_tty_groups,
+};
+module_led_trigger(ledtrig_tty);
+
+MODULE_AUTHOR("Uwe Kleine-König <u.kleine-koenig@pengutronix.de>");
+MODULE_DESCRIPTION("UART LED trigger");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0

Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger

[Johan Hovold]

On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Usage is as follows:
> 
> 	myled=ledname
> 	tty=ttyS0
> 
> 	echo tty > /sys/class/leds/$myled/trigger
> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> 
> . When this new trigger is active it periodically checks the tty's
> statistics and when it changed since the last check the led is flashed
> once.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

> +static ssize_t dev_store(struct device *dev,
> +			 struct device_attribute *attr, const char *buf,
> +			 size_t size)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct tty_struct *tty;
> +	dev_t d;
> +	int ret;
> +
> +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> +		tty = NULL;
> +	} else {
> +		ret = kstrtodev_t(buf, &d);
> +		if (ret < 0)
> +			return ret;
> +
> +		tty = tty_kopen_shared(d);

I really don't have time to look at this, but having the led-trigger
keep the port open looks fundamentally broken (consider modem-control
signals, power, etc).

Johan

Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:
> On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Usage is as follows:
> > 
> > 	myled=ledname
> > 	tty=ttyS0
> > 
> > 	echo tty > /sys/class/leds/$myled/trigger
> > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> > 
> > . When this new trigger is active it periodically checks the tty's
> > statistics and when it changed since the last check the led is flashed
> > once.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> 
> > +static ssize_t dev_store(struct device *dev,
> > +			 struct device_attribute *attr, const char *buf,
> > +			 size_t size)
> > +{
> > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct tty_struct *tty;
> > +	dev_t d;
> > +	int ret;
> > +
> > +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> > +		tty = NULL;
> > +	} else {
> > +		ret = kstrtodev_t(buf, &d);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		tty = tty_kopen_shared(d);
> 
> I really don't have time to look at this, but having the led-trigger
> keep the port open looks fundamentally broken (consider modem-control
> signals, power, etc).

If I understand correctly calling tty_kopen_shared() doesn't open the
device, just keep it referenced which prevents it to disappear. Unless I
miss something it doesn't result in the tty's .open() being called.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger

[Johan Hovold]

On Wed, Feb 19, 2020 at 12:03:06PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:
> > On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > Usage is as follows:
> > > 
> > > 	myled=ledname
> > > 	tty=ttyS0
> > > 
> > > 	echo tty > /sys/class/leds/$myled/trigger
> > > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> > > 
> > > . When this new trigger is active it periodically checks the tty's
> > > statistics and when it changed since the last check the led is flashed
> > > once.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > 
> > > +static ssize_t dev_store(struct device *dev,
> > > +			 struct device_attribute *attr, const char *buf,
> > > +			 size_t size)
> > > +{
> > > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > > +	struct tty_struct *tty;
> > > +	dev_t d;
> > > +	int ret;
> > > +
> > > +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> > > +		tty = NULL;
> > > +	} else {
> > > +		ret = kstrtodev_t(buf, &d);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		tty = tty_kopen_shared(d);
> > 
> > I really don't have time to look at this, but having the led-trigger
> > keep the port open looks fundamentally broken (consider modem-control
> > signals, power, etc).
> 
> If I understand correctly calling tty_kopen_shared() doesn't open the
> device, just keep it referenced which prevents it to disappear. Unless I
> miss something it doesn't result in the tty's .open() being called.

So tty_kopen_shared() is something you added. Judging from a quick look
it seems you can only attach a trigger to an already open port, but the
trigger will then keep the port open (again, consider modem control,
power).

I'm sorry I don't have time to review this myself, but this probably
needs some more eyes on it before being merged.

Johan

Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger

[Johan Hovold]

On Wed, Feb 19, 2020 at 12:19:13PM +0100, Johan Hovold wrote:
> On Wed, Feb 19, 2020 at 12:03:06PM +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:

> > If I understand correctly calling tty_kopen_shared() doesn't open the
> > device, just keep it referenced which prevents it to disappear. Unless I
> > miss something it doesn't result in the tty's .open() being called.
> 
> So tty_kopen_shared() is something you added. Judging from a quick look
> it seems you can only attach a trigger to an already open port, but the
> trigger will then keep the port open (again, consider modem control,
> power).

Sorry, my bad; this reference shouldn't prevent the port from being
closed.

The fact that you need the port to be open before you can attach a
trigger does sound like a problem though (e.g. consider udev rules).

Johan

Re: [PATCH v6 0/4] leds: trigger: implement a tty trigger

[Greg Kroah-Hartman]

On Thu, Feb 13, 2020 at 10:15:56AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Hello,
> 
> This is v6 of my quest to introduce ledtriggers for UARTs. The previous
> series is available at
> 
> 	http://lore.kernel.org/r/20191219093947.15502-1-u.kleine-koenig@pengutronix.de
> 
> The changes compared to that are that parsing of the dev parameter is
> more strict and that I set brightness directly from the kworker instead
> of using led_blink_set_oneshot which makes use of another kworker. (Both
> requested by Pavel Machek.)
> 
> For the former I introduced a new helper kstrtodev_t() in the spirit of
> kstrtoul() to implement the stricter parsing (instead of the lax one
> using plain sscanf() in v5).

Looks good to me, Pavel, any objection to me merging this through the
tty tree?

thanks,

greg k-h

Re: [PATCH v6 0/4] leds: trigger: implement a tty trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Wed 2020-02-19 11:40:12, Greg Kroah-Hartman wrote:
> On Thu, Feb 13, 2020 at 10:15:56AM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Hello,
> > 
> > This is v6 of my quest to introduce ledtriggers for UARTs. The previous
> > series is available at
> > 
> > 	http://lore.kernel.org/r/20191219093947.15502-1-u.kleine-koenig@pengutronix.de
> > 
> > The changes compared to that are that parsing of the dev parameter is
> > more strict and that I set brightness directly from the kworker instead
> > of using led_blink_set_oneshot which makes use of another kworker. (Both
> > requested by Pavel Machek.)
> > 
> > For the former I introduced a new helper kstrtodev_t() in the spirit of
> > kstrtoul() to implement the stricter parsing (instead of the lax one
> > using plain sscanf() in v5).
> 
> Looks good to me, Pavel, any objection to me merging this through the
> tty tree?

I see ton of comments on this one, seems like there will be one more
version?

And I fear merge conflicts if you modify drivers/leds/Makefile...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]