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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZyT7MScAsHxkACfD@pathway.suse.cz>
Date: Fri, 1 Nov 2024 17:00:49 +0100
From: Petr Mladek <pmladek@...e.com>
To: Jocelyn Falempe <jfalempe@...hat.com>
Cc: 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>,
	John Ogness <john.ogness@...utronix.de>,
	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 Fri 2024-10-25 11:46:16, Jocelyn Falempe wrote:
> On 25/10/2024 01:11, Jocelyn Falempe wrote:
> > On 24/10/2024 16:34, Petr Mladek wrote:
> > > On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
> > > > The console is already suspended in printk.c.
> > > 
> > > Does this mean that drm_log_client_suspend() is called
> > > after suspend_console(), please?
> > 
> > To be honest, I wasn't able to tell which one is called first, and if
> > the order is enforced (I didn't check if drivers can be suspended in
> > parallel, or if it's all sequential)..
> > 
> > I then checked if it's possible to suspend the console, but didn't found
> > an easy API to do so, so I went with this lazy patch, just ensuring
> > we're not writing to a suspended graphic driver.
> 
> I've run some tests on my hardware, and the console is suspended before the
> graphic driver:
> 
> [   56.409604] printk: Suspending console(s) (use no_console_suspend to
> debug)
> [   56.411430] serial 00:05: disabled
> [   56.421877] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [   56.421954] sd 4:0:0:0: [sdb] Synchronizing SCSI cache
> [   56.422545] ata1.00: Entering standby power mode
> [   56.422793] DRM log suspend
> 
> But because there is the "no_console_suspend" parameter, and we should make
> sure to not draw when the graphic driver is suspended, I think this patch is
> needed, and good enough.
> I will just rephrase the commit message, to make it clear, that some message
> won't be drawn, only if "no_console_suspend" is set.

Ah, I forgot about the "no_console_suspend" parameter. The problem
with this patch is that it would quietly drop all pending messages.

drm_log_write_thread() does not have any return value.
nbcon_emit_next_record() would assume that the message was printed.
And the kthread would continue emitting next message...

In compare, CON_SUSPENDED would cause that console_is_usable()
returns false. As a result, nbcon_kthread_func() would not try
to emit any message and go into a sleep.

If we set CON_SUSPENDED then the pending messages will get printed
after the resume. If we use this patch, the messages would get lost.


This is why I am not happy with this patch. I would prefer to
block the console. I see three better solutions:

  1. Set CON_SUSPENDED from drm_log_client_suspend even when
     "no_console_suspend" is used.

     It is a bit dirty and might cause some confusion.


  2. Add a new flag, e.g. CON_BLOCKED or CON_DRIVER_BLOCKED,
     which might be used for this purpose.


  3. Allow con->write_thread() to return an error code.

     The question is how exactly the error should be handled.
     The kthread would not know when the printing might succeed
     again.


I personally prefer the 2nd variant.


Best Regards,
Petr

PS: I am sorry for the late reply. I had vacation...