Hi Greg, Thanks alot for taking the time to review this.
On Sun, Oct 4, 2026 at 2:50 PM Greg Kroah-Hartman <[email protected]> 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
