[PATCH v3 0/3] led_trigger_register_format and tty triggers

[PATCH v3 0/3] led_trigger_register_format and tty triggers

[Uwe Kleine-König]

Hello,

while working on a patch that adds led triggers to drivers/tty (patch 3)
I thought that being able to pass a format string (and the respective
parameters) to led_trigger_register_simple instead of a constant string
would be nice. This is implemented in the first patch. The second patch
converts the can leds to this new function which demonstrates nicely the
added benefit for users. Both patches are new in v3.

The third patch finally implements the triggers for the tty framework.
Compared to v2 I reduced the need for #ifdefs, make use of
led_trigger_register_format() and excluded serdev devices from
triggering as suggested by Johan Hovold.

Also code cleanup in the error case is done now and hopefully the kbuild
test robot is happy now.

Best regards
Uwe

Uwe Kleine-König (3):
  leds: triggers: provide led_trigger_register_format()
  can: simplify LED trigger handling
  tty: implement led triggers

 arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
 drivers/leds/led-triggers.c               |  84 +++++++---
 drivers/net/can/led.c                     |  30 +---
 drivers/tty/Kconfig                       |   7 +
 drivers/tty/tty_buffer.c                  |   2 +
 drivers/tty/tty_io.c                      |   3 +
 drivers/tty/tty_port.c                    |  32 +++-
 include/linux/can/dev.h                   |   3 -
 include/linux/leds.h                      |  30 ++--
 include/linux/tty.h                       |  22 +++
 10 files changed, 341 insertions(+), 64 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

-- 
2.17.0

[PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Uwe Kleine-König]

This allows one to simplify drivers that provide a trigger with a
non-constant name (e.g. one trigger per device with the trigger name
depending on the device's name).

Internally the memory the name member of struct led_trigger points to
now always allocated dynamically instead of just taken from the caller.

The function led_trigger_rename_static() must be changed accordingly and
was renamed to led_trigger_rename() for consistency, with the only user
adapted.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/led-triggers.c | 84 +++++++++++++++++++++++++++----------
 drivers/net/can/led.c       |  6 +--
 include/linux/leds.h        | 30 +++++++------
 3 files changed, 81 insertions(+), 39 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b048a2..5d8bb504b07b 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -175,18 +175,34 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
-void led_trigger_rename_static(const char *name, struct led_trigger *trig)
+int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
 {
-	/* new name must be on a temporary string to prevent races */
-	BUG_ON(name == trig->name);
+	const char *prevname;
+	const char *newname;
+	va_list args;
+
+	if (!trig)
+		return 0;
+
+	va_start(args, fmt);
+	newname = kvasprintf_const(GFP_KERNEL, fmt, args);
+	va_end(args);
+
+	if (!newname) {
+		pr_err("Failed to allocate new name for trigger %s\n", trig->name);
+		return -ENOMEM;
+	}
 
 	down_write(&triggers_list_lock);
-	/* this assumes that trig->name was originaly allocated to
-	 * non constant storage */
-	strcpy((char *)trig->name, name);
+	prevname = trig->name;
+	trig->name = newname;
 	up_write(&triggers_list_lock);
+
+	kfree_const(prevname);
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(led_trigger_rename_static);
+EXPORT_SYMBOL_GPL(led_trigger_rename);
 
 /* LED Trigger Interface */
 
@@ -333,34 +349,56 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
 }
 EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
 
-void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+int led_trigger_register_format(struct led_trigger **tp, const char *fmt, ...)
 {
+	va_list args;
 	struct led_trigger *trig;
-	int err;
+	int err = -ENOMEM;
+	const char *name;
+
+	va_start(args, fmt);
+	name = kvasprintf_const(GFP_KERNEL, fmt, args);
+	va_end(args);
 
 	trig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
 
-	if (trig) {
-		trig->name = name;
-		err = led_trigger_register(trig);
-		if (err < 0) {
-			kfree(trig);
-			trig = NULL;
-			pr_warn("LED trigger %s failed to register (%d)\n",
-				name, err);
-		}
-	} else {
-		pr_warn("LED trigger %s failed to register (no memory)\n",
-			name);
-	}
+	if (!name || !trig)
+		goto err;
+
+	trig->name = name;
+
+	err = led_trigger_register(trig);
+	if (err < 0)
+		goto err;
+
 	*tp = trig;
+
+	return 0;
+
+err:
+	kfree(trig);
+	kfree_const(name);
+
+	*tp = NULL;
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(led_trigger_register_format);
+
+void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+{
+	int ret = led_trigger_register_format(tp, "%s", name);
+	if (ret < 0)
+		pr_warn("LED trigger %s failed to register (%d)\n", name, ret);
 }
 EXPORT_SYMBOL_GPL(led_trigger_register_simple);
 
 void led_trigger_unregister_simple(struct led_trigger *trig)
 {
-	if (trig)
+	if (trig) {
 		led_trigger_unregister(trig);
+		kfree_const(trig->name);
+	}
 	kfree(trig);
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index c1b667675fa1..2d7d1b0d20f9 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
 
 	if (msg == NETDEV_CHANGENAME) {
 		snprintf(name, sizeof(name), "%s-tx", netdev->name);
-		led_trigger_rename_static(name, priv->tx_led_trig);
+		led_trigger_rename(priv->tx_led_trig, name);
 
 		snprintf(name, sizeof(name), "%s-rx", netdev->name);
-		led_trigger_rename_static(name, priv->rx_led_trig);
+		led_trigger_rename(priv->rx_led_trig, name);
 
 		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
-		led_trigger_rename_static(name, priv->rxtx_led_trig);
+		led_trigger_rename(priv->rxtx_led_trig, name);
 	}
 
 	return NOTIFY_DONE;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b7e82550e655..e706c28bb35b 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -275,6 +275,8 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
 extern int devm_led_trigger_register(struct device *dev,
 				     struct led_trigger *trigger);
 
+extern int led_trigger_register_format(struct led_trigger **trigger,
+				       const char *fmt, ...);
 extern void led_trigger_register_simple(const char *name,
 				struct led_trigger **trigger);
 extern void led_trigger_unregister_simple(struct led_trigger *trigger);
@@ -298,28 +300,25 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 }
 
 /**
- * led_trigger_rename_static - rename a trigger
- * @name: the new trigger name
+ * led_trigger_rename - rename a trigger
  * @trig: the LED trigger to rename
+ * @fmt: format string for new name
  *
- * Change a LED trigger name by copying the string passed in
- * name into current trigger name, which MUST be large
- * enough for the new string.
- *
- * Note that name must NOT point to the same string used
- * during LED registration, as that could lead to races.
- *
- * This is meant to be used on triggers with statically
- * allocated name.
+ * rebaptize the given trigger.
  */
-extern void led_trigger_rename_static(const char *name,
-				      struct led_trigger *trig);
+extern int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...);
 
 #else
 
 /* Trigger has no members */
 struct led_trigger {};
 
+static inline int led_trigger_register_format(struct led_trigger **trigger,
+					      const char *fmt, ...)
+{
+	return 0;
+}
+
 /* Trigger inline empty functions */
 static inline void led_trigger_register_simple(const char *name,
 					struct led_trigger **trigger) {}
@@ -342,6 +341,11 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return NULL;
 }
 
