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

[Magnus Lindholm]

Hi Greg,

Thanks alot for taking the time to review this.

On Sun, Oct 4, 2026 at 2:50 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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