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

[Jiri Slaby <jirislaby@kernel.org>]

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?

> +};
...
> +/*
> + * 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?

> +	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.

> +	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.

> +	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?

> +	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()

> +	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?

> +		return 0;
> +	if (ret < 0 || ret > count)
> +		return -EIO;
> +	return ret;
thanks,
-- 
js
suse labs