+static inline int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
+{
+	return 0;
+}
+
 #endif /* CONFIG_LEDS_TRIGGERS */
 
 /* Trigger specific functions */
-- 
2.17.0

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Jacek Anaszewski]

Hi Uwe,

Thank you for the patch. It looks fine, but please split
the drivers/net/can/led.c related changes into a separate one.

Best regards,
Jacek Anaszewski

On 05/08/2018 12:05 PM, Uwe Kleine-König wrote:
> This allows one to simplify drivers that provide a trigger with a
> non-constant name (e.g. one trigger per device with the trigger name
> depending on the device's name).
> 
> Internally the memory the name member of struct led_trigger points to
> now always allocated dynamically instead of just taken from the caller.
> 
> The function led_trigger_rename_static() must be changed accordingly and
> was renamed to led_trigger_rename() for consistency, with the only user
> adapted.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/leds/led-triggers.c | 84 +++++++++++++++++++++++++++----------
>   drivers/net/can/led.c       |  6 +--
>   include/linux/leds.h        | 30 +++++++------
>   3 files changed, 81 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 431123b048a2..5d8bb504b07b 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -175,18 +175,34 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
>   }
>   EXPORT_SYMBOL_GPL(led_trigger_set_default);
>   
> -void led_trigger_rename_static(const char *name, struct led_trigger *trig)
> +int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
>   {
> -	/* new name must be on a temporary string to prevent races */
> -	BUG_ON(name == trig->name);
> +	const char *prevname;
> +	const char *newname;
> +	va_list args;
> +
> +	if (!trig)
> +		return 0;
> +
> +	va_start(args, fmt);
> +	newname = kvasprintf_const(GFP_KERNEL, fmt, args);
> +	va_end(args);
> +
> +	if (!newname) {
> +		pr_err("Failed to allocate new name for trigger %s\n", trig->name);
> +		return -ENOMEM;
> +	}
>   
>   	down_write(&triggers_list_lock);
> -	/* this assumes that trig->name was originaly allocated to
> -	 * non constant storage */
> -	strcpy((char *)trig->name, name);
> +	prevname = trig->name;
> +	trig->name = newname;
>   	up_write(&triggers_list_lock);
> +
> +	kfree_const(prevname);
> +
> +	return 0;
>   }
> -EXPORT_SYMBOL_GPL(led_trigger_rename_static);
> +EXPORT_SYMBOL_GPL(led_trigger_rename);
>   
>   /* LED Trigger Interface */
>   
> @@ -333,34 +349,56 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
>   }
>   EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
>   
> -void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +int led_trigger_register_format(struct led_trigger **tp, const char *fmt, ...)
>   {
> +	va_list args;
>   	struct led_trigger *trig;
> -	int err;
> +	int err = -ENOMEM;
> +	const char *name;
> +
> +	va_start(args, fmt);
> +	name = kvasprintf_const(GFP_KERNEL, fmt, args);
> +	va_end(args);
>   
>   	trig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
>   
> -	if (trig) {
> -		trig->name = name;
> -		err = led_trigger_register(trig);
> -		if (err < 0) {
> -			kfree(trig);
> -			trig = NULL;
> -			pr_warn("LED trigger %s failed to register (%d)\n",
> -				name, err);
> -		}
> -	} else {
> -		pr_warn("LED trigger %s failed to register (no memory)\n",
> -			name);
> -	}
> +	if (!name || !trig)
> +		goto err;
> +
> +	trig->name = name;
> +
> +	err = led_trigger_register(trig);
> +	if (err < 0)
> +		goto err;
> +
>   	*tp = trig;
> +
> +	return 0;
> +
> +err:
> +	kfree(trig);
> +	kfree_const(name);
> +
> +	*tp = NULL;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_register_format);
> +
> +void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +{
> +	int ret = led_trigger_register_format(tp, "%s", name);
> +	if (ret < 0)
> +		pr_warn("LED trigger %s failed to register (%d)\n", name, ret);
>   }
>   EXPORT_SYMBOL_GPL(led_trigger_register_simple);
>   
>   void led_trigger_unregister_simple(struct led_trigger *trig)
>   {
> -	if (trig)
> +	if (trig) {
>   		led_trigger_unregister(trig);
> +		kfree_const(trig->name);
> +	}
>   	kfree(trig);
>   }
>   EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> index c1b667675fa1..2d7d1b0d20f9 100644
> --- a/drivers/net/can/led.c
> +++ b/drivers/net/can/led.c
> @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
>   
>   	if (msg == NETDEV_CHANGENAME) {
>   		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> -		led_trigger_rename_static(name, priv->tx_led_trig);
> +		led_trigger_rename(priv->tx_led_trig, name);
>   
>   		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> -		led_trigger_rename_static(name, priv->rx_led_trig);
> +		led_trigger_rename(priv->rx_led_trig, name);
>   
>   		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> -		led_trigger_rename_static(name, priv->rxtx_led_trig);
> +		led_trigger_rename(priv->rxtx_led_trig, name);
>   	}
>   
>   	return NOTIFY_DONE;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b7e82550e655..e706c28bb35b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -275,6 +275,8 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
>   extern int devm_led_trigger_register(struct device *dev,
>   				     struct led_trigger *trigger);
>   
> +extern int led_trigger_register_format(struct led_trigger **trigger,
> +				       const char *fmt, ...);
>   extern void led_trigger_register_simple(const char *name,
>   				struct led_trigger **trigger);
>   extern void led_trigger_unregister_simple(struct led_trigger *trigger);
> @@ -298,28 +300,25 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
>   }
>   
>   /**
> - * led_trigger_rename_static - rename a trigger
> - * @name: the new trigger name
> + * led_trigger_rename - rename a trigger
>    * @trig: the LED trigger to rename
> + * @fmt: format string for new name
>    *
> - * Change a LED trigger name by copying the string passed in
> - * name into current trigger name, which MUST be large
> - * enough for the new string.
> - *
> - * Note that name must NOT point to the same string used
> - * during LED registration, as that could lead to races.
> - *
> - * This is meant to be used on triggers with statically
> - * allocated name.
> + * rebaptize the given trigger.
>    */
> -extern void led_trigger_rename_static(const char *name,
> -				      struct led_trigger *trig);
> +extern int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...);
>   
>   #else
>   
>   /* Trigger has no members */
>   struct led_trigger {};
>   
> +static inline int led_trigger_register_format(struct led_trigger **trigger,
> +					      const char *fmt, ...)
> +{
> +	return 0;
> +}
> +
>   /* Trigger inline empty functions */
>   static inline void led_trigger_register_simple(const char *name,
>   					struct led_trigger **trigger) {}
> @@ -342,6 +341,11 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
>   	return NULL;
>   }
>   
> +static inline int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
> +{
> +	return 0;
> +}
> +
>   #endif /* CONFIG_LEDS_TRIGGERS */
>   
>   /* Trigger specific functions */
> 

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Uwe Kleine-König]

