From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Vincent Whitchurch <vincent.whitchurch@axis.com>,
        Michael Thalmeier <michael.thalmeier@hale.at>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] tty: ttynull: implement console write
In-Reply-To: <Y+0EnQ/Ujtq+nEFf@alley>
References: <20230214115921.399608-1-michael.thalmeier@hale.at>
 <Y+zEAA1hp+3guGxT@axis.com> <Y+ztReOGJwAbpv52@alley>
 <87y1oz7y9s.fsf@jogness.linutronix.de> <Y+0EnQ/Ujtq+nEFf@alley>
Date:   Wed, 15 Feb 2023 23:12:52 +0106
Message-ID: <87bklu37nn.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2023-02-15, Petr Mladek <pmladek@suse.com> wrote:
> I agree that the current code is not that bad. But still, the call
> chain is:
>
>   + console_flush_all()
>     + console_emit_next_record()
>       + call_console_driver()
>         + con->write()

Currently in linux-next it is:

   + console_flush_all()
     + console_emit_next_record()
       + con->write()

> I could imagine another code that would call call_console_driver()
> or console_emit_next_record() directly. We might just hope that
> it would check console_is_usable() or con->write pointer before.

It needs to call console_is_usable() for many reasons, not just the NULL
write() pointer check. call_console_driver() is already gone in
linux-next and calling console_emit_next_record() without first checking
if the console is allowed to print would be a clear bug. Perhaps we
should add kerneldoc to console_emit_next_record() mentioning that the
function assumes it is allowed to call con->write() (based on the many
checks in console_is_usable()).

The check for console usability was left outside of
console_emit_next_record() because that is important information needed
by console_flush_all(), not by console_emit_next_record(). In this case
console_is_usable() is being called not only to know if it can call
console_emit_next_record(), but also to know if any usable console
exists at all.

(Perhaps you want ttynull to be considered "usable"? But even if the
answer to that is "yes", then I would move the NULL check out of
console_is_usable() and into console_emit_next_record() so that it
reports success without performing any string-printing.)

> Also, as you say, con->write is immutable. All real consoles have it
> defined. It does not make sense to check it again and again.
> I would leave console_is_usable() for checks that might really
> change at runtime.

Checking a NULL pointer is a lot cheaper than string-printing a console
message to a buffer to send to a dummy function that does not even use
it.

> IMHO, normal console drivers would be useless without write()
> callback.

When atomic consoles are introduced there will be a second
write_atomic() function. Indeed, in the proposed code it is allowed for
either to be NULL because a console driver might be atomic-only or not
support atomic.

> It sounds perfectly fine to reject useless console
> drivers at all. And it sounds like a reasonable check
> in register_console() that would reject bad console drivers.

It is common practice in most subsystems for callback functions that are
not implemented to be initialized to NULL. ttynull shows that there are
valid use cases. With atomic consoles there may be more. I would be more
inclined to NACK a driver with a dummy write()/write_atomic() callback
because it is misleading the subsystem and wasting real CPU cycles.

John Ogness