Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Greg Kroah-Hartman]

On Mon, Mar 29, 2021 at 04:01:18PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 28, 2021 at 03:02:24PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> > > On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > > > It's very convenient to see what properties and their values
> > > > > are currently being assigned in the registered software nodes.
> > > > >
> > > > > Show properties and their values in sysfs.
> > > 
> > > ...
> > > 
> > > > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> > > >
> > > > No, sysfs is "one value per file", and that is not what you are showing
> > > > here at all :(
> > > 
> > > It is following: it's a "one value" for property in question,
> > > 
> > > As we may read in [1]: "...so it is socially acceptable to express an
> > > array of values of the same type."
> > > 
> > > And here is exactly the case: *values of the same type*.
> > 
> > So what is it going to look like exactly?
> 
> Basically we have two approaches (already done in the kernel!) use space or
> comma for a separator. So:
>  - for boolean it will be an empty string (and it's one value always)
>  - for integers it will be, for example, '0,1,2' (w/o single quotes)
>    for property array with values 0, 1, and 2
>  - for plain integers or arrays out of 1 element it will be plain integer
>  - for strings it will be, for example, '"str1","str2"' (w/o single quotes)
>    for array of string { "str1", "str2" }
>  - for single string or array out of 1 element, it will be '"str"' (w/o single
>    quotes)
> 
> This should be a part of documentation.

And I will complain then too, these "lists of values" are not for sysfs,
sorry.

> > And what tool is going to be
> > there to parse this mess?  Who is going to to use it?
> 
> I guess something like hwinfo (needs a patch).

If nothing needs this, then why are you adding these?

> The idea behind that this is following what ACPI and DT provides to the users
> via /sys/firmware/ (however, in binary format). I can re-do to provide a
> binary, and it will effectively make software nodes in align with the rest.

binary files in sysfs are only to be used as a "pass through" from
hardware to userspace.  That does not seem relevant here.

sorry, please keep this out of sysfs for now.

greg k-h