Re: [PATCH v4 14/14] tty: gunyah: Add tty console driver for RM Console Services

[Jiri Slaby]

On 28. 09. 22, 21:56, Elliot Berman wrote:
> --- /dev/null
> +++ b/drivers/tty/gunyah_tty.c
> @@ -0,0 +1,409 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "gh_rsc_mgr_console: " fmt
> +
> +#include <linux/gunyah_rsc_mgr.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/workqueue.h>
> +#include <linux/spinlock.h>
> +#include <linux/tty_flip.h>
> +#include <linux/console.h>
> +#include <linux/module.h>
> +#include <linux/kfifo.h>
> +#include <linux/kref.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>

Sort alphabetically, please. Not by inv. xmas tree.

> +/*
> + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know
> + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be
> + * added/removed dynamically, we need to make sure we have enough allocated.
> + */
> +#define RSC_MGR_TTY_ADAPTERS		16
> +
> +/* # of payload bytes that can fit in a 1-fragment CONSOLE_WRITE message */
> +#define RM_CONS_WRITE_MSG_SIZE	((1 * (GH_MSGQ_MAX_MSG_SIZE - 8)) - 4)

"1 *" is kind of superfluous.

"- 8 - 4" -- it's too many magic constants in here. Define macros for them.

> +struct rm_cons_port {
> +	struct tty_port port;
> +	u16 vmid;
> +	bool open;
> +	unsigned int index;
> +
> +	DECLARE_KFIFO(put_fifo, char, 1024);

Why is tty_port::xmit_fifo not enough?

> +	spinlock_t fifo_lock;
> +	struct work_struct put_work;
> +
> +	struct rm_cons_data *cons_data;
> +};
> +
> +struct rm_cons_data {
> +	struct tty_driver *tty_driver;

It looks weird to have a driver per console/device.

> +	struct device *dev;
> +
> +	spinlock_t ports_lock;
> +	struct rm_cons_port *ports[RSC_MGR_TTY_ADAPTERS];
> +
> +	struct notifier_block rsc_mgr_notif;
> +	struct console console;
> +};
> +
> +static void put_work_fn(struct work_struct *ws)
> +{
> +	char buf[RM_CONS_WRITE_MSG_SIZE];

Ugh, is this 1024-12? Do not do this on stack!

> +	int count, ret;
> +	struct rm_cons_port *port = container_of(ws, struct rm_cons_port, put_work);
> +
> +	while (!kfifo_is_empty(&port->put_fifo)) {
> +		count = kfifo_out_spinlocked(&port->put_fifo, buf, sizeof(buf), &port->fifo_lock);
> +		if (count <= 0)
> +			continue;

This does not make much sense. 1) kfifo_is_empty() is not locked; 2) 
it's overly complicated. It can be, IMO:
while (1) {
  count = kfifo_out_spinlocked();
  if (!count)
    break;


> +static int rsc_mgr_console_notif(struct notifier_block *nb, unsigned long cmd, void *data)
> +{
> +	int count, i;

Not unsigned?

> +	struct rm_cons_port *rm_port = NULL;
> +	struct tty_port *tty_port = NULL;
> +	struct rm_cons_data *cons_data = container_of(nb, struct rm_cons_data, rsc_mgr_notif);
> +	const struct gh_rm_notification *notif = data;
> +	struct gh_rm_notif_vm_console_chars const * const msg = notif->buff;

Interesting mix of inconsistencies. Once you start with const, once you 
place it after struct. Please make it consistent (start with const).

ANd here, you should apply inv. xmas tree sorting.

> +
> +	if (cmd != GH_RM_NOTIF_VM_CONSOLE_CHARS ||
> +		notif->size < sizeof(*msg))
> +		return NOTIFY_DONE;

Weird indentation. notif->size should start with either 4 spaces less or 
one more tab.

regards,
--
js
suse labs