Re: [PATCH v2 6/6] serial: 8250: make port operations work on struct uart_8250_port

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Oct 13, 2026 at 08:48:51AM +0200, Jiri Slaby (SUSE) wrote:
> serial8250_register_8250_port() returns a line number and the other
> port operations (serial8250_unregister_port(), serial8250_suspend_port(),
> serial8250_resume_port()) accept it. Many drivers then convert the line
> back to the port using serial8250_get_port() anyway.
> 
> Switch the interface to struct uart_8250_port:
> * serial8250_register_8250_port() now returns the registered port, or
> an ERR_PTR() on failure.
> * serial8250_unregister_port(), serial8250_suspend_port() and
> serial8250_resume_port() accept the port.
> 
> Convert all users accordingly. Drivers store the port pointer instead
> of the line number. Several drivers kept a private struct only to hold
> the line (airoha, dfl, em, fsl, ioc3); these are dropped and the port
> is stored as drvdata directly.
> 
> This also fixes two latent issues:
> * For a CIR port, serial8250_register_8250_port() used to return 0.
> Callers stored this as the line, so a later unregister/suspend/resume
> operated on ttyS0. Now the CIR port itself is returned.
> * 8250_acorn checked ports[i] > 0 on removal, so a port registered as
> line 0 was never unregistered.
> 
> There are no more users of serial8250_get_port() outside of the 8250
> core, so drop its EXPORT.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>

Why not having Ilpo's Cc in the below list?

> Assisted-by: LLM
> ---
> Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@kernel.org>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Andrew Jeffery <andrew@codeconstruct.com.au>
> Cc: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: Billy Tsai <billy_tsai@aspeedtech.com>
> Cc: Florian Fainelli <florian.fainelli@broadcom.com>
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> Cc: Ray Jui <ray.jui@broadcom.com>
> Cc: Scott Branden <scott.branden@broadcom.com>
> Cc: Al Cooper <alcooperx@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Paul Cercueil <paul@crapouillou.net>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Cc: Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Cc: Thierry Reding <thierry.reding@kernel.org>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Kees Cook <kees@kernel.org>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-aspeed@lists.ozlabs.org
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-mips@vger.kernel.org
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linux-tegra@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org

> [v2] include err.h (Ilpo)
> ---

...

>  	seq_printf(s, "%7s: /dev/ptp%d\n", "PTP", ptp_clock_index(bp->ptp));
>  	for (i = 0; i < __PORT_COUNT; i++) {

What about

		struct uart_8250_port *uport = bp->port[i].uport;

here and use it accordingly in the below code?

> -		if (bp->port[i].line != -1)
> +		if (bp->port[i].uport)
>  			seq_printf(s, "%7s: /dev/ttyS%d\n", ptp_ocp_tty_port_name(i),
> -				   bp->port[i].line);
> +				   bp->port[i].uport->port.line);
>  	}

...

>  	for (i = 0; i < __PORT_COUNT; i++)
> -		if (bp->port[i].line != -1)
> -			serial8250_unregister_port(bp->port[i].line);
> +		if (bp->port[i].uport)
> +			serial8250_unregister_port(bp->port[i].uport);

Since it takes a pointer now, make it error pointer and NULL-aware

	if (IS_ERR_OR_NULL(...))
		return;

there. With that being done, all this redundant NULL-checks will gone.

...

> static void serial_card_remove(struct expansion_card *ec)

>  	ecard_set_drvdata(ec, NULL);
>  
>  	for (i = 0; i < info->num_ports; i++)
> -		if (info->ports[i] > 0)
> +		if (!IS_ERR_OR_NULL(info->ports[i]))
>  			serial8250_unregister_port(info->ports[i]);

See my point?

...

> static int aspeed_vuart_probe(struct platform_device *pdev)

>  	if (of_property_read_bool(np, "auto-flow-control"))
>  		port.capabilities |= UART_CAP_AFE;
>  
> -	rc = serial8250_register_8250_port(&port);
> -	if (rc < 0)
> +	struct uart_8250_port *uport = serial8250_register_8250_port(&port);
> +	if (IS_ERR(uport)) {
> +		rc = PTR_ERR(uport);
>  		goto err_sysfs_remove;
> +	}

You can get rid of these {} by using PTR_ERR_OR_ZERO(). Also do not define
variables in the middle of the code (it's only allowed for auto cleanup
variables and for-loop iterators).

...

I stopped here, I believe there are many similar cases which may be amended in
the same way as pointed out above.

-- 
With Best Regards,
Andy Shevchenko