[PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

If userspace doesn't end the input with a newline (which can easily
happen if the write happens from a C program that does write(fd,
iface, strlen(iface))), we may end up including garbage from a
previous, longer value in the device_name. For example

# cat device_name

# printf 'eth12' > device_name
# cat device_name
eth12
# printf 'eth3' > device_name
# cat device_name
eth32

I highly doubt anybody is relying on this behaviour, so switch to
simply copying the bytes (we've already checked that size is <
IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
still have to strip a trailing newline.

This is also preparation for future patches.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 3dd3ed46d473..ddc2b90ad7ec 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev = NULL;
 	}
 
-	strncpy(trigger_data->device_name, buf, size);
+	memcpy(trigger_data->device_name, buf, size);
+	trigger_data->device_name[size] = '\0';
 	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
 		trigger_data->device_name[size - 1] = 0;
 
-- 
2.20.1

Re: [PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Uwe Kleine-König]

Hello,

On Wed, Mar 13, 2019 at 09:26:12PM +0100, Rasmus Villemoes wrote:
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev = NULL;
>  	}
>  
> -	strncpy(trigger_data->device_name, buf, size);
> +	memcpy(trigger_data->device_name, buf, size);
> +	trigger_data->device_name[size] = '\0';

This is open-coding

	strlcpy(trigger_data->device_name, buf, size);

Best regards
Uwe

>  	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
>  		trigger_data->device_name[size - 1] = 0;

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

Re: [PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

On 14/03/2019 10.29, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 13, 2019 at 09:26:12PM +0100, Rasmus Villemoes wrote:
>> --- a/drivers/leds/trigger/ledtrig-netdev.c
>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
>>  		trigger_data->net_dev = NULL;
>>  	}
>>  
>> -	strncpy(trigger_data->device_name, buf, size);
>> +	memcpy(trigger_data->device_name, buf, size);
>> +	trigger_data->device_name[size] = '\0';
> 
> This is open-coding
> 
> 	strlcpy(trigger_data->device_name, buf, size);

No. size here is the number of bytes userspace wrote, which never (well,
almost never, they could do something odd) contain a nul byte. Passing
that as size to strlcpy would guarantee that we chopped off the last
character from the user input. And while I do think the generic sysfs
layer guarantees that the PAGE_SIZE buffer is zeroed before reading from
userspace, I don't really want to rely on buf being a nul-terminated
string (which it must be if using strlcpy - remember that that does a
full strlen() to compute its return value). If anything, one could do
strlcpy(, buf, size+1), but IMO copying the actual characters the user
wrote, then explicitly making that into a nul-terminated string is much
more straight-forward.

Rasmus

Re: [PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Uwe Kleine-König]

On Thu, Mar 14, 2019 at 10:57:14AM +0100, Rasmus Villemoes wrote:
> On 14/03/2019 10.29, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Wed, Mar 13, 2019 at 09:26:12PM +0100, Rasmus Villemoes wrote:
> >> --- a/drivers/leds/trigger/ledtrig-netdev.c
> >> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> >> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
> >>  		trigger_data->net_dev = NULL;
> >>  	}
> >>  
> >> -	strncpy(trigger_data->device_name, buf, size);
> >> +	memcpy(trigger_data->device_name, buf, size);
> >> +	trigger_data->device_name[size] = '\0';
> > 
> > This is open-coding
> > 
> > 	strlcpy(trigger_data->device_name, buf, size);
> 
> No. size here is the number of bytes userspace wrote, which never (well,
> almost never, they could do something odd) contain a nul byte. Passing
> that as size to strlcpy would guarantee that we chopped off the last
> character from the user input.

You're right here, strlcpy isn't a replacement here. I withdraw my
suggestion.

Best regards
Uwe

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

Re: [PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Pavel Machek]

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

Hi!

> If userspace doesn't end the input with a newline (which can easily
> happen if the write happens from a C program that does write(fd,
> iface, strlen(iface))), we may end up including garbage from a
> previous, longer value in the device_name. For example
> 
> # cat device_name
> 
> # printf 'eth12' > device_name
> # cat device_name
> eth12
> # printf 'eth3' > device_name
> # cat device_name
> eth32
> 
> I highly doubt anybody is relying on this behaviour, so switch to
> simply copying the bytes (we've already checked that size is <
> IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
> still have to strip a trailing newline.

  char device_name[IFNAMSIZ];

Ok, good catch reporting the bug, but are you sure the fix is right?

AFAICT the design is that device_name does _not_ have to be zero
terminated, and your fix incorrectly limits the size of device_name.

								Pavel
								
> index 3dd3ed46d473..ddc2b90ad7ec 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev = NULL;
>  	}
>  
> -	strncpy(trigger_data->device_name, buf, size);
> +	memcpy(trigger_data->device_name, buf, size);
> +	trigger_data->device_name[size] = '\0';

I'd do = 0 for consistency with code below.

I believe the strncpy() is right to use here, but code should be
modified so that zero-termination is not required.

>  	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
>  		trigger_data->device_name[size - 1] = 0;
>  
									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 1/4] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

