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

[Petr Mladek <pmladek@suse.com>]

On Fri 2024-11-15 05:20:42, Greg Kroah-Hartman wrote:
> On Mon, Oct 28, 2024 at 04:45:46PM +0000, Chris Down wrote:
> > A sysfs interface under /sys/class/console/ is created that permits
> > viewing and configuring per-console attributes. This is the main
> > interface with which we expect users to interact with and configure
> > per-console loglevels.
> > 
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-console
> > @@ -0,0 +1,47 @@
> > +What:		/sys/class/console/<C>/loglevel
> > +Date:		October 2024
> > +Contact:	Chris Down <chris@chrisdown.name>
> > +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 :(

IMHO, it is better than (0) because people might think that "0"
disables all messages or allows just "LOGLEVEL_EMERG".

On the other hand, (-1) is being used for default, undefined, or
unknown values in various situations. For example, see

     git grep "define.*(-1[^0-9]" include/linux/

> > --- /dev/null
> > +++ b/kernel/printk/sysfs.c
> > +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?

READ_ONCE() prevents compiler optimizations. It makes sure that
the value will be read using a single read operation. It might
be outdated but it will be consistent. I believe that READ_ONCE()
should stay.


> > +}
> > +
> > +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> > +			      const char *buf, size_t size)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +	ssize_t ret;
> > +	int level;
> > +
> > +	ret = kstrtoint(buf, 10, &level);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (level == -1)
> > +		goto out;
> 
> As I said above, -1 is an odd thing here, why use it?
> 
> > +
> > +	if (clamp_loglevel(level) != level)
> > +		return -ERANGE;
> > +
> > +out:
> > +	WRITE_ONCE(con->level, level);
> 
> Same here, does this matter?

Same here. I believe that WRITE_ONCE() should stay to guarantee an atomic write.

> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RW(loglevel);
> > +
> > +static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +	int cookie;
> > +	bool enabled;
> > +
> > +	cookie = console_srcu_read_lock();
> > +	enabled = console_srcu_read_flags(con) & CON_ENABLED;
> > +	console_srcu_read_unlock(cookie);
> 
> As the values can change right after reading, do you really need to
> worry about any read locks here?

It is true that the related struct console could not disappear
when this sysfs interface exists. Also it is true that
the read_lock does not prevent any race here.

A plain read is OK.

That said, I suggest to remove this sysfs interface anyway because
the CON_ENABLED flag semantic is bogus. See
https://lore.kernel.org/r/ZzTMrFEcYZf58aqj@pathway.suse.cz and
https://lore.kernel.org/r/ZyoNZfLT6tlVAWjO@pathway.suse.cz


> > +	return sysfs_emit(buf, "%d\n", enabled);
> > +}
> > +
> > +static DEVICE_ATTR_RO(enabled);
> > +
> > +static struct attribute *console_sysfs_attrs[] = {
> > +	&dev_attr_loglevel.attr,
> > +	&dev_attr_effective_loglevel_source.attr,
> > +	&dev_attr_effective_loglevel.attr,
> > +	&dev_attr_enabled.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(console_sysfs);
> > +
> > +static void console_classdev_release(struct device *dev)
> > +{
> > +	kfree(dev);
> > +}
> > +
> > +void console_register_device(struct console *con)
> > +{
> > +	/*
> > +	 * We might be called from register_console() before the class is
> > +	 * registered. If that happens, we'll take care of it in
> > +	 * printk_late_init.
> > +	 */
> > +	if (IS_ERR_OR_NULL(console_class))
> 
> When you change this to be a constant above, this isn't going to be
> needed.
> 
> > +		return;
> > +
> > +	if (WARN_ON(con->classdev))
> > +		return;
> 
> How can this ever happen?
> 
> > +
> > +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > +	if (!con->classdev)
> > +		return;
>
> No error value returned?

Good question.

IMHO, a missing sysfs interface should not be a fatal error
because it might prevent debugging bugs in the sysfs/kobject APIs.
I mean that an error here should not block register_console().

But it is true that we should not ignore this quietly.
We should print an error message at least.

Another question is why is the struct device allocated dynamically?

I guess that it is a memory optimization because struct console
is static. I am not sure if it is worth it. We could always make
it dynamic when people complain.

Best Regards,
Petr