Hello Jacek,

On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
> Thank you for the patch. It looks fine, but please split
> the drivers/net/can/led.c related changes into a separate one.

I renamed led_trigger_rename_static() to led_trigger_rename() (and
changed the parameters). The can change just adapts the only user of
led_trigger_rename_static() to use the new one.

It's not impossible to separate this patches, but I wonder if it's worth
the effort.

The first patch would be like the patch under discussion, just without
the can bits and introducing something like:

	/*
	 * compat stuff to be removed once the only caller is converted
	 */
	static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
	{
		(void)led_trigger_rename(trig, "%s", name);
	}

Then the second patch would just be the 6-line can hunk. And a third
patch would remove the compat function. (Maybe I'd choose to squash the
two can patches together then, but this doesn't reduce the overhead
considerably.) The only upside I can see here is that it increases my
patch count, but it's otherwise not worth the effort for such an easy
change. Further more as there is a strict dependency on these three
patches this either delays the cleanup or (IMHO more likely) the can
change would go in via the led tree anyhow.  (Mark already acked patch 2
of this series and in private confirmed that the agrees to let this
change go in via the led tree, too.)

Best regards
Uwe

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

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Jacek Anaszewski]

Hi Uwe,

On 05/08/2018 10:17 PM, Uwe Kleine-König wrote:
> Hello Jacek,
> 
> On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
>> Thank you for the patch. It looks fine, but please split
>> the drivers/net/can/led.c related changes into a separate one.
> 
> I renamed led_trigger_rename_static() to led_trigger_rename() (and
> changed the parameters). The can change just adapts the only user of
> led_trigger_rename_static() to use the new one.
> 
> It's not impossible to separate this patches, but I wonder if it's worth
> the effort.
> 
> The first patch would be like the patch under discussion, just without
> the can bits and introducing something like:
> 
> 	/*
> 	 * compat stuff to be removed once the only caller is converted
> 	 */
> 	static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
> 	{
> 		(void)led_trigger_rename(trig, "%s", name);
> 	}
> 
> Then the second patch would just be the 6-line can hunk. And a third
> patch would remove the compat function. (Maybe I'd choose to squash the
> two can patches together then, but this doesn't reduce the overhead
> considerably.) The only upside I can see here is that it increases my
> patch count, but it's otherwise not worth the effort for such an easy
> change. Further more as there is a strict dependency on these three
> patches this either delays the cleanup or (IMHO more likely) the can
> change would go in via the led tree anyhow.  (Mark already acked patch 2
> of this series and in private confirmed that the agrees to let this
> change go in via the led tree, too.)