On 14/03/2019 11.14, Pavel Machek wrote:
> Hi!
> 
>> If userspace doesn't end the input with a newline (which can easily
>> happen if the write happens from a C program that does write(fd,
>> iface, strlen(iface))), we may end up including garbage from a
>> previous, longer value in the device_name. For example
>>
>> # cat device_name
>>
>> # printf 'eth12' > device_name
>> # cat device_name
>> eth12
>> # printf 'eth3' > device_name
>> # cat device_name
>> eth32
>>
>> I highly doubt anybody is relying on this behaviour, so switch to
>> simply copying the bytes (we've already checked that size is <
>> IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
>> still have to strip a trailing newline.
> 
>   char device_name[IFNAMSIZ];
> 
> Ok, good catch reporting the bug, but are you sure the fix is right?
> 
> AFAICT the design is that device_name does _not_ have to be zero
> terminated, and your fix incorrectly limits the size of device_name.

Yes, I'm sure.

/**
 *      dev_valid_name - check if name is okay for network device
 *      @name: name string
 *
 *      Network device names need to be valid file names to
 *      to allow sysfs to work.  We also disallow any kind of
 *      whitespace.
 */
bool dev_valid_name(const char *name)
{
        if (*name == '\0')
                return false;
        if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
                return false;

so no netdevice name has a string length > 15 (IOW, there must be a nul
byte within the first 16 bytes of name). Also note all the places a
net_device->name is printed with a simple %s, so they are definitely
always 0-terminated.

Moreover, I'm actually not limiting anything more than was already done;
we already reject any input from userspace >= 16 bytes. I'm simply
ensuring that we're never confused by leftover garbage.

>> --- a/drivers/leds/trigger/ledtrig-netdev.c
>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
>>  		trigger_data->net_dev = NULL;
>>  	}
>>  
>> -	strncpy(trigger_data->device_name, buf, size);
>> +	memcpy(trigger_data->device_name, buf, size);
>> +	trigger_data->device_name[size] = '\0';
> 
> I'd do = 0 for consistency with code below.

I'd rather the other way around :) but yeah, let's just be consistent.
I'll fix in next version.

> I believe the strncpy() is right to use here, but code should be
> modified so that zero-termination is not required.

So, I believe the above should convince you that strncpy is wrong. Or
rather, strncpy is really just a convoluted memcpy: the userspace input
doesn't contain a nul among the size bytes [1], so the "copy all the
bytes, but don't nul-terminate" semantics of strncpy kick in - which is
often a security bug, but the code is such that the zero at
device_name[15] (because it was originally kzalloc'ed) is never
overwritten. So in theory, we could keep strncpy and just add the
nul-termination, but the str* prefix is very misleading (which is
probably why the bug happened in the first place).

[1] And if it did, the "zero the rest of the output buffer" semantics
kick in. That's functionally equivalent to my memcpy() + write one nul
byte, since nothing after that first nul byte in device_name would ever
be inspected.

Rasmus

Re: [PATCH 1/4] leds: netdev trigger: use memcpy in device_name_store

[Pavel Machek]

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

Hi!

> >> If userspace doesn't end the input with a newline (which can easily
> >> happen if the write happens from a C program that does write(fd,
> >> iface, strlen(iface))), we may end up including garbage from a
> >> previous, longer value in the device_name. For example
> >>
> >> # cat device_name
> >>
> >> # printf 'eth12' > device_name
> >> # cat device_name
> >> eth12
> >> # printf 'eth3' > device_name
> >> # cat device_name
> >> eth32
> >>
> >> I highly doubt anybody is relying on this behaviour, so switch to
> >> simply copying the bytes (we've already checked that size is <
> >> IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
> >> still have to strip a trailing newline.
> > 
> >   char device_name[IFNAMSIZ];
> > 
> > Ok, good catch reporting the bug, but are you sure the fix is right?
> > 
> > AFAICT the design is that device_name does _not_ have to be zero
> > terminated, and your fix incorrectly limits the size of device_name.
> 
> Yes, I'm sure.
> 
> /**
>  *      dev_valid_name - check if name is okay for network device
>  *      @name: name string
>  *
>  *      Network device names need to be valid file names to
>  *      to allow sysfs to work.  We also disallow any kind of
>  *      whitespace.
>  */
> bool dev_valid_name(const char *name)
> {
>         if (*name == '\0')
>                 return false;
>         if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
>                 return false;
> 
> so no netdevice name has a string length > 15 (IOW, there must be a nul
> byte within the first 16 bytes of name). Also note all the places a
> net_device->name is printed with a simple %s, so they are definitely
> always 0-terminated.
> 
> Moreover, I'm actually not limiting anything more than was already done;
> we already reject any input from userspace >= 16 bytes. I'm simply
> ensuring that we're never confused by leftover garbage.

Ok. Sorry for the noise.

> >> --- a/drivers/leds/trigger/ledtrig-netdev.c
> >> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> >> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
> >>  		trigger_data->net_dev = NULL;
> >>  	}
> >>  
> >> -	strncpy(trigger_data->device_name, buf, size);
> >> +	memcpy(trigger_data->device_name, buf, size);
> >> +	trigger_data->device_name[size] = '\0';
> > 
> > I'd do = 0 for consistency with code below.
> 
> I'd rather the other way around :) but yeah, let's just be consistent.
> I'll fix in next version.

Or just use your version and fix both places :-). Your option, as long
as it is consistent. 

> > I believe the strncpy() is right to use here, but code should be
> > modified so that zero-termination is not required.
> 
> So, I believe the above should convince you that strncpy is wrong. Or
> rather, strncpy is really just a convoluted memcpy: the userspace input
> doesn't contain a nul among the size bytes [1], so the "copy all the
> bytes, but don't nul-terminate" semantics of strncpy kick in - which is
> often a security bug, but the code is such that the zero at
> device_name[15] (because it was originally kzalloc'ed) is never
> overwritten. So in theory, we could keep strncpy and just add the
> nul-termination, but the str* prefix is very misleading (which is
> probably why the bug happened in the first place).
> 
> [1] And if it did, the "zero the rest of the output buffer" semantics
> kick in. That's functionally equivalent to my memcpy() + write one nul
> byte, since nothing after that first nul byte in device_name would ever
> be inspected.

Actually "zero the rest" semantics kind of makes sense here.

No, I'm sorry, I don't understand. How are you getting stale data
there?

> >> # printf 'eth12' > device_name
> >> # cat device_name
> >> eth12
> >> # printf 'eth3' > device_name
> >> # cat device_name
> >> eth32

strncpy() should zero the rest, and you should get "eth3"...?!

									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 2/4] leds: netdev trigger: factor out middle part of device_name_store

[Rasmus Villemoes]

Take the part of device_name_store that puts the old device (if any),
copies the new device name, looks the name up etc. into a separate
helper function. This is preparation for using that helper from a
function that will initialize the led_netdev_data from a device tree
node. No functional change.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 30 ++++++++++++++++-----------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index ddc2b90ad7ec..55153a7e8433 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -104,19 +104,9 @@ static ssize_t device_name_show(struct device *dev,
 	return len;
 }
 
-static ssize_t device_name_store(struct device *dev,
-				 struct device_attribute *attr, const char *buf,
-				 size_t size)
+static void set_device(struct led_netdev_data *trigger_data,
+		       const char *buf, size_t size)
 {
-	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
-
-	if (size >= IFNAMSIZ)
-		return -EINVAL;
-
-	cancel_delayed_work_sync(&trigger_data->work);
-
-	spin_lock_bh(&trigger_data->lock);
-
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
 		trigger_data->net_dev = NULL;
@@ -137,6 +127,22 @@ static ssize_t device_name_store(struct device *dev,
 			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 
 	trigger_data->last_activity = 0;
+}
+
+static ssize_t device_name_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t size)
+{
+	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+
+	if (size >= IFNAMSIZ)
+		return -EINVAL;
+
+	cancel_delayed_work_sync(&trigger_data->work);
+
+	spin_lock_bh(&trigger_data->lock);
+
+	set_device(trigger_data, buf, size);
 
 	set_baseline_state(trigger_data);
 	spin_unlock_bh(&trigger_data->lock);
-- 
2.20.1

Re: [PATCH 2/4] leds: netdev trigger: factor out middle part of device_name_store

[Uwe Kleine-König]

Hello,

On Wed, Mar 13, 2019 at 09:26:13PM +0100, Rasmus Villemoes wrote:
> +static void set_device(struct led_netdev_data *trigger_data,
> +		       const char *buf, size_t size)

"set_device" is a very generic name. Can you please prefix it with
"ledtrig_netdev_"?

Best regards
Uwe

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

Re: [PATCH 2/4] leds: netdev trigger: factor out middle part of device_name_store

[Rasmus Villemoes]

On 14/03/2019 10.31, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 13, 2019 at 09:26:13PM +0100, Rasmus Villemoes wrote:
>> +static void set_device(struct led_netdev_data *trigger_data,
>> +		       const char *buf, size_t size)
> 
> "set_device" is a very generic name. Can you please prefix it with
> "ledtrig_netdev_"?

Sure. Will do next round.

Re: [PATCH 2/4] leds: netdev trigger: factor out middle part of device_name_store

[Pavel Machek]

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

On Thu 2019-03-14 10:31:11, Uwe Kleine-König wrote:
1;2802;0c> Hello,
> 
> On Wed, Mar 13, 2019 at 09:26:13PM +0100, Rasmus Villemoes wrote:
> > +static void set_device(struct led_netdev_data *trigger_data,
> > +		       const char *buf, size_t size)
> 
> "set_device" is a very generic name. Can you please prefix it with
> "ledtrig_netdev_"?

Its static.
									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 2/4] leds: netdev trigger: factor out middle part of device_name_store

[Uwe Kleine-König]

Hello,

On Thu, Mar 14, 2019 at 11:15:28AM +0100, Pavel Machek wrote:
> On Thu 2019-03-14 10:31:11, Uwe Kleine-König wrote:
> 1;2802;0c> Hello,
> > 
> > On Wed, Mar 13, 2019 at 09:26:13PM +0100, Rasmus Villemoes wrote:
> > > +static void set_device(struct led_netdev_data *trigger_data,
> > > +		       const char *buf, size_t size)
> > 
> > "set_device" is a very generic name. Can you please prefix it with
> > "ledtrig_netdev_"?
> 
> Its static.

Still it pollutes the name space for tools like ctags. Currently there
is no other driver that provides "set_device" but there are symbol names
that are used for different purposes and that's annoying. Right, for
global names that might even be used from other source files, this is
worse. But IMHO it's still annoying enough in cases like this to not do
it.

Best regards
Uwe

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

[PATCH 3/4] leds: netdev trigger: add documentation to leds/common.txt

[Rasmus Villemoes]

The various sysfs files used to configure a netdev-triggered LED are
already documented, but let's also add "netdev" to the list of things
one can reasonably set linux,default-trigger to.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/leds/common.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..7cb88460a47c 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -39,6 +39,9 @@ Optional properties for child nodes:
      "timer" - LED flashes at a fixed, configurable rate
      "pattern" - LED alters the brightness for the specified duration with one
                  software timer (requires "led-pattern" property)
+     "netdev" - LED can be configured via sysfs files (see
+                Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
+                to reflect the state and activity of a net device.
 
 - led-pattern : Array of integers with default pattern for certain triggers.
                 Each trigger may parse this property differently:
-- 
2.20.1

[PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

It can be quite convenient to initialize a netdev-triggered LED with a
device name and setting the rx,tx,link properties from device tree,
instead of having to do that in an init script in userspace.

My main motivation for this is to be able to switch away from the
deprecated CONFIG_CAN_LEDS, so add an example based on that and add a
pointer in the net/can/Kconfig file.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/leds/common.txt       | 17 ++++++++++
 drivers/leds/trigger/ledtrig-netdev.c         | 31 +++++++++++++++++++
 drivers/net/can/Kconfig                       |  3 +-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 7cb88460a47c..4f3a97e73417 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -43,6 +43,23 @@ Optional properties for child nodes:
                 Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
                 to reflect the state and activity of a net device.
 
+                The optional child node netdev can be used to
+                configure initial values for the link, rx, tx and
+                device_name properties. For example, setting
+                linux,default-trigger = "netdev" and adding the child
+                node
+
+                netdev {
+                    rx;
+                    tx;
+                    link;
+                    device-name = "can0";
+                };
+
+                can be used to replace 'linux,default-trigger =
+                "can0-rxtx"' that relies on the deprecated
+                CONFIG_CAN_LEDS.
+
 - led-pattern : Array of integers with default pattern for certain triggers.
                 Each trigger may parse this property differently:
                 - one-shot : two numbers specifying delay on and delay off (in ms),
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 55153a7e8433..1f7c86df1e91 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
@@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work)
 			(atomic_read(&trigger_data->interval)*2));
 }
 
+static void netdev_trig_of_init(struct led_classdev *led_cdev,
+				struct led_netdev_data *trigger_data)
+{
+	struct device_node *np = led_cdev->dev->of_node;
+	const char *device_name;
+
+	if (!np)
+		return;
+	np = of_get_child_by_name(np, "netdev");
+	if (!np)
+		return;
+
+	if (of_property_read_bool(np, "link"))
+		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
+	if (of_property_read_bool(np, "tx"))
+		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
+	if (of_property_read_bool(np, "rx"))
+		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
+	if (!of_property_read_string(np, "device-name", &device_name)) {
+		unsigned len = strlen(device_name);
+
+		if (len < IFNAMSIZ)
+			set_device(trigger_data, device_name, len);
+	}
+	set_baseline_state(trigger_data);
+
+	of_node_put(np);
+}
+
 static int netdev_trig_activate(struct led_classdev *led_cdev)
 {
 	struct led_netdev_data *trigger_data;
@@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	trigger_data->mode = 0;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
 	trigger_data->last_activity = 0;
+	netdev_trig_of_init(led_cdev, trigger_data);
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e0f0ad7a550a..91703a96b636 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -77,7 +77,8 @@ config CAN_LEDS
 	# everything that this driver is doing. This is marked as broken
 	# because it uses stuff that is intended to be changed or removed.
 	# Please consider switching to the netdev trigger and confirm it
-	# fulfills your needs instead of fixing this driver.
+	# fulfills your needs instead of fixing this driver. See e.g.
+	# Documentation/devicetree/bindings/leds/common.txt
 	depends on BROKEN
 	select LEDS_TRIGGERS
 	---help---
-- 
2.20.1

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Uwe Kleine-König]

Hello,

On Wed, Mar 13, 2019 at 09:26:15PM +0100, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script in userspace.

Well, you'd do this in an udev rule instead of an init script.

> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS, so add an example based on that and add a
> pointer in the net/can/Kconfig file.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/leds/common.txt       | 17 ++++++++++
>  drivers/leds/trigger/ledtrig-netdev.c         | 31 +++++++++++++++++++
>  drivers/net/can/Kconfig                       |  3 +-
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..4f3a97e73417 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,23 @@ Optional properties for child nodes:
>                  Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                  to reflect the state and activity of a net device.
>  
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example, setting
> +                linux,default-trigger = "netdev" and adding the child
> +                node
> +
> +                netdev {
> +                    rx;
> +                    tx;
> +                    link;
> +                    device-name = "can0";

Maybe make this:

	device = <&can0>;

?

> +                };
> +
> +                can be used to replace 'linux,default-trigger =
> +                "can0-rxtx"' that relies on the deprecated
> +                CONFIG_CAN_LEDS.

Mentioning "CONFIG_CAN_LEDS" is wrong for a binding document that is
supposed to be independent from the kernel.

>  - led-pattern : Array of integers with default pattern for certain triggers.
>                  Each trigger may parse this property differently:
>                  - one-shot : two numbers specifying delay on and delay off (in ms),
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 55153a7e8433..1f7c86df1e91 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -20,6 +20,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
> +#include <linux/of.h>
>  #include <linux/spinlock.h>
>  #include <linux/timer.h>
>  #include "../leds.h"
> @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work)
>  			(atomic_read(&trigger_data->interval)*2));
>  }
>  
> +static void netdev_trig_of_init(struct led_classdev *led_cdev,
> +				struct led_netdev_data *trigger_data)
> +{
> +	struct device_node *np = led_cdev->dev->of_node;
> +	const char *device_name;
> +
> +	if (!np)
> +		return;
> +	np = of_get_child_by_name(np, "netdev");
> +	if (!np)
> +		return;
> +
> +	if (of_property_read_bool(np, "link"))
> +		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
> +	if (of_property_read_bool(np, "tx"))
> +		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
> +	if (of_property_read_bool(np, "rx"))
> +		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
> +	if (!of_property_read_string(np, "device-name", &device_name)) {
> +		unsigned len = strlen(device_name);
> +
> +		if (len < IFNAMSIZ)
> +			set_device(trigger_data, device_name, len);

if set_device contained the length check you'd have it in only one
place.

> +	}
> +	set_baseline_state(trigger_data);
> +
> +	of_node_put(np);
> +}
> +
>  static int netdev_trig_activate(struct led_classdev *led_cdev)
>  {
>  	struct led_netdev_data *trigger_data;
> @@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  	trigger_data->mode = 0;
>  	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>  	trigger_data->last_activity = 0;
> +	netdev_trig_of_init(led_cdev, trigger_data);
>  
>  	led_set_trigger_data(led_cdev, trigger_data);
>  
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index e0f0ad7a550a..91703a96b636 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -77,7 +77,8 @@ config CAN_LEDS
>  	# everything that this driver is doing. This is marked as broken
>  	# because it uses stuff that is intended to be changed or removed.
>  	# Please consider switching to the netdev trigger and confirm it
> -	# fulfills your needs instead of fixing this driver.
> +	# fulfills your needs instead of fixing this driver. See e.g.
> +	# Documentation/devicetree/bindings/leds/common.txt
>  	depends on BROKEN
>  	select LEDS_TRIGGERS
>  	---help---

This hunk can better live in the patch adding to
Documentation/devicetree/bindings/leds/common.txt or a separate patch.

Best regards
Uwe

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

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

On 14/03/2019 10.36, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 13, 2019 at 09:26:15PM +0100, Rasmus Villemoes wrote:
>> It can be quite convenient to initialize a netdev-triggered LED with a
>> device name and setting the rx,tx,link properties from device tree,
>> instead of having to do that in an init script in userspace.
> 
> Well, you'd do this in an udev rule instead of an init script.

Perhaps, if my userspace had udev. But even then, if I have to modify my
userspace in order to upgrade the kernel (which is what I'm trying to
avoid, for a number of reasons), I'd probably still do it in a
seven-line init script instead of trying to wrap my head around
udev/mdev rule writing.

Initializing the netdev trigger from DT data is certainly in line with
what is possible for certain other triggers via the led-pattern property.

Just out of curiosity, do you have a udev rule (template) for this?

>>  
>> +                The optional child node netdev can be used to
>> +                configure initial values for the link, rx, tx and
>> +                device_name properties. For example, setting
>> +                linux,default-trigger = "netdev" and adding the child
>> +                node
>> +
>> +                netdev {
>> +                    rx;
>> +                    tx;
>> +                    link;
>> +                    device-name = "can0";
> 
> Maybe make this:
> 
> 	device = <&can0>;

Huh? I don't think there's any guarantee that the netdevice has a DT
node/phandle, and even if it did, how would the init code map from that
phandle to the string it should fill into ->device_name? Since DT
natively has bools and strings, it's much more natural to just use those
types which map nicely to the sysfs properties.

> ?
> 
>> +                };
>> +
>> +                can be used to replace 'linux,default-trigger =
>> +                "can0-rxtx"' that relies on the deprecated
>> +                CONFIG_CAN_LEDS.
> 
> Mentioning "CONFIG_CAN_LEDS" is wrong for a binding document that is
> supposed to be independent from the kernel.

Well, this is in relation to the already linux-specific
linux,default-trigger binding. But I can certainly reword this to a
simple example that doesn't mention what it replaces and why, and just
move the mentioning of CONFIG_CAN_LEDS to the commit message.

>>  - led-pattern : Array of integers with default pattern for certain triggers.
>>                  Each trigger may parse this property differently:
>>                  - one-shot : two numbers specifying delay on and delay off (in ms),
>> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
>> index 55153a7e8433..1f7c86df1e91 100644
>> --- a/drivers/leds/trigger/ledtrig-netdev.c
>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/list.h>
>>  #include <linux/module.h>
>>  #include <linux/netdevice.h>
>> +#include <linux/of.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/timer.h>
>>  #include "../leds.h"
>> @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work)
>>  			(atomic_read(&trigger_data->interval)*2));
>>  }
>>  
>> +static void netdev_trig_of_init(struct led_classdev *led_cdev,
>> +				struct led_netdev_data *trigger_data)
>> +{
>> +	struct device_node *np = led_cdev->dev->of_node;
>> +	const char *device_name;
>> +
>> +	if (!np)
>> +		return;
>> +	np = of_get_child_by_name(np, "netdev");
>> +	if (!np)
>> +		return;
>> +
>> +	if (of_property_read_bool(np, "link"))
>> +		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
>> +	if (of_property_read_bool(np, "tx"))
>> +		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
>> +	if (of_property_read_bool(np, "rx"))
>> +		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
>> +	if (!of_property_read_string(np, "device-name", &device_name)) {
>> +		unsigned len = strlen(device_name);
>> +
>> +		if (len < IFNAMSIZ)
>> +			set_device(trigger_data, device_name, len);
> 
> if set_device contained the length check you'd have it in only one
> place.

I considered that, but rejected it because it complicates the existing
user of set_device() (where the call happens under a lock; from the
_of_init function, nobody else knows about the trigger_data instance
yet). But on second thought, I think you're right that it's better done
in that one place. So I'll refactor.

>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index e0f0ad7a550a..91703a96b636 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -77,7 +77,8 @@ config CAN_LEDS
>>  	# everything that this driver is doing. This is marked as broken
>>  	# because it uses stuff that is intended to be changed or removed.
>>  	# Please consider switching to the netdev trigger and confirm it
>> -	# fulfills your needs instead of fixing this driver.
>> +	# fulfills your needs instead of fixing this driver. See e.g.
>> +	# Documentation/devicetree/bindings/leds/common.txt
>>  	depends on BROKEN
>>  	select LEDS_TRIGGERS
>>  	---help---
> 
> This hunk can better live in the patch adding to
> Documentation/devicetree/bindings/leds/common.txt or a separate patch.

OK, will move to separate patch.

Rasmus

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Pavel Machek]

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

On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script in userspace.
> 
> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS, so add an example based on that and add a
> pointer in the net/can/Kconfig file.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/leds/common.txt       | 17 ++++++++++
>  drivers/leds/trigger/ledtrig-netdev.c         | 31 +++++++++++++++++++
>  drivers/net/can/Kconfig                       |  3 +-
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..4f3a97e73417 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,23 @@ Optional properties for child nodes:
>                  Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                  to reflect the state and activity of a net device.
>  
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example, setting
> +                linux,default-trigger = "netdev" and adding the child
> +                node
> +
> +                netdev {
> +                    rx;
> +                    tx;
> +                    link;
> +                    device-name = "can0";
> +                };
> +
> +                can be used to replace 'linux,default-trigger =
> +                "can0-rxtx"' that relies on the deprecated
> +                CONFIG_CAN_LEDS.

I'm sorry, but no, not like this. I see it works for you, only having
single can device, but it would quickly break with two of them and two
ethernets are rather common.

So this will need to be device = <&phandle_of_ethernet_device> or
something like that. There may be example with usb port triggers.

Thanks,
									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 4/4] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

