[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zz1tOxW6PO_2OeSA@chrisdown.name>
Date: Wed, 20 Nov 2024 05:01:47 +0000
From: Chris Down <chris@...isdown.name>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
John Ogness <john.ogness@...utronix.de>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Tony Lindgren <tony.lindgren@...ux.intel.com>, kernel-team@...com
Subject: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for
per-console loglevels
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@...isdown.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 :(
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.
Powered by blists - more mailing lists