Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1484140 > unrolled thread

Re: [PATCH v2] leds: Introduce userspace leds driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-15 15:10 +0200
    Re: [PATCH v2] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-15 17:00 +0200
      Re: [PATCH v2] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-15 17:30 +0200
        Re: [PATCH v2] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-16 09:10 +0200
      Re: [PATCH v2] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-15 18:20 +0200

#1484140 — Re: [PATCH v2] leds: Introduce userspace leds driver

Hi!

> >@@ -0,0 +1,23 @@
> >+/*
> >+ * Userspace driver support for leds subsystem
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License as published by
> >+ * the Free Software Foundation; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ */
> >+#ifndef _UAPI__ULEDS_H_
> >+#define _UAPI__ULEDS_H_
> >+
> >+#define ULEDS_MAX_NAME_SIZE	80
> >+
> >+struct uleds_user_dev {
> >+	char name[ULEDS_MAX_NAME_SIZE];
> >+};

We already have path component length limit somewhere, right? Just use
it?

(And is struct with char array good idea at all? Perphaps it can just
use write() length up to something reasonable, and not bother with new
header file for userspace?)

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

[toc] | [next] | [standalone]

#1484268

Hi Pavel,

On 09/15/2016 03:01 PM, Pavel Machek wrote:
> Hi!
>
>>> @@ -0,0 +1,23 @@
>>> +/*
>>> + * Userspace driver support for leds subsystem
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#ifndef _UAPI__ULEDS_H_
>>> +#define _UAPI__ULEDS_H_
>>> +
>>> +#define ULEDS_MAX_NAME_SIZE	80
>>> +
>>> +struct uleds_user_dev {
>>> +	char name[ULEDS_MAX_NAME_SIZE];
>>> +};
>
> We already have path component length limit somewhere, right? Just use
> it?
>
> (And is struct with char array good idea at all? Perphaps it can just
> use write() length up to something reasonable, and not bother with new
> header file for userspace?)

In fact in this case the addition of another public header can be
avoided.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1484314

On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:
> Hi Pavel,
>
> On 09/15/2016 03:01 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> @@ -0,0 +1,23 @@
>>>> +/*
>>>> + * Userspace driver support for leds subsystem
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify
>>>> + * it under the terms of the GNU General Public License as
>>>> published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +#ifndef _UAPI__ULEDS_H_
>>>> +#define _UAPI__ULEDS_H_
>>>> +
>>>> +#define ULEDS_MAX_NAME_SIZE    80
>>>> +
>>>> +struct uleds_user_dev {
>>>> +    char name[ULEDS_MAX_NAME_SIZE];
>>>> +};
>>
>> We already have path component length limit somewhere, right? Just use
>> it?
>>
>> (And is struct with char array good idea at all? Perphaps it can just
>> use write() length up to something reasonable, and not bother with new
>> header file for userspace?)
>
> In fact in this case the addition of another public header can be
> avoided.
>

The main reason I did it this way is in case someone wants to extend 
this to also, for example, set the max_brightness value. If we use an 
arbitrary size string, we could never add max_brightness without 
breaking userspace.

If we are sure we will never want to pass any other parameters other 
than name, then we can do away with the struct.

[toc] | [prev] | [next] | [standalone]

#1484711

Hi David,

On 09/15/2016 05:25 PM, David Lechner wrote:
> On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:
>> Hi Pavel,
>>
>> On 09/15/2016 03:01 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> @@ -0,0 +1,23 @@
>>>>> +/*
>>>>> + * Userspace driver support for leds subsystem
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> modify
>>>>> + * it under the terms of the GNU General Public License as
>>>>> published by
>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +#ifndef _UAPI__ULEDS_H_
>>>>> +#define _UAPI__ULEDS_H_
>>>>> +
>>>>> +#define ULEDS_MAX_NAME_SIZE    80
>>>>> +
>>>>> +struct uleds_user_dev {
>>>>> +    char name[ULEDS_MAX_NAME_SIZE];
>>>>> +};
>>>
>>> We already have path component length limit somewhere, right? Just use
>>> it?
>>>
>>> (And is struct with char array good idea at all? Perphaps it can just
>>> use write() length up to something reasonable, and not bother with new
>>> header file for userspace?)
>>
>> In fact in this case the addition of another public header can be
>> avoided.
>>
>
> The main reason I did it this way is in case someone wants to extend
> this to also, for example, set the max_brightness value. If we use an
> arbitrary size string, we could never add max_brightness without
> breaking userspace.
>
> If we are sure we will never want to pass any other parameters other
> than name, then we can do away with the struct.

This is sound argument. Let's limit the name size to 64, as in case
of name variable in led_classdev_register(). This patch could also
add include directive "#include <uapi/leds/uleds.h>" to
drivers/leds/led-class.c and replace 64 with a new LED_MAX_NAME_LEN 
macro defined in the uleds.h header.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1484381

On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:
> Hi Pavel,
>
> On 09/15/2016 03:01 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> @@ -0,0 +1,23 @@
>>>> +/*
>>>> + * Userspace driver support for leds subsystem
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify
>>>> + * it under the terms of the GNU General Public License as
>>>> published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +#ifndef _UAPI__ULEDS_H_
>>>> +#define _UAPI__ULEDS_H_
>>>> +
>>>> +#define ULEDS_MAX_NAME_SIZE    80
>>>> +
>>>> +struct uleds_user_dev {
>>>> +    char name[ULEDS_MAX_NAME_SIZE];
>>>> +};
>>
>> We already have path component length limit somewhere, right? Just use
>> it?

As a matter of fact, in led_classdev_register(), the name is limited to 
64. This is hard-coded (no #define).

      char name[64];

The given name is truncated to this length in a call to 
led_classdev_next_name().

>>
>> (And is struct with char array good idea at all? Perphaps it can just
>> use write() length up to something reasonable, and not bother with new
>> header file for userspace?)
>
> In fact in this case the addition of another public header can be
> avoided.
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web