linux-kernel - Re: [PATCH 4/5] i3c: slave: func: add tty driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <83a0f0a0-56b1-4021-a37d-ef68e7fab712@kernel.org>
Date:   Thu, 19 Oct 2023 09:21:38 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Frank Li <Frank.Li@....com>, miquel.raynal@...tlin.com,
        conor.culhane@...vaco.com, alexandre.belloni@...tlin.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, corbet@....net, joe@...ches.com,
        linux-i3c@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Cc:     gregkh@...uxfoundation.org, imx@...ts.linux.dev,
        linux-serial@...r.kernel.org
Subject: Re: [PATCH 4/5] i3c: slave: func: add tty driver

On 18. 10. 23, 23:58, Frank Li wrote:
> Add tty over I3C slave function driver.

Many of the master review comments apply also here. Please fix here too. 
More below.

> --- /dev/null
> +++ b/drivers/i3c/func/tty.c
> @@ -0,0 +1,548 @@
...
> +static void i3c_slave_tty_rx_complete(struct i3c_request *req)
> +{
> +	struct ttyi3c_port *port = req->context;
> +
> +	if (req->status == I3C_REQUEST_CANCEL) {
> +		i3c_slave_ctrl_free_request(req);
> +		return;
> +	}
> +
> +	for (int i = 0; i < req->actual; i++)
> +		tty_insert_flip_char(&port->port, *(u8 *)(req->buf + i), 0);

Maybe I miss something obvious, but req->buf is void *. So why not 
simple tty_insert_flip_string()?


> +	sport->buffer = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (!sport->buffer)
> +		return -ENOMEM;
> +
> +	sport->xmit.buf = (void *)get_zeroed_page(GFP_KERNEL);

tty_port_alloc_xmit_buf()


> +static int i3c_tty_probe(struct i3c_slave_func *func)
> +{
> +	struct device *dev = &func->dev;
> +	struct ttyi3c_port *port;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->i3cdev = func;
> +	dev_set_drvdata(&func->dev, port);
> +
> +	port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&func->dev));

Another wq? You'd have to have a strong reason for these. Drop that.

> +	if (!port->workqueue)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&port->work, i3c_slave_tty_i3c_work);
> +
> +	return 0;
> +}
> +
> +static void  i3c_tty_remove(struct i3c_slave_func *func)
> +{
> +	struct ttyi3c_port *port;
> +
> +	port = dev_get_drvdata(&func->dev);

That can be on one line.

> +
> +	destroy_workqueue(port->workqueue);
> +}


> +static int i3c_open(struct tty_struct *tty, struct file *filp)
> +{
> +	struct ttyi3c_port *sport = tty->driver_data;
> +	int ret;
> +
> +	if (!i3c_slave_ctrl_get_addr(sport->i3cdev->ctrl)) {
> +		dev_info(&sport->i3cdev->dev, "No slave addr assigned, try hotjoin");

Should this be a debug print instead?

> +		ret = i3c_slave_ctrl_hotjoin(sport->i3cdev->ctrl);
> +		if (ret) {
> +			dev_err(&sport->i3cdev->dev, "Hotjoin failure, check connection");
> +			return ret;
> +		}
> +	}
> +
> +	return tty_port_open(&sport->port, tty, filp);

regards,
-- 
js
suse labs