OK, makes sense. I'll wait also for ack on 3/3 since it should
go through the LED tree as well - uses a new led_trigger_register_format().

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Pavel Machek]

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

Hi!

> This allows one to simplify drivers that provide a trigger with a
> non-constant name (e.g. one trigger per device with the trigger name
> depending on the device's name).
> 
> Internally the memory the name member of struct led_trigger points to
> now always allocated dynamically instead of just taken from the caller.
> 
> The function led_trigger_rename_static() must be changed accordingly and
> was renamed to led_trigger_rename() for consistency, with the only user
> adapted.

Well, I'm not sure if we want to have _that_ many trigger. Trigger
interface is going to become.. "interesting".

We have 4K limit on total number of triggers. We use rather strange
interface to select trigger.

> @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
>  
>  	if (msg == NETDEV_CHANGENAME) {
>  		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> -		led_trigger_rename_static(name, priv->tx_led_trig);
> +		led_trigger_rename(priv->tx_led_trig, name);
>  
>  		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> -		led_trigger_rename_static(name, priv->rx_led_trig);
> +		led_trigger_rename(priv->rx_led_trig, name);
>  
>  		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> -		led_trigger_rename_static(name, priv->rxtx_led_trig);
> +		led_trigger_rename(priv->rxtx_led_trig, name);
>  	}
>  

I know this is not your fault, but if you have a space or "[]" in
netdev names, confusing things will happen.

I believe we should have triggers "net-rx, net-tx" and it should have
parameter "which device it acts on". 
									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 --]

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Pavel Machek]

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

On Thu 2018-05-10 13:21:01, Pavel Machek wrote:
> Hi!
> 
> > This allows one to simplify drivers that provide a trigger with a
> > non-constant name (e.g. one trigger per device with the trigger name
> > depending on the device's name).
> > 
> > Internally the memory the name member of struct led_trigger points to
> > now always allocated dynamically instead of just taken from the caller.
> > 
> > The function led_trigger_rename_static() must be changed accordingly and
> > was renamed to led_trigger_rename() for consistency, with the only user
> > adapted.
> 
> Well, I'm not sure if we want to have _that_ many trigger. Trigger
> interface is going to become.. "interesting".
> 
> We have 4K limit on total number of triggers. We use rather strange
> interface to select trigger.
> 
> > @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> >  
> >  	if (msg == NETDEV_CHANGENAME) {
> >  		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> > -		led_trigger_rename_static(name, priv->tx_led_trig);
> > +		led_trigger_rename(priv->tx_led_trig, name);
> >  
> >  		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> > -		led_trigger_rename_static(name, priv->rx_led_trig);
> > +		led_trigger_rename(priv->rx_led_trig, name);
> >  
> >  		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> > -		led_trigger_rename_static(name, priv->rxtx_led_trig);
> > +		led_trigger_rename(priv->rxtx_led_trig, name);
> >  	}
> >  
> 
> I know this is not your fault, but if you have a space or "[]" in
> netdev names, confusing things will happen.

Hmm. If we are doing this we really should check trigger names for
forbidden characters. At least "[] " should be forbidden.
								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 --]

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Uwe Kleine-König]

