linux-kernel - Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

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: <Z3gCeP_P7VPpcOLA@pathway.suse.cz>
Date: Fri, 3 Jan 2025 16:30:00 +0100
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Esben Haabendal <esben@...nix.com>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Ray Jui <rjui@...adcom.com>, Scott Branden <sbranden@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>, Sunil V L <sunilvl@...tanamicro.com>,
	Matt Turner <mattst88@...il.com>, Stefan Wahren <wahrenst@....net>,
	Uwe Kleine-König <u.kleine-koenig@...libre.com>,
	Kevin Hilman <khilman@...libre.com>,
	Markus Schneider-Pargmann <msp@...libre.com>,
	Udit Kumar <u-kumar1@...com>,
	Griffin Kroah-Hartman <griffin@...ah.com>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Serge Semin <fancer.lancer@...il.com>,
	linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER
 toggling for RS485

On Fri 2024-12-27 23:51:20, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console ->write() callback needs to enable/disable Tx. It does
> this by calling the ->rs485_start_tx() and ->rs485_stop_tx()
> callbacks. However, some of these callbacks also disable/enable
> interrupts and makes power management calls. This causes 2
> problems for console writing:
> 
> 1. A console write can occur in contexts that are illegal for
>    pm_runtime_*(). It is not even necessary for console writing
>    to use pm_runtime_*() because a console already does this in
>    serial8250_console_setup() and serial8250_console_exit().
>
> 2. The console ->write() callback already handles
>    disabling/enabling the interrupts by properly restoring the
>    previous IER value.

I was a bit confused by the description of the 2nd problem.
It is not clear whether it actually is a problem.

My understanding is that the nested IER manipulation does not
harm. And it is not needed at the same time. As a result,
the related pr_runtime_*() calls are not needed either.
So this is 2nd reason why the problematic pr_runtime_*() calls
can be removed in the serial8250_console_write() code path.

> Add an argument @toggle_ier to the ->rs485_start_tx() and
> ->rs485_stop_tx() callbacks to specify if they may disable/enable
> receive interrupts while using pm_runtime_*(). Console writing
> will not allow the toggling.
> 
> For all call sites other than console writing there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>

It seems that the patch does what is says. I'll try to describe
it using my words to explain how I understand it.

IMHO, the main motivation is to prevent calling pm_runtime_*() in
serial8250_console_write(). It is not safe in some contexts,
especially in NMI. And it is not needed from two reasons:

  1. The console->write() callback is used only by registered consoles.
     And the pm_runtime usage counter is already bumped during
     the console registration by serial8250_console_setup().

  2. The pm_runtime_*() calls are used in the following code path

	+ serial8250_console_write()
	  + up->rs485_start_tx()
	    + serial8250_em485_start_tx()
	      + serial8250_stop_rx()

     This code is not needed because serial8250_console_write()
     always clears IER by __serial8250_clear_IER(up) anyway.

     In fact, the extra IER manipulation in serial8250_em485_start_tx()
     might be seen as a bug. Well, it is a NOP.

All in all, the patch looks good to me.

Reviewed-by: Petr Mladek <pmladek@...e.com>

Best Regards,
Petr