On 14/03/2019 11.29, Pavel Machek wrote:
> On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote:
>> It can be quite convenient to initialize a netdev-triggered LED with a
>> device name and setting the rx,tx,link properties from device tree,
>> instead of having to do that in an init script in userspace.
>>
>> +                The optional child node netdev can be used to
>> +                configure initial values for the link, rx, tx and
>> +                device_name properties. For example, setting
>> +                linux,default-trigger = "netdev" and adding the child
>> +                node
>> +
>> +                netdev {
>> +                    rx;
>> +                    tx;
>> +                    link;
>> +                    device-name = "can0";
>> +                };
>> +
>> +                can be used to replace 'linux,default-trigger =
>> +                "can0-rxtx"' that relies on the deprecated
>> +                CONFIG_CAN_LEDS.
> 
> I'm sorry, but no, not like this. I see it works for you, only having
> single can device, but it would quickly break with two of them and two
> ethernets are rather common.
> 
> So this will need to be device = <&phandle_of_ethernet_device> or
> something like that. There may be example with usb port triggers.

Huh? I have two CAN devices, and there are two LEDs on the front panel
labeled CAN-A and CAN-B; my device tree nodes for that are

		cana {
			label = "cana:green:activity";
			gpios = <&gpio0 10 0>;
			default-state = "off";
			linux,default-trigger = "netdev";

			netdev {
				rx;
				tx;
				link;
				device-name = "can0";
			};
		};

		canb {
			label = "canb:green:activity";
			gpios = <&gpio0 5 0>;
			default-state = "off";
			linux,default-trigger = "netdev";

			netdev {
				rx;
				tx;
				link;
				device-name = "can1";
			};
		};

and this works just fine. The only change from the old DT is the
addition of the netdev nodes and changing linux,default-trigger from
"can0-rxtx", "can1-rxtx" to both "netdev".

Rasmus

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Pavel Machek]

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

On Thu 2019-03-14 12:26:38, Rasmus Villemoes wrote:
> On 14/03/2019 11.29, Pavel Machek wrote:
> > On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote:
> >> It can be quite convenient to initialize a netdev-triggered LED with a
> >> device name and setting the rx,tx,link properties from device tree,
> >> instead of having to do that in an init script in userspace.
> >>
> >> +                The optional child node netdev can be used to
> >> +                configure initial values for the link, rx, tx and
> >> +                device_name properties. For example, setting
> >> +                linux,default-trigger = "netdev" and adding the child
> >> +                node
> >> +
> >> +                netdev {
> >> +                    rx;
> >> +                    tx;
> >> +                    link;
> >> +                    device-name = "can0";
> >> +                };
> >> +
> >> +                can be used to replace 'linux,default-trigger =
> >> +                "can0-rxtx"' that relies on the deprecated
> >> +                CONFIG_CAN_LEDS.
> > 
> > I'm sorry, but no, not like this. I see it works for you, only having
> > single can device, but it would quickly break with two of them and two
> > ethernets are rather common.
> > 
> > So this will need to be device = <&phandle_of_ethernet_device> or
> > something like that. There may be example with usb port triggers.
> 
> Huh? I have two CAN devices, and there are two LEDs on the front panel
> labeled CAN-A and CAN-B; my device tree nodes for that are
...
> and this works just fine. The only change from the old DT is the
> addition of the netdev nodes and changing linux,default-trigger from
> "can0-rxtx", "can1-rxtx" to both "netdev".

Yeah and now insert the modules for the can devices in different
order... May not happen in your case but will be fairly common for
ethernets.

									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 4/4] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

On 14/03/2019 13.00, Pavel Machek wrote:
linux,default-trigger from
> 
> Yeah and now insert the modules for the can devices in different
> order... May not happen in your case but will be fairly common for
> ethernets.

Unpredictable device names is not a problem I'm trying to solve, nor one
that actually occurs on embedded systems. And even if one has such a
system, one is likely running udev or similar to rename devices to
something sane and predictable, in which case a fixed string _still_
works fine. Or one uses udev rules to munge the netdev trigger sysfs
files, and has applications dealing with devices appearing as banana457.

OK, so there's of_find_net_device_by_node, so a phandle solution might
also be possible, and might work just as well for my case. But I still
think initializing with a fixed string is the simplest and sanest thing
to do.

Rasmus

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Pavel Machek]

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

On Thu 2019-03-14 14:19:42, Rasmus Villemoes wrote:
> On 14/03/2019 13.00, Pavel Machek wrote:
> linux,default-trigger from
> > 
> > Yeah and now insert the modules for the can devices in different
> > order... May not happen in your case but will be fairly common for
> > ethernets.
> 
> Unpredictable device names is not a problem I'm trying to solve, nor one
> that actually occurs on embedded systems. And even if one has such a
> system, one is likely running udev or similar to rename devices to
> something sane and predictable, in which case a fixed string _still_
> works fine. Or one uses udev rules to munge the netdev trigger sysfs
> files, and has applications dealing with devices appearing as banana457.
> 
> OK, so there's of_find_net_device_by_node, so a phandle solution might
> also be possible, and might work just as well for my case. But I still
> think initializing with a fixed string is the simplest and sanest thing
> to do.

Simplest, yes. Mergeable, no. Sorry.

Udev will not help with renaming -- it runs after kernel boot, while
udev parsing is usually done before userland starts.

Plus, device tree should really describe hardware, and be usable with
other operating systems. Of course, there are exceptions, but "can0"
is unneccessarily linux-specific.

									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 4/4] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

On 17/03/2019 20.11, Pavel Machek wrote:
> On Thu 2019-03-14 14:19:42, Rasmus Villemoes wrote:
>> On 14/03/2019 13.00, Pavel Machek wrote:
>> linux,default-trigger from
>>>
> Udev will not help with renaming -- it runs after kernel boot, while
> udev parsing is usually done before userland starts.

This is simply plain wrong. The netdev trigger explicitly tracks a
netdevice _by name_, and has netdev notifier in place to get notified
should a netdevice by that name ever appear.

> Plus, device tree should really describe hardware, and be usable with
> other operating systems. Of course, there are exceptions, but "can0"
> is unneccessarily linux-specific.

And "linux,netdev-trigger" is not linux-specific? And why are you so
hung up on the can example, it would work just as fine for usb0 or whatever.

Anyway, to whoever is responsible for merging this stuff, please at most
apply patch 1, factoring out the string-to-netdev handling doesn't make
much sense without the final configure-from-dt stuff.

Rasmus

Re: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree

[Jacek Anaszewski]

Hi Rasmus,

Thank you for the patch set.

I have one comment below.

On 3/13/19 9:26 PM, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script in userspace.
> 
> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS, so add an example based on that and add a
> pointer in the net/can/Kconfig file.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   .../devicetree/bindings/leds/common.txt       | 17 ++++++++++
>   drivers/leds/trigger/ledtrig-netdev.c         | 31 +++++++++++++++++++
>   drivers/net/can/Kconfig                       |  3 +-
>   3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..4f3a97e73417 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,23 @@ Optional properties for child nodes:
>                   Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                   to reflect the state and activity of a net device.
>   
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example, setting
> +                linux,default-trigger = "netdev" and adding the child
> +                node
> +
> +                netdev {
> +                    rx;
> +                    tx;
> +                    link;
> +                    device-name = "can0";
> +                };
> +
> +                can be used to replace 'linux,default-trigger =
> +                "can0-rxtx"' that relies on the deprecated
> +                CONFIG_CAN_LEDS.
> +
>   - led-pattern : Array of integers with default pattern for certain triggers.
>                   Each trigger may parse this property differently:
>                   - one-shot : two numbers specifying delay on and delay off (in ms),
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 55153a7e8433..1f7c86df1e91 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -20,6 +20,7 @@
>   #include <linux/list.h>
>   #include <linux/module.h>
>   #include <linux/netdevice.h>
> +#include <linux/of.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include "../leds.h"
> @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work)
>   			(atomic_read(&trigger_data->interval)*2));
>   }
>   
> +static void netdev_trig_of_init(struct led_classdev *led_cdev,
> +				struct led_netdev_data *trigger_data)
> +{
> +	struct device_node *np = led_cdev->dev->of_node;
> +	const char *device_name;
> +
> +	if (!np)
> +		return;
> +	np = of_get_child_by_name(np, "netdev");
> +	if (!np)
> +		return;
> +
> +	if (of_property_read_bool(np, "link"))
> +		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
> +	if (of_property_read_bool(np, "tx"))
> +		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
> +	if (of_property_read_bool(np, "rx"))
> +		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
> +	if (!of_property_read_string(np, "device-name", &device_name)) {
> +		unsigned len = strlen(device_name);
> +
> +		if (len < IFNAMSIZ)
> +			set_device(trigger_data, device_name, len);
> +	}
> +	set_baseline_state(trigger_data);
> +
> +	of_node_put(np);
> +}
> +
>   static int netdev_trig_activate(struct led_classdev *led_cdev)
>   {
>   	struct led_netdev_data *trigger_data;
> @@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>   	trigger_data->mode = 0;
>   	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>   	trigger_data->last_activity = 0;
> +	netdev_trig_of_init(led_cdev, trigger_data);

You want to execute this only if recently introduced flag
LED_INIT_DEFAULT_TRIGGER is set. Please compare how
drivers/leds/trigger/ledtrig-pattern.c uses it.

>   
>   	led_set_trigger_data(led_cdev, trigger_data);
>   
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index e0f0ad7a550a..91703a96b636 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -77,7 +77,8 @@ config CAN_LEDS
>   	# everything that this driver is doing. This is marked as broken
>   	# because it uses stuff that is intended to be changed or removed.
>   	# Please consider switching to the netdev trigger and confirm it
> -	# fulfills your needs instead of fixing this driver.
> +	# fulfills your needs instead of fixing this driver. See e.g.
> +	# Documentation/devicetree/bindings/leds/common.txt
>   	depends on BROKEN
>   	select LEDS_TRIGGERS
>   	---help---
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

If userspace doesn't end the input with a newline (which can easily
happen if the write happens from a C program that does write(fd,
iface, strlen(iface))), we may end up including garbage from a
previous, longer value in the device_name. For example

# cat device_name

# printf 'eth12' > device_name
# cat device_name
eth12
# printf 'eth3' > device_name
# cat device_name
eth32

I highly doubt anybody is relying on this behaviour, so switch to
simply copying the bytes (we've already checked that size is <
IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
still have to strip a trailing newline.

This is also preparation for future patches.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 3dd3ed46d473..8d476b92f58c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev = NULL;
 	}
 
-	strncpy(trigger_data->device_name, buf, size);
+	memcpy(trigger_data->device_name, buf, size);
+	trigger_data->device_name[size] = 0;
 	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
 		trigger_data->device_name[size - 1] = 0;
 
-- 
2.20.1

Re: [PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Pavel Machek]

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

On Thu 2019-03-14 15:06:14, Rasmus Villemoes wrote:
> If userspace doesn't end the input with a newline (which can easily
> happen if the write happens from a C program that does write(fd,
> iface, strlen(iface))), we may end up including garbage from a
> previous, longer value in the device_name. For example
> 
> # cat device_name
> 
> # printf 'eth12' > device_name
> # cat device_name
> eth12
> # printf 'eth3' > device_name
> # cat device_name
> eth32
> 
> I highly doubt anybody is relying on this behaviour, so switch to
> simply copying the bytes (we've already checked that size is <
> IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
> still have to strip a trailing newline.
> 
> This is also preparation for future patches.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Jacek Anaszewski]

Hi Rasmus,

Thank you for the patch.

On 3/14/19 3:06 PM, Rasmus Villemoes wrote:
> If userspace doesn't end the input with a newline (which can easily
> happen if the write happens from a C program that does write(fd,
> iface, strlen(iface))), we may end up including garbage from a
> previous, longer value in the device_name. For example
> 
> # cat device_name
> 
> # printf 'eth12' > device_name
> # cat device_name
> eth12
> # printf 'eth3' > device_name
> # cat device_name
> eth32
> 
> I highly doubt anybody is relying on this behaviour, so switch to
> simply copying the bytes (we've already checked that size is <
> IFNAMSIZ) and unconditionally zero-terminate it; of course, we also
> still have to strip a trailing newline.
> 
> This is also preparation for future patches.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   drivers/leds/trigger/ledtrig-netdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 3dd3ed46d473..8d476b92f58c 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -122,7 +122,8 @@ static ssize_t device_name_store(struct device *dev,
>   		trigger_data->net_dev = NULL;
>   	}
>   
> -	strncpy(trigger_data->device_name, buf, size);
> +	memcpy(trigger_data->device_name, buf, size);
> +	trigger_data->device_name[size] = 0;
>   	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
>   		trigger_data->device_name[size - 1] = 0;
>   
> 

Added tag:

Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")

and applied to the fixes-for-5.1-rc3 branch.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

On 26/03/2019 20.53, Jacek Anaszewski wrote:
> Hi Rasmus,
> 
> Thank you for the patch.
> 
> On 3/14/19 3:06 PM, Rasmus Villemoes wrote:
>> If userspace doesn't end the input with a newline (which can easily
>> happen if the write happens from a C program that does write(fd,
>> iface, strlen(iface))), we may end up including garbage from a
>> previous, longer value in the device_name. For example
>>
>> # cat device_name
>>
>> # printf 'eth12' > device_name
>> # cat device_name
>> eth12
>> # printf 'eth3' > device_name
>> # cat device_name
>> eth32
>>
> 
> Added tag:
> 
> Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")
> 
> and applied to the fixes-for-5.1-rc3 branch.
> 

You're stripping lines beginning with #. This is the commit in -next:


commit 09466021a80c926aa7de68e5162bdfea2a117483
Author: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date:   Thu Mar 14 15:06:14 2019 +0100

    leds: netdev trigger: use memcpy in device_name_store

    If userspace doesn't end the input with a newline (which can easily
    happen if the write happens from a C program that does write(fd,
    iface, strlen(iface))), we may end up including garbage from a
    previous, longer value in the device_name. For example

    eth12
    eth32

which is entirely useless and confusing. Please fix this before it
actually hits mainline. And you may want to look into "git commit
--cleanup" and/or "git config commit.cleanup" (scissors is much better
than the default strip).

Rasmus

Re: [PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Jacek Anaszewski]

On 3/27/19 4:26 PM, Rasmus Villemoes wrote:
> On 26/03/2019 20.53, Jacek Anaszewski wrote:
>> Hi Rasmus,
>>
>> Thank you for the patch.
>>
>> On 3/14/19 3:06 PM, Rasmus Villemoes wrote:
>>> If userspace doesn't end the input with a newline (which can easily
>>> happen if the write happens from a C program that does write(fd,
>>> iface, strlen(iface))), we may end up including garbage from a
>>> previous, longer value in the device_name. For example
>>>
>>> # cat device_name
>>>
>>> # printf 'eth12' > device_name
>>> # cat device_name
>>> eth12
>>> # printf 'eth3' > device_name
>>> # cat device_name
>>> eth32
>>>
>>
>> Added tag:
>>
>> Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")
>>
>> and applied to the fixes-for-5.1-rc3 branch.
>>
> 
> You're stripping lines beginning with #. This is the commit in -next:
> 
> 
> commit 09466021a80c926aa7de68e5162bdfea2a117483
> Author: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Date:   Thu Mar 14 15:06:14 2019 +0100
> 
>      leds: netdev trigger: use memcpy in device_name_store
> 
>      If userspace doesn't end the input with a newline (which can easily
>      happen if the write happens from a C program that does write(fd,
>      iface, strlen(iface))), we may end up including garbage from a
>      previous, longer value in the device_name. For example
> 
>      eth12
>      eth32
> 
> which is entirely useless and confusing. Please fix this before it
> actually hits mainline. And you may want to look into "git commit
> --cleanup" and/or "git config commit.cleanup" (scissors is much better
> than the default strip).

Thanks for the heads-up. I must admit I'm hitting into that for the
first time. After "git am" it was all OK, but it got screwed up after
"git rebase -i". And having "commit.cleanup = scissors" set globally all
the time is annoying if one extensively uses interactive rebase for
rewording commit messages. It entails the need for manual removal of
the whole stuff that appears then after actual commit message prepended
with "#" comment characters.

This is probably the reason why people use often other characters
for command prompt (see the other fix for ledtrig-netdev). Note that, 
while it is of no so great importance for mainline, when people cherry
pick such patch to their out-of-tree repositories and then rebase, they
hit into the same issue.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Rasmus Villemoes]

On 27/03/2019 22.20, Jacek Anaszewski wrote:

> Thanks for the heads-up. I must admit I'm hitting into that for the
> first time. After "git am" it was all OK, but it got screwed up after
> "git rebase -i". And having "commit.cleanup = scissors" set globally all
> the time is annoying if one extensively uses interactive rebase for
> rewording commit messages. It entails the need for manual removal of
> the whole stuff that appears then after actual commit message prepended
> with "#" comment characters.

Eh, no? At least, whenever I do commit or rebase -i, git automatically
inserts a trailer starting with the magic scissor line

# ------------------------ >8 ------------------------
# Do not modify or remove the line above.
# Everything below it will be ignored.

Maybe there's some other config option to get that, or it depends on git
version. But I certainly don't do anything at all other than write or
modify the commit message.

Never had a problem myself since I set commit.cleanup = scissors, but I
have had lots of my commit messages mangled, which is why I'm reacting.

> This is probably the reason why people use often other characters
> for command prompt (see the other fix for ledtrig-netdev).

Command prompt char is not the only problem; C snippets with #include or
other preprocessor directives also regularly gets mangled, as does shell
snippets with a bit of commentary.

Rasmus

Re: [PATCH v2 1/6] leds: netdev trigger: use memcpy in device_name_store

[Jacek Anaszewski]

On 3/27/19 10:31 PM, Rasmus Villemoes wrote:
> On 27/03/2019 22.20, Jacek Anaszewski wrote:
> 
>> Thanks for the heads-up. I must admit I'm hitting into that for the
>> first time. After "git am" it was all OK, but it got screwed up after
>> "git rebase -i". And having "commit.cleanup = scissors" set globally all
>> the time is annoying if one extensively uses interactive rebase for
>> rewording commit messages. It entails the need for manual removal of
>> the whole stuff that appears then after actual commit message prepended
>> with "#" comment characters.
> 
> Eh, no? At least, whenever I do commit or rebase -i, git automatically
> inserts a trailer starting with the magic scissor line
> 
> # ------------------------ >8 ------------------------
> # Do not modify or remove the line above.
> # Everything below it will be ignored.
> 
> Maybe there's some other config option to get that, or it depends on git
> version. But I certainly don't do anything at all other than write or
> modify the commit message.

I don't have this "#-- >8 --" line. Will have to find out the reason.
I'm using git version 2.11.0 btw.

> Never had a problem myself since I set commit.cleanup = scissors, but I
> have had lots of my commit messages mangled, which is why I'm reacting.
> 
>> This is probably the reason why people use often other characters
>> for command prompt (see the other fix for ledtrig-netdev).
> 
> Command prompt char is not the only problem; C snippets with #include or
> other preprocessor directives also regularly gets mangled, as does shell
> snippets with a bit of commentary.

True. I certainly will have to get my git config fixed.

-- 
Best regards,
Jacek Anaszewski

[PATCH v2 2/6] leds: netdev trigger: factor out middle part of device_name_store

[Rasmus Villemoes]

Take the part of device_name_store that puts the old device (if any),
copies the new device name, looks the name up etc. into a separate
helper function. This is preparation for using that helper from a
function that will initialize the led_netdev_data from a device tree
node. No functional change.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 30 ++++++++++++++++-----------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 8d476b92f58c..21605033e322 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -104,19 +104,9 @@ static ssize_t device_name_show(struct device *dev,
 	return len;
 }
 
-static ssize_t device_name_store(struct device *dev,
-				 struct device_attribute *attr, const char *buf,
-				 size_t size)
+static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
+				   const char *buf, size_t size)
 {
-	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
-
-	if (size >= IFNAMSIZ)
-		return -EINVAL;
-
-	cancel_delayed_work_sync(&trigger_data->work);
-
-	spin_lock_bh(&trigger_data->lock);
-
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
 		trigger_data->net_dev = NULL;
@@ -137,6 +127,22 @@ static ssize_t device_name_store(struct device *dev,
 			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 
 	trigger_data->last_activity = 0;
+}
+
+static ssize_t device_name_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t size)
+{
+	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+
+	if (size >= IFNAMSIZ)
+		return -EINVAL;
+
+	cancel_delayed_work_sync(&trigger_data->work);
+
+	spin_lock_bh(&trigger_data->lock);
+
+	netdev_trig_set_device(trigger_data, buf, size);
 
 	set_baseline_state(trigger_data);
 	spin_unlock_bh(&trigger_data->lock);
-- 
2.20.1

Re: [PATCH v2 2/6] leds: netdev trigger: factor out middle part of device_name_store

[Pavel Machek]

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

Hi!

> Take the part of device_name_store that puts the old device (if any),
> copies the new device name, looks the name up etc. into a separate
> helper function. This is preparation for using that helper from a
> function that will initialize the led_netdev_data from a device tree
> node. No functional change.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 30 ++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 8d476b92f58c..21605033e322 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -104,19 +104,9 @@ static ssize_t device_name_show(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t device_name_store(struct device *dev,
> -				 struct device_attribute *attr, const char *buf,
> -				 size_t size)
> +static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
> +				   const char *buf, size_t size)
>  {
> -	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> -
> -	if (size >= IFNAMSIZ)
> -		return -EINVAL;
> -
> -	cancel_delayed_work_sync(&trigger_data->work);
> -
> -	spin_lock_bh(&trigger_data->lock);
> -
>  	if (trigger_data->net_dev) {
>  		dev_put(trigger_data->net_dev);
>  		trigger_data->net_dev = NULL;

I'd expect the helper function to do the >= IFNAMSIZ checking... but
not a huge deal.

									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 v2 3/6] leds: netdev trigger: move newline handling back to device_name_store

[Rasmus Villemoes]

Currently, setting device_name to a 15-character name requires
avoiding a trailing newline (e.g. by using 'echo -n' or 'printf'),
which is inconsistent and potentially surprising. Instead of
potentially including a newline in the copy and then overwriting it,
move the newline detection logic to the sysfs handler itself, and
handle it by passing a smaller buffer size to
netdev_trig_set_device().

This also makes netdev_trig_set_device() a better fit for a future
user, which will not need the newline truncation logic.

We still have to tell userspace we consumed all the bytes it gave us,
so we have to stash the original value of size.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 21605033e322..c35439291424 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -114,8 +114,6 @@ static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
 
 	memcpy(trigger_data->device_name, buf, size);
 	trigger_data->device_name[size] = 0;
-	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
-		trigger_data->device_name[size - 1] = 0;
 
 	if (trigger_data->device_name[0] != 0)
 		trigger_data->net_dev =
@@ -134,7 +132,11 @@ static ssize_t device_name_store(struct device *dev,
 				 size_t size)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+	size_t orig_size = size;
 
+	/* Ignore trailing newline */
+	if (size > 0 && buf[size - 1] == '\n')
+		size--;
 	if (size >= IFNAMSIZ)
 		return -EINVAL;
 
@@ -147,7 +149,7 @@ static ssize_t device_name_store(struct device *dev,
 	set_baseline_state(trigger_data);
 	spin_unlock_bh(&trigger_data->lock);
 
-	return size;
+	return orig_size;
 }
 
 static DEVICE_ATTR_RW(device_name);
-- 
2.20.1

Re: [PATCH v2 3/6] leds: netdev trigger: move newline handling back to device_name_store

[Pavel Machek]

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

On Thu 2019-03-14 15:06:16, Rasmus Villemoes wrote:
> Currently, setting device_name to a 15-character name requires
> avoiding a trailing newline (e.g. by using 'echo -n' or 'printf'),
> which is inconsistent and potentially surprising. Instead of
> potentially including a newline in the copy and then overwriting it,
> move the newline detection logic to the sysfs handler itself, and
> handle it by passing a smaller buffer size to
> netdev_trig_set_device().
> 
> This also makes netdev_trig_set_device() a better fit for a future
> user, which will not need the newline truncation logic.
> 
> We still have to tell userspace we consumed all the bytes it gave us,
> so we have to stash the original value of size.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 21605033e322..c35439291424 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -114,8 +114,6 @@ static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
>  
>  	memcpy(trigger_data->device_name, buf, size);
>  	trigger_data->device_name[size] = 0;
> -	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
> -		trigger_data->device_name[size - 1] = 0;
>  
>  	if (trigger_data->device_name[0] != 0)
>  		trigger_data->net_dev =
> @@ -134,7 +132,11 @@ static ssize_t device_name_store(struct device *dev,
>  				 size_t size)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> +	size_t orig_size = size;
>  
> +	/* Ignore trailing newline */
> +	if (size > 0 && buf[size - 1] == '\n')
> +		size--;
>  	if (size >= IFNAMSIZ)
>  		return -EINVAL;
>  
> @@ -147,7 +149,7 @@ static ssize_t device_name_store(struct device *dev,
>  	set_baseline_state(trigger_data);
>  	spin_unlock_bh(&trigger_data->lock);
>  
> -	return size;
> +	return orig_size;
>  }
>  
>  static DEVICE_ATTR_RW(device_name);

-- 
(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 v2 4/6] leds: netdev trigger: move name length checking to netdev_trig_set_device

[Rasmus Villemoes]

It's better to check that size is sane in the function that does the
memcpy'ing and 0-termination to the IFNAMSIZ-sized buffer instead of
relying on callers getting it right. Not rejecting size upfront does
mean we would do the cancel_delayed_work_sync(), but that gets fixed
up by the set_baseline_state() call.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/trigger/ledtrig-netdev.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index c35439291424..e4a76ce4e4c7 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -104,9 +104,12 @@ static ssize_t device_name_show(struct device *dev,
 	return len;
 }
 
-static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
-				   const char *buf, size_t size)
+static ssize_t netdev_trig_set_device(struct led_netdev_data *trigger_data,
+				      const char *buf, size_t size)
 {
+	if (size >= IFNAMSIZ)
+		return -EINVAL;
+
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
 		trigger_data->net_dev = NULL;
@@ -125,6 +128,7 @@ static void netdev_trig_set_device(struct led_netdev_data *trigger_data,
 			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 
 	trigger_data->last_activity = 0;
+	return 0;
 }
 
 static ssize_t device_name_store(struct device *dev,
@@ -133,23 +137,22 @@ static ssize_t device_name_store(struct device *dev,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	size_t orig_size = size;
+	ssize_t ret;
 
 	/* Ignore trailing newline */
 	if (size > 0 && buf[size - 1] == '\n')
 		size--;
-	if (size >= IFNAMSIZ)
-		return -EINVAL;
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
 	spin_lock_bh(&trigger_data->lock);
 
-	netdev_trig_set_device(trigger_data, buf, size);
+	ret = netdev_trig_set_device(trigger_data, buf, size);
 
 	set_baseline_state(trigger_data);
 	spin_unlock_bh(&trigger_data->lock);
 
-	return orig_size;
+	return ret ? ret : orig_size;
 }
 
 static DEVICE_ATTR_RW(device_name);
-- 
2.20.1

Re: [PATCH v2 4/6] leds: netdev trigger: move name length checking to netdev_trig_set_device

[Pavel Machek]

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

On Thu 2019-03-14 15:06:17, Rasmus Villemoes wrote:
> It's better to check that size is sane in the function that does the
> memcpy'ing and 0-termination to the IFNAMSIZ-sized buffer instead of
> relying on callers getting it right. Not rejecting size upfront does
> mean we would do the cancel_delayed_work_sync(), but that gets fixed
> up by the set_baseline_state() call.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Exactly. 2,4:

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 v2 5/6] leds: netdev trigger: add documentation to leds/common.txt

[Rasmus Villemoes]

The various sysfs files used to configure a netdev-triggered LED are
already documented, but let's also add "netdev" to the list of things
one can reasonably set linux,default-trigger to.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/leds/common.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..7cb88460a47c 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -39,6 +39,9 @@ Optional properties for child nodes:
      "timer" - LED flashes at a fixed, configurable rate
      "pattern" - LED alters the brightness for the specified duration with one
                  software timer (requires "led-pattern" property)
+     "netdev" - LED can be configured via sysfs files (see
+                Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
+                to reflect the state and activity of a net device.
 
 - led-pattern : Array of integers with default pattern for certain triggers.
                 Each trigger may parse this property differently:
-- 
2.20.1

[PATCH v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

It can be quite convenient to initialize a netdev-triggered LED with a
device name and setting the rx,tx,link properties from device tree,
instead of having to do that in an init script (or udev rule) in
userspace.

My main motivation for this is to be able to switch away from the
deprecated CONFIG_CAN_LEDS. The example added to common.txt
corresponds to switching linux,default-trigger = "can0-rxtx" to
"netdev" and adding the indicated netdev subnode.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/leds/common.txt       | 11 +++++++
 drivers/leds/trigger/ledtrig-netdev.c         | 30 +++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 7cb88460a47c..f8078c4cf6f8 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -43,6 +43,17 @@ Optional properties for child nodes:
                 Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
                 to reflect the state and activity of a net device.
 
+                The optional child node netdev can be used to
+                configure initial values for the link, rx, tx and
+                device_name properties. For example,
+
+                netdev {
+                    rx;
+                    tx;
+                    link;
+                    device-name = "can0";
+                };
+
 - led-pattern : Array of integers with default pattern for certain triggers.
                 Each trigger may parse this property differently:
                 - one-shot : two numbers specifying delay on and delay off (in ms),
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index e4a76ce4e4c7..387932cfea55 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
@@ -400,6 +401,33 @@ static void netdev_trig_work(struct work_struct *work)
 			(atomic_read(&trigger_data->interval)*2));
 }
 
+static void netdev_trig_of_init(struct led_classdev *led_cdev,
+				struct led_netdev_data *trigger_data)
+{
+	struct device_node *np = led_cdev->dev->of_node;
+	const char *device_name;
+
+	if (!np)
+		return;
+	np = of_get_child_by_name(np, "netdev");
+	if (!np)
+		return;
+
+	if (of_property_read_bool(np, "link"))
+		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
+	if (of_property_read_bool(np, "tx"))
+		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
+	if (of_property_read_bool(np, "rx"))
+		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
+	if (!of_property_read_string(np, "device-name", &device_name))
+		netdev_trig_set_device(trigger_data, device_name,
+				       strlen(device_name));
+
+	set_baseline_state(trigger_data);
+
+	of_node_put(np);
+}
+
 static int netdev_trig_activate(struct led_classdev *led_cdev)
 {
 	struct led_netdev_data *trigger_data;
@@ -423,6 +451,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	trigger_data->mode = 0;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
 	trigger_data->last_activity = 0;
+	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER)
+		netdev_trig_of_init(led_cdev, trigger_data);
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
-- 
2.20.1

Re: [PATCH v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Jacek Anaszewski]

Rasmus,

Thank you for the update.
Still, there is one thing to improve.

On 3/14/19 3:06 PM, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script (or udev rule) in
> userspace.
> 
> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS. The example added to common.txt
> corresponds to switching linux,default-trigger = "can0-rxtx" to
> "netdev" and adding the indicated netdev subnode.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   .../devicetree/bindings/leds/common.txt       | 11 +++++++
>   drivers/leds/trigger/ledtrig-netdev.c         | 30 +++++++++++++++++++
>   2 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..f8078c4cf6f8 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,17 @@ Optional properties for child nodes:
>                   Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                   to reflect the state and activity of a net device.
>   
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example,
> +
> +                netdev {
> +                    rx;
> +                    tx;
> +                    link;
> +                    device-name = "can0";
> +                };
> +
>   - led-pattern : Array of integers with default pattern for certain triggers.
>                   Each trigger may parse this property differently:
>                   - one-shot : two numbers specifying delay on and delay off (in ms),
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index e4a76ce4e4c7..387932cfea55 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -20,6 +20,7 @@
>   #include <linux/list.h>
>   #include <linux/module.h>
>   #include <linux/netdevice.h>
> +#include <linux/of.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include "../leds.h"
> @@ -400,6 +401,33 @@ static void netdev_trig_work(struct work_struct *work)
>   			(atomic_read(&trigger_data->interval)*2));
>   }
>   
> +static void netdev_trig_of_init(struct led_classdev *led_cdev,
> +				struct led_netdev_data *trigger_data)
> +{
> +	struct device_node *np = led_cdev->dev->of_node;
> +	const char *device_name;
> +
> +	if (!np)
> +		return;
> +	np = of_get_child_by_name(np, "netdev");
> +	if (!np)
> +		return;
> +
> +	if (of_property_read_bool(np, "link"))
> +		__set_bit(NETDEV_LED_LINK, &trigger_data->mode);
> +	if (of_property_read_bool(np, "tx"))
> +		__set_bit(NETDEV_LED_TX, &trigger_data->mode);
> +	if (of_property_read_bool(np, "rx"))
> +		__set_bit(NETDEV_LED_RX, &trigger_data->mode);
> +	if (!of_property_read_string(np, "device-name", &device_name))
> +		netdev_trig_set_device(trigger_data, device_name,
> +				       strlen(device_name));
> +
> +	set_baseline_state(trigger_data);
> +
> +	of_node_put(np);
> +}
> +
>   static int netdev_trig_activate(struct led_classdev *led_cdev)
>   {
>   	struct led_netdev_data *trigger_data;
> @@ -423,6 +451,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>   	trigger_data->mode = 0;
>   	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>   	trigger_data->last_activity = 0;
> +	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER)
> +		netdev_trig_of_init(led_cdev, trigger_data);

