LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 7/7] hvc: add an M3000 firmware console backend
       [not found] <20261002161515.932316-1-linmag7@gmail.com>
@ 2026-10-02 16:14 ` Magnus Lindholm
  2026-10-04 12:50   ` Greg Kroah-Hartman
  0 siblings, 1 reply; 3+ messages in thread
From: Magnus Lindholm @ 2026-10-02 16:14 UTC (permalink / raw)
  To: sparclinux, David S . Miller, Andreas Larsson
  Cc: linux-kernel, Magnus Lindholm, Greg Kroah-Hartman, Jiri Slaby,
	linuxppc-dev, linux-serial

Provide hvc0 through the IKKAKU Open Firmware pseudo-console, validating
model and console properties before matching. Serialize firmware calls,
keep the bounce buffer in locked mappings and poll input each open tick.

Keep output buffers const and bound no-progress retries because HVC
retries EAGAIN indefinitely. A stalled firmware call remains unbounded.
Use the existing SPARC p1275 interface and HVC output contract.

Signed-off-by: Magnus Lindholm <linmag7@gmail.com>
---
 MAINTAINERS                 |   6 ++
 drivers/tty/hvc/Kconfig     |  10 +++
 drivers/tty/hvc/Makefile    |   2 +
 drivers/tty/hvc/hvc_m3000.c | 167 ++++++++++++++++++++++++++++++++++++
 4 files changed, 185 insertions(+)
 create mode 100644 drivers/tty/hvc/hvc_m3000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3a19da74d00c..5bc9b1cabd0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10728,6 +10728,12 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/fujitsu-laptop.c
 
+FUJITSU M3000 FIRMWARE CONSOLE
+M:	Magnus Lindholm <linmag7@gmail.com>
+L:	linux-serial@vger.kernel.org
+S:	Maintained
+F:	drivers/tty/hvc/hvc_m3000.c
+
 FUJITSU TABLET EXTRAS
 M:	Robert Gerlach <khnz@gmx.de>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 5866195de26a..0feb7ec98f4f 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -134,3 +134,13 @@ config HVCS
 	  will depend on arch specific APIs exported from hvcserver.ko
 	  which will also be compiled when this driver is built as a
 	  module.
+
+config HVC_M3000
+	bool "M3000 firmware pseudo-console"
+	depends on SPARC64_VII
+	select HVC_DRIVER
+	help
+	  Provide hvc0 using Open Firmware stdin/stdout on IKKAKU only.
+	  Input is polled; no native UART registers are accessed.
+	  Firmware calls are serialized and use a locked-image bounce buffer.
+	  Say Y to use the M3000 firmware pseudo-console as hvc0.
diff --git a/drivers/tty/hvc/Makefile b/drivers/tty/hvc/Makefile
index 98880e357941..5b0277deb7c9 100644
--- a/drivers/tty/hvc/Makefile
+++ b/drivers/tty/hvc/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_HVC_IUCV)		+= hvc_iucv.o
 obj-$(CONFIG_HVC_UDBG)		+= hvc_udbg.o
 obj-$(CONFIG_HVC_RISCV_SBI)	+= hvc_riscv_sbi.o
 obj-$(CONFIG_HVCS)		+= hvcs.o
