From: Kroah-Hartman <gregkh@linuxfoundation.org>
To: Petr Mladek <pmladek@suse.com>
Cc: Calvin Owens <calvinowens@fb.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
linux-api@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 2/3] printk: Add /sys/consoles/ interface
Date: Fri, 3 Nov 2017 16:58:00 +0100 [thread overview]
Message-ID: <20171103155800.GA15085@kroah.com> (raw)
In-Reply-To: <20171103154651.GJ20040@pathway.suse.cz>
On Fri, Nov 03, 2017 at 04:46:51PM +0100, Petr Mladek wrote:
> On Fri 2017-11-03 15:32:34, Kroah-Hartman wrote:
> > > > diff --git a/Documentation/ABI/testing/sysfs-consoles b/Documentation/ABI/testing/sysfs-consoles
> > > > new file mode 100644
> > > > index 0000000..6a1593e
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-consoles
> > > > @@ -0,0 +1,13 @@
> > > > +What: /sys/consoles/
> >
> > Eeek, what!
> >
> > > I rather add Greg in CC. I am not 100% sure that the top level
> > > directory is the right thing to do.
> >
> > Neither do I.
> >
> > > Alternative might be to hide this under /sys/kernel/consoles/.
> >
> > No no no.
> >
> > > > diff --git a/include/linux/console.h b/include/linux/console.h
> > > > index a5b5d79..76840be 100644
> > > > --- a/include/linux/console.h
> > > > +++ b/include/linux/console.h
> > > > @@ -148,6 +148,7 @@ struct console {
> > > > void *data;
> > > > struct console *next;
> > > > int level;
> > > > + struct kobject *kobj;
> >
> > Why are you using "raw" kobjects and not a "real" struct device? This
> > is a device, use that interface instead please.
>
> Hmm, struct console has a member
>
> struct tty_driver *(*device)(struct console *, int *);
>
> but it is set only when the console has tty binding.
I don't understand what you are referring to here, sorry.
> > If you need a console 'bus' to place them on, fine, but the virtual bus
> > is probably best and simpler to use.
> >
> > That is if you _really_ feel you need sysfs interaction with the console
> > layer (hint, I am not yet convinced...)
>
> The purpose of this patch is to make a userfriendly interface
> for setting console-specific loglevel (message filtering).
>
> It curretnly uses kobject for creating a simple directory
> structure under /sys/. It is inspired by /sys/power/, /sys/kernel/mm/,
> /sys/kernel/debug/tracing/ stuff.
>
> There are ideas to add more files that would allow to modify even the
> global setting. This is currectly possible by the four numbers in
> /proc/sys/kernel/printk. Nobody knows what the four numbers mean.
> IMHO, the following interface would be easier to work with:
>
> /sys/console/loglevel
> /sys/console/min_loglevel
> /sys/condole/default_loglevel
No, please do not polute the /sys/* namespace with stuff like this. If
this is associated with a specific device, hang the sysfs files off of
it. Your console is in the /sys/device/ tree, right?
> > > /*
> > > * Find the related struct console a safe way. The kobject
> > > * desctruction is asynchronous.
> > > */
> > > > + console_lock();
> > > > + for_each_console(con) {
> > > > + if (con->kobj == kobj) {
> >
> > You are doing something wrong, go from kobj to your console directly,
> > the fact that you can not do that here is a _huge_ hint that your
> > structure is not correct.
> >
> > Hint, it's not correct at all :)
>
> I know that we are not following the original purpose of sysfs.
> But there are more (mis)users.
Sure, and I will gladly work to fix those. But do not add known-broken
code to the tree :)
thanks,
greg k-h
next prev parent reply other threads:[~2017-11-03 15:57 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-29 0:43 [PATCH 1/3] printk: Introduce per-console loglevel setting Calvin Owens
2017-09-29 0:43 ` [PATCH 2/3] printk: Add /sys/consoles/ interface Calvin Owens
2017-11-03 14:21 ` Petr Mladek
2017-11-03 14:32 ` Kroah-Hartman
2017-11-03 15:46 ` Petr Mladek
2017-11-03 15:58 ` Kroah-Hartman [this message]
2017-11-08 21:32 ` Calvin Owens
2017-11-09 8:03 ` Kroah-Hartman
2017-09-29 0:43 ` [PATCH 3/3] printk: Add ability to set loglevel via "console=" cmdline Calvin Owens
2017-11-03 15:15 ` Petr Mladek
2017-10-19 23:40 ` [PATCH 1/3] printk: Introduce per-console loglevel setting Calvin Owens
2017-10-20 8:05 ` Petr Mladek
2017-10-20 17:33 ` Calvin Owens
2017-11-03 12:00 ` Petr Mladek
2017-11-03 13:41 ` Steven Rostedt
2018-10-19 0:04 ` Sergey Senozhatsky
2018-10-19 22:03 ` Calvin Owens
2018-10-22 2:37 ` Sergey Senozhatsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171103155800.GA15085@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=calvinowens@fb.com \
--cc=kernel-team@fb.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).