linux-kernel - Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <844j4lepak.fsf@jogness.linutronix.de>
Date: Tue, 05 Nov 2024 15:25:31 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Jocelyn Falempe <jfalempe@...hat.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, Javier Martinez Canillas
 <javierm@...hat.com>, "Guilherme G . Piccoli" <gpiccoli@...lia.com>,
 bluescreen_avenger@...izon.net, Caleb Connolly
 <caleb.connolly@...aro.org>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

On 2024-11-05, Petr Mladek <pmladek@...e.com> wrote:
> Observation:
>
>   + CON_ENABLED is not needed for the original purpose. Only enabled
>     consoles are added into @console_list.
>
>   + CON_ENABLED is still used to explicitely block the console driver
>     during suspend by console_stop()/console_start() in serial_core.c.
>
>     It is not bad. But it is a bit confusing because we have
>     CON_SUSPENDED flag now and this is about suspend/resume.

Also note that CON_ENABLED is used to gate ->unblank(). It should
probably consider CON_SUSPENDED as well.

>   + CON_SUSPENDED is per-console flag but it is set synchronously
>     for all consoles.
>
>     IMHO, a global variable would make more sense for the big hammer
>     purpose.
>
>
> Big question:
>
>   Does the driver really needs to call console_stop()/console_start()
>   when there is the big hammer?
>
>   I would preserve it because it makes the design more robust.

Agreed. They serve different purposes.

console_stop()/console_start() is a method for _drivers_ to communicate
that they must not be called because their hardware is not
available/functioning.

console_suspend()/console_resume() is a method for the _system_ to
communicate that consoles should be silent because they are annoying or
we do not trust that they won't cause problems.

>   Anyway, the driver-specific handling looks like the right solution.
>   The big hammer feels like a workaround.

Agreed. Do the 6 call sites even really need the big hammer? I am
guessing yes because there are probably console drivers that do not use
console_stop()/console_start() in their suspend/resume and thus rely on
the whole subsystem being disabled.

> Reasonable semantic:
>
>   1. Rename:
>
> 	console_suspend() -> console_suspend_all()
> 	console_resume()  -> console_resume_all()
>
>      and manipulate a global @consoles_suspended variable agagin.
>      It is the big hammer API.

Agreed. As a global variable, it can still rely on SRCU for
synchronization.

>   2. Rename:
>
> 	console_stop(con)  -> console_suspend(con)
> 	console_start(con) -> console_resume(con)
>
>      and manipulare the per-console CON_SUSPENDED flag here.

Agreed.

>    3. Get rid of the ambiguous CON_ENABLED flag. It won't longer
>       have any purpose.
>
>       Except that it is also used to force console registration.
>       But it can be done a better way, e.g. by introducing
>       register_console_force() API.

Agreed.

John