+
+obj-$(CONFIG_HVC_M3000) += hvc_m3000.o
diff --git a/drivers/tty/hvc/hvc_m3000.c b/drivers/tty/hvc/hvc_m3000.c
new file mode 100644
index 000000000000..0ef4c888a6a2
--- /dev/null
+++ b/drivers/tty/hvc/hvc_m3000.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Experimental M3000 Open Firmware pseudo-console backend. */
+#include <linux/console.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/irqflags.h>
+#include <linux/string.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <asm/oplib.h>
+#include <asm/spitfire.h>
+#include "hvc_console.h"
+
+#define M3000_WRITE_ATTEMPTS 8
+
+static int m3000_stdin;
+static bool m3000_ready;
+/* Keep firmware buffers within the locked kernel image mappings. */
+static u8 m3000_buffer[256];
+static DEFINE_RAW_SPINLOCK(m3000_buffer_lock);
+static struct timer_list m3000_poll_timer;
+static bool m3000_poll_active;
+
+/*
+ * HVC's idle backoff can exceed the firmware input FIFO's capacity.
+ * Wake its worker each tick while open; keep PROM calls out of the timer.
+ */
+static void m3000_poll_tick(struct timer_list *timer)
+{
+	if (!READ_ONCE(m3000_poll_active))
+		return;
+	hvc_kick();
+	mod_timer(&m3000_poll_timer, jiffies + 1);
+}
+
+static int m3000_open(struct hvc_struct *hp, int data)
+{
+	WRITE_ONCE(m3000_poll_active, true);
+	mod_timer(&m3000_poll_timer, jiffies + 1);
+	return 0;
+}
+
+static void m3000_close(struct hvc_struct *hp, int data)
+{
+	WRITE_ONCE(m3000_poll_active, false);
+	timer_delete_sync(&m3000_poll_timer);
+}
+
+static ssize_t m3000_io(u8 *in, const u8 *out, size_t count)
+{
+	unsigned long args[7], flags;
+	bool input = in != NULL;
+	int ret;
+
+	if (!count)
+		return 0;
+	count = min_t(size_t, count, input ? 1 : sizeof(m3000_buffer));
+	/* Serialize the bounce buffer; p1275 separately serializes firmware. */
+	raw_spin_lock_irqsave(&m3000_buffer_lock, flags);
+	if (!input)
+		memcpy(m3000_buffer, out, count);
+	args[0] = (unsigned long)(input ? "read" : "write");
+	args[1] = 3;
+	args[2] = 1;
+	args[3] = (unsigned int)(input ? m3000_stdin : prom_stdout);
+	args[4] = (unsigned long)m3000_buffer;
+	args[5] = count;
+	args[6] = (unsigned long)-1;
+	p1275_cmd_direct(args);
+	ret = (int)args[6];
+	if (ret > 0 && ret <= count && input)
+		memcpy(in, m3000_buffer, ret);
+	raw_spin_unlock_irqrestore(&m3000_buffer_lock, flags);
+	if (ret == -2 || ret == 0)
+		return input ? 0 : -EAGAIN;
+	if (ret < 0 || ret > count)
+		return -EIO;
+	return ret;
+}
+
+static ssize_t m3000_get(u32 termno, u8 *buf, size_t count)
+{
+	return m3000_io(buf, NULL, count);
+}
+
+static ssize_t m3000_put(u32 termno, const u8 *buf, size_t count)
+{
+	ssize_t ret;
+	int attempt;
+
+	/*
+	 * HVC retries -EAGAIN forever, so drop this chunk without logging when
+	 * the no-progress budget expires. This cannot bound a stalled firmware
+	 * call or lock acquisition.
+	 */
+	for (attempt = 0; attempt < M3000_WRITE_ATTEMPTS; attempt++) {
+		ret = m3000_io(NULL, buf, count);
+		if (ret != -EAGAIN)
+			return ret;
+		cpu_relax();
+	}
+	return -EIO;
+}
+
+static const struct hv_ops m3000_ops = {
+	.get_chars = m3000_get,
+	.put_chars = m3000_put,
+	.notifier_add = m3000_open,
+	.notifier_del = m3000_close,
+	.notifier_hangup = m3000_close,
+};
+
+static bool __init m3000_property_matches(phandle node, const char *prop,
+					 const char *expected)
+{
+	char value[64];
+	int len;
+
+	len = prom_getproperty(node, prop, value, sizeof(value) - 1);
+	if (len <= 0)
+		return false;
+	value[len] = '\0';
+	return !strcmp(value, expected);
+}
+
+static int __init m3000_console_init(void)
+{
+	phandle node;
+	int ret;
+
+	if (tlb_type != sparc64_vii)
+		return -ENODEV;
+	if (!m3000_property_matches(prom_finddevice("/"), "model", "IKKAKU"))
+		return -ENODEV;
+	m3000_stdin = prom_getint(prom_chosen_node, "stdin");
+	if (!m3000_stdin || m3000_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(m3000_stdin);
+	if (!m3000_property_matches(node, "name", "pseudo-console"))
+		return -ENODEV;
+	ret = hvc_instantiate(0, 0, &m3000_ops);
+	if (ret < 0)
+		return ret;
+	m3000_ready = true;
+	return 0;
+}
+console_initcall(m3000_console_init);
+
+static int __init m3000_tty_init(void)
+{
+	struct hvc_struct *hp;
+
+	if (!m3000_ready)
+		return -ENODEV;
+	timer_setup(&m3000_poll_timer, m3000_poll_tick, 0);
+	hp = hvc_alloc(0, 0, &m3000_ops, sizeof(m3000_buffer));
+	if (IS_ERR(hp))
+		return PTR_ERR(hp);
+	hp->ws.ws_row = 24;
+	hp->ws.ws_col = 80;
+	pr_info("M3000: firmware-backed hvc0 tty ready (serialized, polled input)\n");
+	return 0;
+}
+device_initcall(m3000_tty_init);
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH 7/7] hvc: add an M3000 firmware console backend
  2026-10-02 16:14 ` [PATCH 7/7] hvc: add an M3000 firmware console backend Magnus Lindholm
@ 2026-10-04 12:50   ` Greg Kroah-Hartman
  2026-10-04 15:24     ` Magnus Lindholm
  0 siblings, 1 reply; 3+ messages in thread
From: Greg Kroah-Hartman @ 2026-10-04 12:50 UTC (permalink / raw)
  To: Magnus Lindholm
  Cc: sparclinux, David S . Miller, Andreas Larsson, linux-kernel,
	Jiri Slaby, linuxppc-dev, linux-serial

On Fri, Oct 02, 2026 at 06:14:29PM +0200, Magnus Lindholm wrote:
> Provide hvc0 through the IKKAKU Open Firmware pseudo-console, validating
> model and console properties before matching. Serialize firmware calls,
> keep the bounce buffer in locked mappings and poll input each open tick.
> 
> Keep output buffers const and bound no-progress retries because HVC
> retries EAGAIN indefinitely. A stalled firmware call remains unbounded.
> Use the existing SPARC p1275 interface and HVC output contract.
> 
> Signed-off-by: Magnus Lindholm <linmag7@gmail.com>
> ---
>  MAINTAINERS                 |   6 ++
>  drivers/tty/hvc/Kconfig     |  10 +++
>  drivers/tty/hvc/Makefile    |   2 +
>  drivers/tty/hvc/hvc_m3000.c | 167 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 185 insertions(+)
>  create mode 100644 drivers/tty/hvc/hvc_m3000.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3a19da74d00c..5bc9b1cabd0a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10728,6 +10728,12 @@ L:	platform-driver-x86@vger.kernel.org
>  S:	Maintained
>  F:	drivers/platform/x86/fujitsu-laptop.c
>  
> +FUJITSU M3000 FIRMWARE CONSOLE
> +M:	Magnus Lindholm <linmag7@gmail.com>
> +L:	linux-serial@vger.kernel.org
> +S:	Maintained
> +F:	drivers/tty/hvc/hvc_m3000.c
> +
>  FUJITSU TABLET EXTRAS
>  M:	Robert Gerlach <khnz@gmx.de>
>  L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
> index 5866195de26a..0feb7ec98f4f 100644
> --- a/drivers/tty/hvc/Kconfig
> +++ b/drivers/tty/hvc/Kconfig
> @@ -134,3 +134,13 @@ config HVCS
>  	  will depend on arch specific APIs exported from hvcserver.ko
>  	  which will also be compiled when this driver is built as a
>  	  module.
> +
> +config HVC_M3000
> +	bool "M3000 firmware pseudo-console"
> +	depends on SPARC64_VII
> +	select HVC_DRIVER
> +	help
> +	  Provide hvc0 using Open Firmware stdin/stdout on IKKAKU only.
> +	  Input is polled; no native UART registers are accessed.
> +	  Firmware calls are serialized and use a locked-image bounce buffer.
> +	  Say Y to use the M3000 firmware pseudo-console as hvc0.
> diff --git a/drivers/tty/hvc/Makefile b/drivers/tty/hvc/Makefile
> index 98880e357941..5b0277deb7c9 100644
> --- a/drivers/tty/hvc/Makefile
> +++ b/drivers/tty/hvc/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_HVC_IUCV)		+= hvc_iucv.o
>  obj-$(CONFIG_HVC_UDBG)		+= hvc_udbg.o
>  obj-$(CONFIG_HVC_RISCV_SBI)	+= hvc_riscv_sbi.o
>  obj-$(CONFIG_HVCS)		+= hvcs.o
> +
> +obj-$(CONFIG_HVC_M3000) += hvc_m3000.o

No need for a blank line, right?  And why no tab used here?

> diff --git a/drivers/tty/hvc/hvc_m3000.c b/drivers/tty/hvc/hvc_m3000.c
> new file mode 100644
> index 000000000000..0ef4c888a6a2
> --- /dev/null
> +++ b/drivers/tty/hvc/hvc_m3000.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Experimental M3000 Open Firmware pseudo-console backend. */

What will make it not "experimental"?

And no copyright?



> +#include <linux/console.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/irqflags.h>
> +#include <linux/string.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +#include <asm/oplib.h>
> +#include <asm/spitfire.h>
> +#include "hvc_console.h"
> +
> +#define M3000_WRITE_ATTEMPTS 8

why 8?

> +
> +static int m3000_stdin;
> +static bool m3000_ready;
> +/* Keep firmware buffers within the locked kernel image mappings. */
> +static u8 m3000_buffer[256];
> +static DEFINE_RAW_SPINLOCK(m3000_buffer_lock);
> +static struct timer_list m3000_poll_timer;
> +static bool m3000_poll_active;

Shouldn't these be in a structure attached to the console somewhere?
Otherwise you have limited yourself to just one of these.

> +
> +/*
> + * HVC's idle backoff can exceed the firmware input FIFO's capacity.
> + * Wake its worker each tick while open; keep PROM calls out of the timer.
> + */
> +static void m3000_poll_tick(struct timer_list *timer)
> +{
> +	if (!READ_ONCE(m3000_poll_active))
> +		return;

Why are you accessing m3000_poll_active like this in an attempt to not
use a real lock?  Are you _sure_ a bool will work this way properly?

> +	hvc_kick();
> +	mod_timer(&m3000_poll_timer, jiffies + 1);
> +}
> +
> +static int m3000_open(struct hvc_struct *hp, int data)
> +{
> +	WRITE_ONCE(m3000_poll_active, true);
> +	mod_timer(&m3000_poll_timer, jiffies + 1);
> +	return 0;
> +}
> +
> +static void m3000_close(struct hvc_struct *hp, int data)
> +{
> +	WRITE_ONCE(m3000_poll_active, false);
> +	timer_delete_sync(&m3000_poll_timer);
> +}
> +
> +static ssize_t m3000_io(u8 *in, const u8 *out, size_t count)
> +{
> +	unsigned long args[7], flags;
> +	bool input = in != NULL;
> +	int ret;
> +
> +	if (!count)
> +		return 0;
> +	count = min_t(size_t, count, input ? 1 : sizeof(m3000_buffer));

No need for min_t(), why not just min()?

> +	/* Serialize the bounce buffer; p1275 separately serializes firmware. */
> +	raw_spin_lock_irqsave(&m3000_buffer_lock, flags);

why is this a "raw" spinlock?

> +	if (!input)
> +		memcpy(m3000_buffer, out, count);
> +	args[0] = (unsigned long)(input ? "read" : "write");

A string being cast to an unsigned long?  Are you _sure_?

> +	args[1] = 3;
> +	args[2] = 1;
> +	args[3] = (unsigned int)(input ? m3000_stdin : prom_stdout);
> +	args[4] = (unsigned long)m3000_buffer;
> +	args[5] = count;
> +	args[6] = (unsigned long)-1;
> +	p1275_cmd_direct(args);
> +	ret = (int)args[6];
> +	if (ret > 0 && ret <= count && input)
> +		memcpy(in, m3000_buffer, ret);
> +	raw_spin_unlock_irqrestore(&m3000_buffer_lock, flags);
> +	if (ret == -2 || ret == 0)
> +		return input ? 0 : -EAGAIN;
> +	if (ret < 0 || ret > count)
> +		return -EIO;
> +	return ret;
> +}
> +
> +static ssize_t m3000_get(u32 termno, u8 *buf, size_t count)
> +{
> +	return m3000_io(buf, NULL, count);
> +}
> +
> +static ssize_t m3000_put(u32 termno, const u8 *buf, size_t count)
> +{
> +	ssize_t ret;
> +	int attempt;
> +
> +	/*
> +	 * HVC retries -EAGAIN forever, so drop this chunk without logging when
> +	 * the no-progress budget expires. This cannot bound a stalled firmware
> +	 * call or lock acquisition.
> +	 */
> +	for (attempt = 0; attempt < M3000_WRITE_ATTEMPTS; attempt++) {
> +		ret = m3000_io(NULL, buf, count);
> +		if (ret != -EAGAIN)
> +			return ret;
> +		cpu_relax();
> +	}
> +	return -EIO;
> +}
> +
> +static const struct hv_ops m3000_ops = {
> +	.get_chars = m3000_get,
> +	.put_chars = m3000_put,
> +	.notifier_add = m3000_open,
> +	.notifier_del = m3000_close,
> +	.notifier_hangup = m3000_close,
> +};
> +
> +static bool __init m3000_property_matches(phandle node, const char *prop,
> +					 const char *expected)
> +{
> +	char value[64];
> +	int len;
> +
> +	len = prom_getproperty(node, prop, value, sizeof(value) - 1);
> +	if (len <= 0)
> +		return false;
> +	value[len] = '\0';
> +	return !strcmp(value, expected);
> +}
> +
> +static int __init m3000_console_init(void)
> +{
> +	phandle node;
> +	int ret;
> +
> +	if (tlb_type != sparc64_vii)
> +		return -ENODEV;
> +	if (!m3000_property_matches(prom_finddevice("/"), "model", "IKKAKU"))
> +		return -ENODEV;
> +	m3000_stdin = prom_getint(prom_chosen_node, "stdin");
> +	if (!m3000_stdin || m3000_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(m3000_stdin);
> +	if (!m3000_property_matches(node, "name", "pseudo-console"))
> +		return -ENODEV;
> +	ret = hvc_instantiate(0, 0, &m3000_ops);
> +	if (ret < 0)
> +		return ret;
> +	m3000_ready = true;
> +	return 0;
> +}
> +console_initcall(m3000_console_init);
> +
> +static int __init m3000_tty_init(void)
> +{
> +	struct hvc_struct *hp;
> +
> +	if (!m3000_ready)
> +		return -ENODEV;
> +	timer_setup(&m3000_poll_timer, m3000_poll_tick, 0);
> +	hp = hvc_alloc(0, 0, &m3000_ops, sizeof(m3000_buffer));
> +	if (IS_ERR(hp))
> +		return PTR_ERR(hp);
> +	hp->ws.ws_row = 24;
> +	hp->ws.ws_col = 80;
> +	pr_info("M3000: firmware-backed hvc0 tty ready (serialized, polled input)\n");

When drivers work properly, they are quiet.

thanks,

greg k-h


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 7/7] hvc: add an M3000 firmware console backend
  2026-10-04 12:50   ` Greg Kroah-Hartman
@ 2026-10-04 15:24     ` Magnus Lindholm
  0 siblings, 0 replies; 3+ messages in thread
From: Magnus Lindholm @ 2026-10-04 15:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: sparclinux, David S . Miller, Andreas Larsson, linux-kernel,
	Jiri Slaby, linuxppc-dev, linux-serial

Hi Greg,

Thanks alot for taking the time to review this.

On Sun, Oct 4, 2026 at 2:50 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> > @@ -11,3 +11,5 @@ obj-$(CONFIG_HVC_IUCV)              += hvc_iucv.o
> >  obj-$(CONFIG_HVC_UDBG)               += hvc_udbg.o
> >  obj-$(CONFIG_HVC_RISCV_SBI)  += hvc_riscv_sbi.o
> >  obj-$(CONFIG_HVCS)           += hvcs.o
> > +
> > +obj-$(CONFIG_HVC_M3000) += hvc_m3000.o
>
> No need for a blank line, right?  And why no tab used here?
>

Agreed; I have removed the blank line and matched the Makefile's tab
alignment.

> > diff --git a/drivers/tty/hvc/hvc_m3000.c b/drivers/tty/hvc/hvc_m3000.c
> > new file mode 100644
> > index 000000000000..0ef4c888a6a2
> > --- /dev/null
> > +++ b/drivers/tty/hvc/hvc_m3000.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Experimental M3000 Open Firmware pseudo-console backend. */
>
> What will make it not "experimental"?
>

That was stale wording from bring-up, rather than a defined support
status. I have removed it.

> And no copyright?
>
>

I'll  add a copyright notice for the v2.

> > +
> > +#define M3000_WRITE_ATTEMPTS 8
>
> why 8?
>

There is no hardware requirement for eight. The intent was to bound
no-progress retries because HVC's console write path retries -EAGAIN
indefinitely; I will revisit the retry policy before sending v2.

> > +
> > +static int m3000_stdin;
> > +static bool m3000_ready;
> > +/* Keep firmware buffers within the locked kernel image mappings. */
> > +static u8 m3000_buffer[256];
> > +static DEFINE_RAW_SPINLOCK(m3000_buffer_lock);
> > +static struct timer_list m3000_poll_timer;
> > +static bool m3000_poll_active;
>
> Shouldn't these be in a structure attached to the console somewhere?
> Otherwise you have limited yourself to just one of these.
>

I can group the state into a structure. This backend uses the single
/chosen stdin/stdout pair, and the buffer must remain in the locked
kernel image mappings for firmware access. I will also review how the
state is associated with HVC before finalizing a v2

> > +
> > +/*
> > + * HVC's idle backoff can exceed the firmware input FIFO's capacity.
> > + * Wake its worker each tick while open; keep PROM calls out of the timer.
> > + */
> > +static void m3000_poll_tick(struct timer_list *timer)
> > +{
> > +     if (!READ_ONCE(m3000_poll_active))
> > +             return;
>
> Why are you accessing m3000_poll_active like this in an attempt to not
> use a real lock?  Are you _sure_ a bool will work this way properly?
>

Agreed that READ_ONCE/WRITE_ONCE alone do not explain the timer lifecycle.
I will review synchronization between open, close and timer rearming,
and make that protocol explicit in v2.

> > +     hvc_kick();
> > +     mod_timer(&m3000_poll_timer, jiffies + 1);
> > +}
> > +
> > +static int m3000_open(struct hvc_struct *hp, int data)
> > +{
> > +     WRITE_ONCE(m3000_poll_active, true);
> > +     mod_timer(&m3000_poll_timer, jiffies + 1);
> > +     return 0;
> > +}
> > +
> > +static void m3000_close(struct hvc_struct *hp, int data)
> > +{
> > +     WRITE_ONCE(m3000_poll_active, false);
> > +     timer_delete_sync(&m3000_poll_timer);
> > +}
> > +
> > +static ssize_t m3000_io(u8 *in, const u8 *out, size_t count)
> > +{
> > +     unsigned long args[7], flags;
> > +     bool input = in != NULL;
> > +     int ret;
> > +
> > +     if (!count)
> > +             return 0;
> > +     count = min_t(size_t, count, input ? 1 : sizeof(m3000_buffer));
>
> No need for min_t(), why not just min()?
>

Agreed, will change it to min().

> > +     /* Serialize the bounce buffer; p1275 separately serializes firmware. */
> > +     raw_spin_lock_irqsave(&m3000_buffer_lock, flags);
>
> why is this a "raw" spinlock?
>

The intent is to protect the shared firmware buffer in console output
contexts where sleeping is not allowed. I will check the precise HVC
calling contexts and lock ordering before deciding whether the raw lock
is necessary and documenting that choice.

> > +     if (!input)
> > +             memcpy(m3000_buffer, out, count);
> > +     args[0] = (unsigned long)(input ? "read" : "write");
>
> A string being cast to an unsigned long?  Are you _sure_?
>

Yes: p1275_cmd_direct() takes unsigned long firmware argument cells,
and the first cell carries the service-name pointer. The existing
SPARC prom_console_write_buf() uses the same cast for "write".

> > +     if (!m3000_ready)
> > +             return -ENODEV;
> > +     timer_setup(&m3000_poll_timer, m3000_poll_tick, 0);
> > +     hp = hvc_alloc(0, 0, &m3000_ops, sizeof(m3000_buffer));
> > +     if (IS_ERR(hp))
> > +             return PTR_ERR(hp);
> > +     hp->ws.ws_row = 24;
> > +     hp->ws.ws_col = 80;
> > +     pr_info("M3000: firmware-backed hvc0 tty ready (serialized, polled input)\n");
>
> When drivers work properly, they are quiet.
>

Agreed; I have removed the initialization message.

Thanks again for the review. I'll prepare a v2 addressing your
comments and explain the reasoning behind any design choices
that remain.

Regards

Magnus


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-10-04 15:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20261002161515.932316-1-linmag7@gmail.com>
2026-10-02 16:14 ` [PATCH 7/7] hvc: add an M3000 firmware console backend Magnus Lindholm
2026-10-04 12:50   ` Greg Kroah-Hartman
2026-10-04 15:24     ` Magnus Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox