linux-kernel - Re: [PATCH 8/9] serdev: add a tty port controller 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]

Date:   Sat, 07 Jan 2017 16:11:54 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Rob Herring <robh@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Jiri Slaby <jslaby@...e.com>,
        Sebastian Reichel <sre@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Dr . H . Nikolaus Schaller" <hns@...delico.com>,
        Peter Hurley <peter@...leysoftware.com>,
        Alan Cox <gnomes@...rguk.ukuu.org.uk>
Cc:     Loic Poulain <loic.poulain@...el.com>, Pavel Machek <pavel@....cz>,
        NeilBrown <neil@...wn.name>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-bluetooth@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/9] serdev: add a tty port controller driver

On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
> Add a serdev controller driver for tty ports.
> 
> The controller is registered with serdev when tty ports are registered
> with the TTY core. As the TTY core is built-in only, this has the side
> effect of making serdev built-in as well.
> 
> 

> +if SERIAL_DEV_BUS
> +
> +config SERIAL_DEV_CTRL_TTYPORT
> +	bool "Serial device TTY port controller"
> +	depends on TTY

> +	depends on SERIAL_DEV_BUS=y

Do you need one?


> +static int ttyport_receive_buf(struct tty_port *port, const unsigned
> char *cp,
> +				const unsigned char *fp, size_t
> count)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> +	mutex_lock(&serport->lock);
> +
> +	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> +		goto out;
> +
> +	serdev_controller_receive_buf(ctrl, cp, count);
> +
> +out:

out_unlock: ?

> +	mutex_unlock(&serport->lock);
> +	return count;
> +}
> +
> +static void ttyport_write_wakeup(struct tty_port *port)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags);

This doesn't prevent to be called this function in parallel. Is it okay?

> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		serdev_controller_write_wakeup(ctrl);
> +}

> +
> +static int ttyport_write_buf(struct serdev_controller *ctrl, const
> unsigned char *data, size_t len)
> +{
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +	return serport->tty->ops->write(serport->tty, data, len);

Just tty->ops->...(); ?

> +}



> +int serdev_tty_port_register(struct tty_port *port, struct device
> *parent,
> +			    struct tty_driver *drv, int idx)
> +{
> +	struct serdev_controller *ctrl;
> +	struct serport *serport;
> +	int ret;
> +
> +	if (!port || !drv || !parent || !parent->of_node)

And if it's ACPI? Perhaps last is redundant.

> +		return -ENODEV;
> +
> +	ctrl = serdev_controller_alloc(parent, sizeof(struct
> serport));
> +	if (!ctrl)
> +		return -ENOMEM;
> +	serport = serdev_controller_get_drvdata(ctrl);
> +
> +	mutex_init(&serport->lock);
> +	serport->port = port;
> +	serport->tty_idx = idx;
> +	serport->tty_drv = drv;
> +
> +	ctrl->ops = &ctrl_ops;
> +
> +	ret = serdev_controller_add(ctrl);
> +	if (ret)
> +		goto err;
> +
> +	printk(KERN_INFO "serdev: Serial port %s\n", drv->name);

Hmm... It's not a debug message, why not use pr_info()?

> +	return 0;
> +
> +err:

err_controller_put: ?

> +	serdev_controller_put(ctrl);
> +	return ret;
> +}
> +
> +void serdev_tty_port_unregister(struct tty_port *port)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> 

> +	if (!serport)
> +		return;

Same question, whose responsibility to do this?


+
> +#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
> +int serdev_tty_port_register(struct tty_port *port, struct device
> *parent,
> +			    struct tty_driver *drv, int idx);
> +void serdev_tty_port_unregister(struct tty_port *port);
> +#else
> +static inline int serdev_tty_port_register(struct tty_port *port,
> +					   struct device *parent,
> +					   struct tty_driver *drv,
> int idx)
> +{
> +	return -ENODEV;
> +}
> +static inline void serdev_tty_port_unregister(struct tty_port *port)
> {}

> +#endif

Perhaps comment to see from which if this one.

> +
>  #endif

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy