Re: [PATCH v4 10/10] hvc: add an M3000 firmware console backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hvc: hvc_m3000 fails to register console when CONFIG_VT_CONSOLE=y
- [Low] tty: hvc_m3000: ineffective window size initialization in m3000_tty_init()
--

commit 7a0bedd3c2b41c2e4d2aba3f69c932366fc243a8
Author: Magnus Lindholm <linmag7@gmail.com>

hvc: add an M3000 firmware console backend

This commit adds an HVC console backend for the Fujitsu M3000 (IKKAKU
platform) using the Open Firmware pseudo-console via the SPARC p1275
interface. It registers hvc0 after validating the firmware model and console
endpoints, polling input with a timer while the TTY is open.

> diff --git a/drivers/tty/hvc/hvc_m3000.c b/drivers/tty/hvc/hvc_m3000.c
> new file mode 100644
> index 0000000000000..e2150d4793080
> --- /dev/null
> +++ b/drivers/tty/hvc/hvc_m3000.c
> @@ -0,0 +1,202 @@

[ ... ]

> +static int __init m3000_console_init(void)
> +{
> +	struct m3000_console *con = &m3000_consoles[0];
> +	phandle node;
> +	int ret;
> +
> +	if (tlb_type != sparc64_vii)
> +		return -ENODEV;
> +	if (!m3000_property_matches(prom_finddevice("/"), "model", "IKKAKU"))
> +		return -ENODEV;
> +	con->stdin = prom_getint(prom_chosen_node, "stdin");
> +	if (!con->stdin || con->stdin == -1 || !prom_stdout || prom_stdout == -1)
> +		return -ENODEV;
> +	node = prom_inst2pkg(prom_stdout);
> +	if (!m3000_property_matches(node, "name", "pseudo-console"))
> +		return -ENODEV;
> +	node = prom_inst2pkg(con->stdin);
> +	if (!m3000_property_matches(node, "name", "pseudo-console"))
> +		return -ENODEV;
> +	ret = hvc_instantiate(0, 0, &m3000_ops);
> +	if (ret < 0)
> +		return ret;
> +	con->ready = true;
> +	return 0;
> +}
> +console_initcall(m3000_console_init);

[Severity: Medium]
When booting with CONFIG_VT_CONSOLE=y (which is enabled in sparc64_defconfig)
and without an explicit console=hvc0 command-line parameter, will hvc0 fail
to register as the kernel console here?

During console_init(), con_init() in drivers/tty/vt/vt.c runs before the hvc
console initcalls because vt/ precedes hvc/ in drivers/tty/Makefile. With no
preferred console selected yet, register_console(&vt_console_driver) enables
tty0 (backed by dummy_con) via try_enable_default_console() and unregisters
prom_early_console.

Next, hvc_console_init() in drivers/tty/hvc/hvc_console.c runs (linked before
hvc_m3000.o in drivers/tty/hvc/Makefile) and calls
register_console(&hvc_console). In kernel/printk/printk.c:register_console():

	if (preferred_console < 0) {
		if (hlist_empty(&console_list) || !console_first()->device ||
		    console_first()->flags & CON_BOOT) {
			try_enable_default_console(newcon);
		}
	}

Because console_first() is now vt_console_driver (which has a non-NULL
.device and does not have CON_BOOT set), try_enable_default_console() is
skipped and hvc_console.index remains -1.

When m3000_console_init() then runs and calls
hvc_instantiate(0, 0, &m3000_ops) -> hvc_check_console(0):

	if (index == hvc_console.index)
		register_console(&hvc_console);

Because hvc_console.index is -1 rather than 0, and add_preferred_console()
was never called, register_console(&hvc_console) is not invoked, leaving the
system without kernel console output after prom_early_console is unregistered.

Should m3000_console_init() call add_preferred_console("hvc", 0, NULL) before
hvc_instantiate() (or before hvc_console_init() runs) when Open Firmware's
stdout/stdin are verified to be pseudo-console?

> +
> +static int __init m3000_tty_init(void)
> +{
> +	struct m3000_console *con = &m3000_consoles[0];
> +	struct hvc_struct *hp;
> +
> +	if (!con->ready)
> +		return -ENODEV;
> +	timer_setup(&con->poll_timer, m3000_poll_tick, 0);
> +	hp = hvc_alloc(0, 0, &m3000_ops, sizeof(con->buffer));
> +	if (IS_ERR(hp))
> +		return PTR_ERR(hp);
> +	hp->ws.ws_row = 24;
> +	hp->ws.ws_col = 80;
> +	return 0;
> +}
> +device_initcall(m3000_tty_init);

[Severity: Low]
Does setting hp->ws.ws_row and hp->ws.ws_col directly in m3000_tty_init()
actually propagate the 80x24 window size to the TTY when /dev/hvc0 is opened?

In drivers/tty/hvc/hvc_console.c, neither hvc_install() nor hvc_open() reads
hp->ws. The only consumer of hp->ws is the workqueue callback
hvc_set_winsz():

	tty = tty_port_tty_get(&hp->port);
	if (!tty)
		return;

	spin_lock_irqsave(&hp->lock, hvc_flags);
	ws = hp->ws;
	spin_unlock_irqrestore(&hp->lock, hvc_flags);

	tty_do_resize(tty, &ws);

hvc_set_winsz() only runs when hp->tty_resize is scheduled via hvc_resize()
or __hvc_resize(), and requires a tty_struct to already be attached to
hp->port (which happens in hvc_open() via tty_port_tty_set() right before
hp->ops->notifier_add is called).

Should this use hvc_resize(hp, ws) inside m3000_open() instead so that
hvc_set_winsz() is scheduled once the TTY is attached?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018134738.1016106-1-linmag7@gmail.com?part=10