On Thu, May 10, 2018 at 01:22:29PM +0200, Pavel Machek wrote:
> On Thu 2018-05-10 13:21:01, Pavel Machek wrote:
> > Hi!
> > 
> > > This allows one to simplify drivers that provide a trigger with a
> > > non-constant name (e.g. one trigger per device with the trigger name
> > > depending on the device's name).
> > > 
> > > Internally the memory the name member of struct led_trigger points to
> > > now always allocated dynamically instead of just taken from the caller.
> > > 
> > > The function led_trigger_rename_static() must be changed accordingly and
> > > was renamed to led_trigger_rename() for consistency, with the only user
> > > adapted.
> > 
> > Well, I'm not sure if we want to have _that_ many trigger. Trigger
> > interface is going to become.. "interesting".
> > 
> > We have 4K limit on total number of triggers. We use rather strange
> > interface to select trigger.
> > 
> > > @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> > >  
> > >  	if (msg == NETDEV_CHANGENAME) {
> > >  		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->tx_led_trig);
> > > +		led_trigger_rename(priv->tx_led_trig, name);
> > >  
> > >  		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->rx_led_trig);
> > > +		led_trigger_rename(priv->rx_led_trig, name);
> > >  
> > >  		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->rxtx_led_trig);
> > > +		led_trigger_rename(priv->rxtx_led_trig, name);
> > >  	}
> > >  
> > 
> > I know this is not your fault, but if you have a space or "[]" in
> > netdev names, confusing things will happen.
> 
> Hmm. If we are doing this we really should check trigger names for
> forbidden characters. At least "[] " should be forbidden.

I think you don't expect me to change the patch, but to make this
explicit: My patch doesn't make this problem worse, so this would be an
orthogonal change and doesn't affect this one.

Spaces don't seem to be allowed in netdev names:

uwe@taurus:~$ sudo ip link set wlp3s0 name 'la la'
Error: argument "la la" is wrong: "name" not a valid ifname

(Didn't check if only ip forbids that, of if that is a kernel policy.) I
could rename my device to "lala[]" though.

Best regards
Uwe

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

Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()

[Andy Shevchenko]

On Thu, May 10, 2018 at 2:22 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Thu 2018-05-10 13:21:01, Pavel Machek wrote:

>> I know this is not your fault, but if you have a space or "[]" in
>> netdev names, confusing things will happen.
>
> Hmm. If we are doing this we really should check trigger names for
> forbidden characters. At least "[] " should be forbidden.

So, string_escape_mem() is your helper here.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/3] can: simplify LED trigger handling

[Uwe Kleine-König]

The new function led_trigger_register_format allows one to simplify LED
trigger handling considerably.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/can/led.c   | 30 +++++++-----------------------
 include/linux/can/dev.h |  3 ---
 2 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index 2d7d1b0d20f9..111f9769e9da 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -81,19 +81,9 @@ void devm_can_led_init(struct net_device *netdev)
 		return;
 	}
 
-	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
-		 "%s-tx", netdev->name);
-	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
-		 "%s-rx", netdev->name);
-	snprintf(priv->rxtx_led_trig_name, sizeof(priv->rxtx_led_trig_name),
-		 "%s-rxtx", netdev->name);
-
-	led_trigger_register_simple(priv->tx_led_trig_name,
-				    &priv->tx_led_trig);
-	led_trigger_register_simple(priv->rx_led_trig_name,
-				    &priv->rx_led_trig);
-	led_trigger_register_simple(priv->rxtx_led_trig_name,
-				    &priv->rxtx_led_trig);
+	led_trigger_register_format(&priv->tx_led_trig, "%s-tx", netdev->name);
+	led_trigger_register_format(&priv->rx_led_trig, "%s-rx", netdev->name);
+	led_trigger_register_format(&priv->rxtx_led_trig, "%s-rxtx", netdev->name);
 
 	devres_add(&netdev->dev, res);
 }
@@ -105,23 +95,17 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
 	struct can_priv *priv = safe_candev_priv(netdev);
-	char name[CAN_LED_NAME_SZ];
 
 	if (!priv)
 		return NOTIFY_DONE;
 
-	if (!priv->tx_led_trig || !priv->rx_led_trig || !priv->rxtx_led_trig)
-		return NOTIFY_DONE;
-
 	if (msg == NETDEV_CHANGENAME) {
-		snprintf(name, sizeof(name), "%s-tx", netdev->name);
-		led_trigger_rename(priv->tx_led_trig, name);
+		led_trigger_rename(priv->tx_led_trig, "%s-tx", netdev->name);
 
-		snprintf(name, sizeof(name), "%s-rx", netdev->name);
-		led_trigger_rename(priv->rx_led_trig, name);
+		led_trigger_rename(priv->rx_led_trig, "%s-rx", netdev->name);
 
-		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
-		led_trigger_rename(priv->rxtx_led_trig, name);
+		led_trigger_rename(priv->rxtx_led_trig,
+				   "%s-rxtx", netdev->name);
 	}
 
 	return NOTIFY_DONE;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 055aaf5ed9af..ff358269aa9c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -73,11 +73,8 @@ struct can_priv {
 
 #ifdef CONFIG_CAN_LEDS
 	struct led_trigger *tx_led_trig;
-	char tx_led_trig_name[CAN_LED_NAME_SZ];
 	struct led_trigger *rx_led_trig;
-	char rx_led_trig_name[CAN_LED_NAME_SZ];
 	struct led_trigger *rxtx_led_trig;
-	char rxtx_led_trig_name[CAN_LED_NAME_SZ];
 #endif
 };
 
-- 
2.17.0

Re: [PATCH v3 2/3] can: simplify LED trigger handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 569 bytes --]

On 05/08/2018 12:05 PM, Uwe Kleine-König wrote:
> The new function led_trigger_register_format allows one to simplify LED
> trigger handling considerably.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/3] tty: implement led triggers

[Uwe Kleine-König]

The rx trigger fires when data is pushed to the ldisc by the driver. This
is a bit later than the actual receiving of data but has the nice benefit
that it doesn't need adaption for each driver and isn't in the hot path.

Similarly the tx trigger fires when data was copied from userspace and is
given to the ldisc.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
 drivers/tty/Kconfig                       |   7 +
 drivers/tty/tty_buffer.c                  |   2 +
 drivers/tty/tty_io.c                      |   3 +
 drivers/tty/tty_port.c                    |  32 +++-
 include/linux/tty.h                       |  22 +++
 6 files changed, 256 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

diff --git a/arch/arm/boot/dts/imx25-logitech-baby.dts b/arch/arm/boot/dts/imx25-logitech-baby.dts
new file mode 100644
index 000000000000..39cf763d228b
--- /dev/null
+++ b/arch/arm/boot/dts/imx25-logitech-baby.dts
@@ -0,0 +1,192 @@
+/dts-v1/;
+#include "imx25.dtsi"
+
+/ {
+	model = "Logitech MX25 Baby";
+	compatible = "logitech,baby", "fsl,imx25";
+
+	chosen {
+		linux,stdout-path = &uart2;
+	};
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+
+	clock-frequency = <100000>;
+
+	status = "okay";
+
+	codec: tlv320aic3104@18 {
+		compatible = "ti,tlv320aic310x";
+		reg = <0x18>;
+//		HPVDD-supply
+//		SPRVDD-supply
+//		SPLVDD-supply
+//		AVDD-supply
+//		IOVDD-supply
+//		DVDD-supply
+//		?gpio-reset
+//		?ai31xx-micbias-vg
+	};
+};
+
+&i2c2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+
+	clock-frequency = <100000>;
+
+	status = "okay";
+
+	msp430@10 {
+		reg = <0x10>;
+	};
+};
+
+&iomuxc {
+	baby {
+		pinctrl_fec: fecgrp {
+			fsl,pins = <
+				MX25_PAD_FEC_MDC__FEC_MDC		0x040
+				MX25_PAD_FEC_MDIO__FEC_MDIO		0x1f0
+				MX25_PAD_FEC_TDATA0__FEC_TDATA0		0x040
+				MX25_PAD_FEC_TDATA1__FEC_TDATA1		0x040
+				MX25_PAD_A22__FEC_TDATA2		0x000
+				MX25_PAD_A23__FEC_TDATA3		0x000
+				MX25_PAD_FEC_TX_EN__FEC_TX_EN		0x040
+				MX25_PAD_FEC_RDATA0__FEC_RDATA0		0x0c0
+				MX25_PAD_FEC_RDATA1__FEC_RDATA1		0x0c0
+				MX25_PAD_A20__FEC_RDATA2		0x000
+				MX25_PAD_A21__FEC_RDATA3		0x000
+				MX25_PAD_FEC_RX_DV__FEC_RX_DV		0x0c0
+				MX25_PAD_FEC_TX_CLK__FEC_TX_CLK		0x1c0
+				MX25_PAD_A17__FEC_TX_ERR		0x080
+				MX25_PAD_A19__FEC_RX_ERR		0x080
+				MX25_PAD_A24__FEC_RX_CLK		0x000
+				MX25_PAD_A18__FEC_COL			0x080
+				MX25_PAD_A25__FEC_CRS			0x080
+
+				/*
+				 * PHY_RESET:
+				 * hwref 1: MX25_PIN_A10
+				 * hwref 2: MX25_PIN_CSI_D7
+				 * hwref 3+: MX25_PIN_PWM
+				 */
+				MX25_PAD_PWM__GPIO_1_26			0x0c0
+			>;
+		};
+
+		pinctrl_i2c1: i2c1grp {
+			fsl,pins = <
+				MX25_PAD_I2C1_CLK__I2C1_CLK		0x0a8
+				MX25_PAD_I2C1_DAT__I2C1_DAT		0x0a8
+			>;
+		};
+
+		pinctrl_i2c2: i2c2grp {
+			fsl,pins = <
+				MX25_PAD_GPIO_C__I2C2_SCL		0x0e8
+				MX25_PAD_GPIO_D__I2C2_SDA		0x0a8
+			>;
+		};
+
+		pinctrl_nfc: nfcgrp {
+			fsl,pins = <
+				MX25_PAD_NFRB__NFRB			0x0
+				MX25_PAD_NFWP_B__NFWP_B			0x0
+				MX25_PAD_NFRE_B__NFRE_B			0x0
+				MX25_PAD_NFWE_B__NFWE_B			0x0
+				MX25_PAD_NFALE__NFALE			0x0
+				MX25_PAD_NFCLE__NFCLE			0x0
+				MX25_PAD_NF_CE0__NF_CE0			0x0
+				MX25_PAD_D0__D0				0x0
+				MX25_PAD_D1__D1				0x0
+				MX25_PAD_D2__D2				0x0
+				MX25_PAD_D3__D3				0x0
+				MX25_PAD_D4__D4				0x0
+				MX25_PAD_D5__D5				0x0
+				MX25_PAD_D6__D6				0x0
+				MX25_PAD_D7__D7				0x0
+			>;
+		};
+
+		pinctrl_uart2: uart2grp {
+			fsl,pins = <
+				MX25_PAD_UART2_RXD__UART2_RXD	0x1e0
+				MX25_PAD_UART2_TXD__UART2_TXD	0x0e0
+				/*
+				 * These are configured in the vendor kernel,
+				 * but the corresponding lines don't seem to be
+				 * available:
+				 * MX25_PAD_UART2_RTS__UART2_RTS	0x1e0
+				 * MX25_PAD_UART2_CTS__UART2_CTS	0x0e0
+				 */
+			>;
+		};
+	};
+};
+
+&fec {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec>;
+
+	//phy-reset-gpios = <&gpio1 26 0>;
+	phy-handle = <&ethphy>;
+	phy-mode = "rmii";
+
+	status = "okay";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ethphy: ethernet-phy@1 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <1>;
+			max-speed = <100>;
+		};
+	};
+};
+
+&kpp {
+	linux,keymap = <
+		0x000000cf	/* KEY_PLAY */
+		0x0001004e	/* KEY_KPPLUS */
+		0x00020069	/* KEY_LEFT */
+		0x00030066	/* KEY_HOME */
+		0x0100003b	/* KEY_F1 */
+		0x010100a4	/* KEY_PLAYPAUSE */
+		0x010200a3	/* KEY_NEXTSONG */
+		0x010300a5	/* KEY_PREVIOUSSONG */
+		0x0200003f	/* KEY_F5 */
+		0x0201003e	/* KEY_F4 */
+		0x0202003d	/* KEY_F3 */
+		0x0203003c	/* KEY_F2 */
+		0x03000000	/* KEY_RESERVED */
+		0x03010000	/* KEY_RESERVED */
+		0x0302008e	/* KEY_SLEEP */
+		0x03030040	/* KEY_F6 */
+		>;
+
+	status = "okay";
+};
+
+&nfc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_nfc>;
+
+	nand-on-flash-bbt;
+	nand-bus-width = <8>;
+	nand-ecc-mode = "hw";
+
+	status = "okay";
+};
+
+&uart2 {
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_uart2>;
+
+        status = "okay";
+};
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 0840d27381ea..b119c0fa1f5a 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -41,6 +41,13 @@ config VT
 	  If unsure, say Y, or else you won't be able to do much with your new
 	  shiny Linux system :-)
 