We don't necessarily want OF settings to be used for initialization on
each LED trigger activation for the LED class device, but only on the
first one. That's why the triggers using this flags clean it here:

led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;

>   
>   	led_set_trigger_data(led_cdev, trigger_data);
>   
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Rasmus Villemoes]

On 14/03/2019 15.24, Jacek Anaszewski wrote:
> Rasmus,
> 
> Thank you for the update.
> Still, there is one thing to improve.
> 

>>   static int netdev_trig_activate(struct led_classdev *led_cdev)
>>   {
>>       struct led_netdev_data *trigger_data;
>> @@ -423,6 +451,8 @@ static int netdev_trig_activate(struct
>> led_classdev *led_cdev)
>>       trigger_data->mode = 0;
>>       atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>>       trigger_data->last_activity = 0;
>> +    if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER)
>> +        netdev_trig_of_init(led_cdev, trigger_data);
> 
> We don't necessarily want OF settings to be used for initialization on
> each LED trigger activation for the LED class device, but only on the
> first one. That's why the triggers using this flags clean it here:
> 
> led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;

Right, I noticed that pattern, but wondered about it. If this is
supposed to be a general thing, why isn't it just done by the trigger
core after the call of led_trigger_set() in the two places the bit gets
set? I.e.

		led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
		led_trigger_set(led_cdev, trig);
+		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;

I can certainly add clearing the flag to my code, just wondering.

Rasmus

Re: [PATCH v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Jacek Anaszewski]

On 3/14/19 4:05 PM, Rasmus Villemoes wrote:
> On 14/03/2019 15.24, Jacek Anaszewski wrote:
>> Rasmus,
>>
>> Thank you for the update.
>> Still, there is one thing to improve.
>>
> 
>>>    static int netdev_trig_activate(struct led_classdev *led_cdev)
>>>    {
>>>        struct led_netdev_data *trigger_data;
>>> @@ -423,6 +451,8 @@ static int netdev_trig_activate(struct
>>> led_classdev *led_cdev)
>>>        trigger_data->mode = 0;
>>>        atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>>>        trigger_data->last_activity = 0;
>>> +    if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER)
>>> +        netdev_trig_of_init(led_cdev, trigger_data);
>>
>> We don't necessarily want OF settings to be used for initialization on
>> each LED trigger activation for the LED class device, but only on the
>> first one. That's why the triggers using this flags clean it here:
>>
>> led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
> 
> Right, I noticed that pattern, but wondered about it. If this is
> supposed to be a general thing, why isn't it just done by the trigger
> core after the call of led_trigger_set() in the two places the bit gets
> set? I.e.
> 
> 		led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> 		led_trigger_set(led_cdev, trig);
> +		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
> 
> I can certainly add clearing the flag to my code, just wondering.

Good point. It would have to be applied in both
led_trigger_set_default() and led_trigger_register(). So you either
need to add generic support for clearing the flag (and update the three
users) or update your patch alone.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Pavel Machek]

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

On Thu 2019-03-14 15:06:19, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script (or udev rule) in
> userspace.
> 
> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS. The example added to common.txt
> corresponds to switching linux,default-trigger = "can0-rxtx" to
> "netdev" and adding the indicated netdev subnode.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/leds/common.txt       | 11 +++++++
>  drivers/leds/trigger/ledtrig-netdev.c         | 30 +++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..f8078c4cf6f8 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,17 @@ Optional properties for child nodes:
>                  Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                  to reflect the state and activity of a net device.
>  
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example,
> +
> +                netdev {
> +                    rx;
> +                    tx;
> +                    link;
> +                    device-name = "can0";

Well, NAK, as explained before. Use phandle.

Alternatively, adding comment why CONFIG_CAN_LEDS can not be removed
just yet near the deprecation note would be acceptable.

									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 v2 6/6] leds: netdev trigger: allow setting initial values in device tree

[Rob Herring]

On Thu, Mar 14, 2019 at 03:06:19PM +0100, Rasmus Villemoes wrote:
> It can be quite convenient to initialize a netdev-triggered LED with a
> device name and setting the rx,tx,link properties from device tree,
> instead of having to do that in an init script (or udev rule) in
> userspace.
> 
> My main motivation for this is to be able to switch away from the
> deprecated CONFIG_CAN_LEDS. The example added to common.txt
> corresponds to switching linux,default-trigger = "can0-rxtx" to
> "netdev" and adding the indicated netdev subnode.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/leds/common.txt       | 11 +++++++
>  drivers/leds/trigger/ledtrig-netdev.c         | 30 +++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 7cb88460a47c..f8078c4cf6f8 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -43,6 +43,17 @@ Optional properties for child nodes:
>                  Documentation/ABI/testing/sysfs-class-led-trigger-netdev)
>                  to reflect the state and activity of a net device.
>  
> +                The optional child node netdev can be used to
> +                configure initial values for the link, rx, tx and
> +                device_name properties. For example,
> +
> +                netdev {

I was going to let the use of netdev slide, but now you have it 
twice and that's a Linuxism.

> +                    rx;
> +                    tx;
> +                    link;

What do these mean?

> +                    device-name = "can0";

can0 is a linux thing and doesn't belong in DT.

> +                };

I really don't like this. What's this going to look like if every 
trigger wants to add something like this? What's special about netdev 
leds?

We really need to decouple the Linux LED trigger design from the DT 
bindings. I have no problem associating LEDs with devices in DT, but it 
shouldn't just bring the LED trigger design into the bindings.