Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Chris Down]

Thanks for looking this over :-) All not mentioned points in this reply are 
acked.

Greg Kroah-Hartman writes:
> > diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
> > new file mode 100644
> > index 000000000000..40b90b190af3
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-console
> > @@ -0,0 +1,47 @@
> > +What:		/sys/class/console/
> > +Date:		October 2024
>
> It's no longer October 2024 :(

What would you recommend? When I sent them it was, and it doesn't seem 
realistic to think that it's going to be less than one month from me sending 
the patches to when it gets merged, no?

> > +What:		/sys/class/console/<C>/loglevel
> > +Date:		October 2024
> > +Contact:	Chris Down <chris@xxxxxxxxxxxxxx>
> > +Description:	Read write. The current per-console loglevel, which will take
> > +		effect if not overridden by other non-sysfs controls (see
> > +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> > +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
> > +		takes the special value "-1" to indicate that no per-console
> > +		loglevel is set, and we should defer to the global controls.
>
> -1 is odd, why?  That's going to confuse everyone :(

I originally had it that you had to send "unset" instead of -1, but in 
discussion with Petr it was suggested to change it to -1.

Petr, what do you think?

> > +	if (console->classdev)
> > +		device_unregister(console->classdev);
>
> How could this be NULL?

I think it's from an earlier version of the patch where we would still continue 
setup even if we couldn't allocate it. I'm okay removing it.

> > +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
>
> While I admire the use of READ_ONCE() properly, it really doesn't matter
> for sysfs as it could change right afterwards and no one cares.  So no
> need for that here, right?

I'm not sure I understand, could you clarify? From my reading of the code it 
looks like we need this to avoid tearing.