Hi Jiri,
On Mon, Oct 12, 2026 at 8:46 AM Jiri Slaby <[email protected]> wrote:
>
> On 11. 10. 26, 21:55, Magnus Lindholm wrote:
> ...
> > --- /dev/null
> > +++ b/drivers/tty/hvc/hvc_m3000.c
> > @@ -0,0 +1,205 @@
>
> > +struct m3000_console {
> > + int stdin;
> > + bool ready;
> > + u8 buffer[256];
> > + raw_spinlock_t buffer_lock;
> > + struct timer_list poll_timer;
> > + struct mutex poll_mutex; /* Serialize open against synchronous close.
> > */
> > + spinlock_t poll_lock; /* Protect active state and timer rearming. */
> > + bool poll_active;
>
> Perhaps group these a bit better, so that there are not such holes?
>
Done for v3. Reordering the members reduced the structure from 344
to 336 bytes with the tested configuration.
> > +};
> ...
> > +/*
> > + * 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)
> > +{
> > + struct m3000_console *con = timer_container_of(con, timer,
> > poll_timer);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&con->poll_lock, flags);
>
> Any reason not to use guard()s?
>
No particular reason. I've switched this to guard(spinlock_irqsave).
> > + if (con->poll_active) {
> > + hvc_kick();
> > + mod_timer(&con->poll_timer, jiffies + 1);
> > + }
> > + spin_unlock_irqrestore(&con->poll_lock, flags);
> > +}
> > +
> > +static int m3000_open(struct hvc_struct *hp, int data)
> > +{
> > + struct m3000_console *con = m3000_console(hp->vtermno);
> > + unsigned long flags;
> > +
> > + if (!con)
> > + return -ENODEV;
> > + mutex_lock(&con->poll_mutex);
> > + spin_lock_irqsave(&con->poll_lock, flags);
>
> Same here.
>
Done, using guards for both locks, preserving their acquisition and
release order.
> > + con->poll_active = true;
> > + mod_timer(&con->poll_timer, jiffies + 1);
> > + spin_unlock_irqrestore(&con->poll_lock, flags);
> > + mutex_unlock(&con->poll_mutex);
> > + return 0;
> > +}
> > +
> > +static void m3000_close(struct hvc_struct *hp, int data)
> > +{
> > + struct m3000_console *con = m3000_console(hp->vtermno);
> > + unsigned long flags;
> > +
> > + if (!con)
> > + return;
> > + mutex_lock(&con->poll_mutex);
> > + spin_lock_irqsave(&con->poll_lock, flags);
>
> And here.
>
Done, using guard(mutex) and a scoped spinlock guard. The spinlock is
released before timer_delete_sync(), while the mutex remains held.
> > + con->poll_active = false;
> > + spin_unlock_irqrestore(&con->poll_lock, flags);
> > + /* Block reopen until the callback has finished and cannot rearm. */
> > + timer_delete_sync(&con->poll_timer);
> > + mutex_unlock(&con->poll_mutex);
> > +}
> > +
> > +static ssize_t m3000_io(struct m3000_console *con, u8 *in,
> > + const u8 *out, size_t count)
> > +{
> > + unsigned long args[7], flags;
> > + bool input = in;
>
> You can test "in" below directly, right?
>
Yes, I've removed the boolean and now test "in" directly.
> > + int ret;
> > +
> > + if (!count)
> > + return 0;
> > + count = min(count, input ? 1 : sizeof(con->buffer));
> > + /* Console writes can run with interrupts disabled; this lock cannot
> > sleep. */
> > + raw_spin_lock_irqsave(&con->buffer_lock, flags);
>
> scoped_guard()
>
Done for v3. Reordering the members reduced the structure from 344
to 336 bytes with the tested configuration.
> > + if (!input)
> > + memcpy(con->buffer, out, count);
> > + args[0] = (unsigned long)(input ? "read" : "write");
> > + args[1] = 3;
> > + args[2] = 1;
> > + args[3] = (unsigned int)(input ? con->stdin : prom_stdout);
> > + args[4] = (unsigned long)con->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, con->buffer, ret);
> > + raw_spin_unlock_irqrestore(&con->buffer_lock, flags);
> > + if (ret == -2 || ret == 0)
>
> That's full of magic constants. Can we have defines for those?
>
I've added defines for the firmware argument/return counts and the
-1/-2 status values. I've kept zero literal as a zero-byte result.
> > + return 0;
> > + if (ret < 0 || ret > count)
> > + return -EIO;
> > + return ret;
> thanks,
> --
> js
> suse labs
Both GCC and Clang W=1 builds completed, and the GCC-built kernel
passed boot and HVC input/output, open/close, and hangup/reopen tests
on the M3000.
I'll put out the v3 asap
Regards,
Magnus
