linux-kernel - Re: [PATCH 26/29] serial: 8250: use serial

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: <Z7dI9-mTl820lyY-@smile.fi.intel.com>
Date: Thu, 20 Feb 2025 17:23:35 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>
Cc: gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 26/29] serial: 8250: use serial_in/out() helpers

On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> There are serial_in/out() helpers to be used instead of direct
> p->serial_in/out(). Use them in various 8250 drivers.

Okay, I have checked all calls, and I think to avoid possible issues
we need to add a few comments here and there, see below.

But first of all, can we actually use serial_port_in()/serial_port_out()
in the cases where we have already a pointer to uart_port?

...

Here we should add a comment like

/*
 * This function is being called as part of the uart_port::serial_out()
 * routine. Hence it must not call serial_out() on itself against
 * the modified registers here, i.e. LCR.
 */

> static void dw8250_force_idle(struct uart_port *p)
> {
> 	struct uart_8250_port *up = up_to_u8250p(p);
> 	unsigned int lsr;

Here we should add a comment like

	/*
	 * The following call currently performs serial_out()
	 * against the FCR register. Because it differs to LCR
	 * there will be no dead loop, but if it ever gets
	 * modified, we might need a new custom version that
	 * avoids infinite recursion.
	 */

> 	serial8250_clear_and_reinit_fifos(up);

...

On top of this function we need to add the same comment as on top of
dw8250_force_idle() above.

>  static void dw8250_check_lcr(struct uart_port *p, int offset, int value)

...

The rest is legit as we shouldn't use those in the ->serial_in()/->serial_out().

-- 
With Best Regards,
Andy Shevchenko