+config TTY_LEDS_TRIGGERS
+	bool "Enable support for TTY actions making LEDs blink"
+	depends on LEDS_TRIGGERS
+	---help---
+	  This enable support for tty triggers. It provides two LED triggers
+	  (rx and tx) for each TTY.
+
 config CONSOLE_TRANSLATIONS
 	depends on VT
 	default y
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c996b6859c5e..364080ce8e91 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -521,6 +521,8 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}
 
+		tty_led_trigger_rx(port);
+
 		count = receive_buf(port, head, count);
 		if (!count)
 			break;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7c838b90a31d..8ef597dc0c3d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -955,6 +955,9 @@ static inline ssize_t do_tty_write(
 		ret = -EFAULT;
 		if (copy_from_user(tty->write_buf, buf, size))
 			break;
+
+		tty_led_trigger_tx(tty->port);
+
 		ret = write(tty, file, tty->write_buf, size);
 		if (ret <= 0)
 			break;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 25d736880013..d313edfa6315 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -37,6 +37,8 @@ static int tty_port_default_receive_buf(struct tty_port *port,
 
 	ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
 
+	tty_led_trigger_rx(port);
+
 	tty_ldisc_deref(disc);
 
 	return ret;
@@ -163,8 +165,31 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
 		return dev;
 	}
 
-	return tty_register_device_attr(driver, index, device, drvdata,
-			attr_grp);
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
+		int ret;
+
+		ret = led_trigger_register_format(&port->led_trigger_rx,
+						  "%s%d-rx", driver->name, index);
+		if (ret < 0)
+			pr_warn("Failed to register rx trigger for %s%d (%d)\n",
+				driver->name, index, ret);
+
+		ret = led_trigger_register_format(&port->led_trigger_tx,
+						  "%s%d-tx", driver->name, index);
+		if (ret < 0)
+			pr_warn("Failed to register tx trigger for %s%d (%d)\n",
+				driver->name, index, ret);
+	}
+
+	dev = tty_register_device_attr(driver, index,
+				       device, drvdata, attr_grp);
+
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS) && IS_ERR(dev)) {
+		led_trigger_unregister_simple(port->led_trigger_tx);
+		led_trigger_unregister_simple(port->led_trigger_rx);
+	}
+
+	return dev;
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
 
@@ -206,6 +231,9 @@ void tty_port_unregister_device(struct tty_port *port,
 	if (ret == 0)
 		return;
 
+	led_trigger_unregister_simple(port->led_trigger_rx);
+	led_trigger_unregister_simple(port->led_trigger_tx);
+
 	tty_unregister_device(driver, index);
 }
 EXPORT_SYMBOL_GPL(tty_port_unregister_device);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1dd587ba6d88..7e48de671bfa 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -13,6 +13,7 @@
 #include <uapi/linux/tty.h>
 #include <linux/rwsem.h>
 #include <linux/llist.h>
+#include <linux/leds.h>
 
 
 /*
@@ -249,8 +250,29 @@ struct tty_port {
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
 	void 			*client_data;
+
+	struct led_trigger	*led_trigger_rx;
+	struct led_trigger	*led_trigger_tx;
 };
 
+static inline void tty_led_trigger(struct led_trigger *trig)
+{
+	unsigned long delay_ms = 50;
+
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS))
+		led_trigger_blink_oneshot(trig, &delay_ms, &delay_ms, 0);
+}
+
+static inline void tty_led_trigger_rx(struct tty_port *port)
+{
+	tty_led_trigger(port->led_trigger_rx);
+}
+
+static inline void tty_led_trigger_tx(struct tty_port *port)
+{
+	tty_led_trigger(port->led_trigger_tx);
+}
+
 /* tty_port::iflags bits -- use atomic bit ops */
 #define TTY_PORT_INITIALIZED	0	/* device is initialized */
 #define TTY_PORT_SUSPENDED	1	/* device is suspended */
-- 
2.17.0

Re: [PATCH v3 3/3] tty: implement led triggers

[Johan Hovold]

On Tue, May 08, 2018 at 12:05:43PM +0200, Uwe Kleine-König wrote:
> The rx trigger fires when data is pushed to the ldisc by the driver. This
> is a bit later than the actual receiving of data but has the nice benefit
> that it doesn't need adaption for each driver and isn't in the hot path.
> 
> Similarly the tx trigger fires when data was copied from userspace and is
> given to the ldisc.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++

Looks like you included more than intended in this patch.

>  drivers/tty/Kconfig                       |   7 +
>  drivers/tty/tty_buffer.c                  |   2 +
>  drivers/tty/tty_io.c                      |   3 +
>  drivers/tty/tty_port.c                    |  32 +++-
>  include/linux/tty.h                       |  22 +++
>  6 files changed, 256 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

Johan

Re: [PATCH v3 3/3] tty: implement led triggers

[Uwe Kleine-König]

On Tue, May 08, 2018 at 12:08:51PM +0200, Johan Hovold wrote:
> On Tue, May 08, 2018 at 12:05:43PM +0200, Uwe Kleine-König wrote:
> > The rx trigger fires when data is pushed to the ldisc by the driver. This
> > is a bit later than the actual receiving of data but has the nice benefit
> > that it doesn't need adaption for each driver and isn't in the hot path.
> > 
> > Similarly the tx trigger fires when data was copied from userspace and is
> > given to the ldisc.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
> 
> Looks like you included more than intended in this patch.

Right, this doesn't belong here. Feel free to assume I didn't include it
in the patch and comment the rest :-)

Best regards
Uwe

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

Re: [PATCH v3 3/3] tty: implement led triggers

[Andy Shevchenko]

On Tue, May 8, 2018 at 1:05 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The rx trigger fires when data is pushed to the ldisc by the driver. This
> is a bit later than the actual receiving of data but has the nice benefit
> that it doesn't need adaption for each driver and isn't in the hot path.
>
> Similarly the tx trigger fires when data was copied from userspace and is
> given to the ldisc.

>  #include <uapi/linux/tty.h>
>  #include <linux/rwsem.h>
>  #include <linux/llist.h>
> +#include <linux/leds.h>

Even for unordered lists of inclusions I would still try to put new
lines in "somehow" ordered positions.
For example here, I would rather put it before llist.h.

-- 
With Best Regards,
Andy Shevchenko