Re: [PATCH v3 43/44] tty/metag_da: Add metag DA TTY driver

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 10026 bytes --]

Hi Jiri,

On 10/01/13 16:03, Jiri Slaby wrote:
> Hi, few comments/questions below.
> 
> On 01/10/2013 04:31 PM, James Hogan wrote:
>> --- /dev/null
>> +++ b/drivers/tty/metag_da.c
>> @@ -0,0 +1,696 @@
> ...
>> +struct dashtty_port {
>> +	struct tty_port		 port;
>> +	spinlock_t		 rx_lock;
>> +	void			*rx_buf;
>> +	struct mutex		 xmit_lock;
>> +	unsigned int		 xmit_cnt;
>> +	unsigned int		 xmit_head;
>> +	unsigned int		 xmit_tail;
>> +	struct completion	 xmit_empty;
>> +};
>> +
>> +static struct dashtty_port dashtty_ports[NUM_TTY_CHANNELS];
> 
>> +static int chancall(int bios_function, int channel, int arg2, void *arg3,
>> +			void *arg4)
>> +{
>> +	register int   bios_function__ __asm__("D1Ar1") = bios_function;
>> +	register int   channel__       __asm__("D0Ar2") = channel;
>> +	register int   arg2__          __asm__("D1Ar3") = arg2;
>> +	register void *arg3__          __asm__("D0Ar4") = arg3;
>> +	register void *arg4__          __asm__("D1Ar5") = arg4;
>> +	register int   bios_call       __asm__("D0Ar6") = 3;
>> +	register int   result          __asm__("D0Re0");
> 
> This is really ugly. Couldn't it be specified in the asm's constraints
> below?

Unfortunately not. There aren't any constraints for these individual
registers. It could be made to use u64's with shifting and add/or'ing
but then the generated code is pretty bad. Using struct {int lo,hi;}'s
produces better code (none of the registers actually need moving from
their argument positions) but honestly isn't any less ugly.

> 
>> +	__asm__ volatile (
>> +		"SETL   [A0StP++], %6,%5\n\t"
>> +		"SETL   [A0StP++], %4,%3\n\t"
>> +		"SETL   [A0StP++], %2,%1\n\t"

Using specific registers also allows these three to be combined into a
single MSETL instruction... I'll do that.

>> +		"ADD    A0StP, A0StP, #8\n\t"
>> +		"SWITCH #0x0C30208\n\t"
>> +		"GETD   %0, [A0StP+#-8]\n\t"
>> +		"SUB    A0StP, A0StP, #(4*6)+8\n\t"
>> +		: "=r" (result)/* outs */
>> +		: "r" (bios_function__), "r" (channel__), "r" (arg2__),
>> +		  "r" (arg3__), "r" (arg4__), "r" (bios_call) /* ins */
>> +		: "memory");
>> +
>> +	return result;
>> +}
> ...
>> +static int dashtty_port_activate(struct tty_port *port, struct tty_struct *tty)
>> +{
>> +	struct dashtty_port *dport = container_of(port, struct dashtty_port,
>> +						  port);
>> +	void *rx_buf;
>> +
>> +	/* Allocate the buffer we use for writing data */
>> +	if (tty_port_alloc_xmit_buf(port) < 0)
>> +		goto err;
>> +
>> +	/* Allocate the buffer we use for reading data */
>> +	rx_buf = kzalloc(RX_BUF_SIZE, GFP_KERNEL);
>> +	if (!rx_buf)
>> +		goto err_free_xmit;
>> +
>> +	spin_lock_bh(&dport->rx_lock);
>> +	dport->rx_buf = rx_buf;
>> +	spin_unlock_bh(&dport->rx_lock);
>> +
>> +	return 0;
>> +err_free_xmit:
>> +	tty_port_free_xmit_buf(port);
>> +err:
>> +	return -ENOMEM;
>> +}
>> +
>> +static void dashtty_port_shutdown(struct tty_port *port)
>> +{
>> +	struct dashtty_port *dport = container_of(port, struct dashtty_port,
>> +						  port);
>> +	void *rx_buf;
>> +	unsigned int count;
>> +
>> +	mutex_lock(&dport->xmit_lock);
>> +	count = dport->xmit_cnt;
>> +	mutex_unlock(&dport->xmit_lock);
>> +	if (count) {
>> +		/*
>> +		 * There's still data to write out, so wake and wait for the
>> +		 * writer thread to drain the buffer.
>> +		 */
>> +		del_timer(&put_timer);
>> +		wake_up_interruptible(&dashtty_waitqueue);
>> +		wait_for_completion(&dport->xmit_empty);
>> +	}
>> +
>> +	/* Null the read buffer (timer could still be running!) */
>> +	spin_lock_bh(&dport->rx_lock);
>> +	rx_buf = dport->rx_buf;
>> +	dport->rx_buf = NULL;
>> +	spin_unlock_bh(&dport->rx_lock);
>> +	/* Free the read buffer */
>> +	kfree(rx_buf);
>> +
>> +	/* Free the write buffer */
>> +	tty_port_free_xmit_buf(port);
>> +}
>> +
>> +static const struct tty_port_operations dashtty_port_ops = {
>> +	.activate	= dashtty_port_activate,
>> +	.shutdown	= dashtty_port_shutdown,
>> +};
>> +
>> +static int dashtty_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> +	struct dashtty_port *port;
>> +	int ret, line;
>> +
>> +	if (!tty)
>> +		return -ENODEV;
> 
> This will never happen.

Agreed, I thought I'd removed these but must've missed them.

> 
>> +	line = tty->index;
>> +
>> +	if (line < 0 || line >= NUM_TTY_CHANNELS)
>> +		return -ENODEV;
> 
> Neither this.

Agreed.

> 
>> +	port = &dashtty_ports[line];
>> +
>> +	port->port.ops = &dashtty_port_ops;
> 
> You can do that along with tty_port_init in the module_init function.

Yes, good idea.

> 
>> +	ret = tty_port_install(&port->port, driver, tty);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Don't add the poll timer if we're opening a console. This
>> +	 * avoids the overhead of polling the Dash but means it is not
>> +	 * possible to have a login on /dev/console.
>> +	 *
>> +	 */
>> +	if (line != CONSOLE_CHANNEL)
>> +		if (atomic_inc_return(&num_channels_need_poll) == 1)
>> +			add_poll_timer(&poll_timer);
> 
> This should be in activate I suppose. You don't need install/cleanup at
> all I beleive.

Yes, I've moved it in there (but with s/line != CONSOLE_CHANNEL/dport !=
&dashtty_ports[CONSOLE_CHANNEL]/). That just leaves a one line install
callback which just passes the relevant tty_port to tty_port_install.

> 
>> +	return 0;
>> +}
>> +
>> +static void dashtty_cleanup(struct tty_struct *tty)
>> +{
>> +	int line;
>> +
>> +	if (!tty)
>> +		return;
> 
> The same as above.
> 
>> +	line = tty->index;
>> +
>> +	if (line < 0 || line >= NUM_TTY_CHANNELS)
>> +		return;
> 
> Detto.
> 
>> +	if (line != CONSOLE_CHANNEL)
>> +		if (atomic_dec_and_test(&num_channels_need_poll))
>> +			del_timer_sync(&poll_timer);
> 
> To shutdown. No need for cleanup.

agreed, done.

>> +static int dashtty_write_room(struct tty_struct *tty)
>> +{
>> +	struct dashtty_port *dport;
>> +	int channel;
>> +	int room;
>> +
>> +	channel = FIRST_TTY_CHANNEL + tty->index;
> 
> FIRST_TTY_CHANNEL doesn't make much sense. Better to be removed completely.

Agreed, I'm not sure what this was originally for but I've removed it now.

> 
>> +	dport = &dashtty_ports[channel];
>> +
>> +	/* report the space in the xmit buffer */
>> +	mutex_lock(&dport->xmit_lock);
>> +	room = SERIAL_XMIT_SIZE - dport->xmit_cnt;
>> +	mutex_unlock(&dport->xmit_lock);
>> +
>> +	return room;
>> +}
> ...
>> +static int __init dashtty_init(void)
>> +{
>> +	int ret;
>> +	int nport;
>> +	struct dashtty_port *dport;
>> +
>> +	if (!metag_da_enabled())
>> +		return -ENODEV;
>> +
>> +	channel_driver = tty_alloc_driver(NUM_TTY_CHANNELS, 0);
> 
> This should be s/0/TTY_DRIVER_REAL_RAW/.

Neat, thanks

> 
>> +	if (IS_ERR(channel_driver))
>> +		return PTR_ERR(channel_driver);
>> +
>> +	channel_driver->owner = THIS_MODULE;
> 
> No need to set this.

Yes, I see it's passed into __tty_alloc_driver automatically.

> 
>> +	channel_driver->driver_name = "ttyDA";
> 
> This should be rather "metag".

Yes, I'll set to "metag_da" if that's okay since the transport is the
DA. I presume this is pretty much just to appear in /proc/tty/drivers?

> 
>> +	channel_driver->name = "ttyDA";
>> +	channel_driver->major = DA_TTY_MAJOR;
>> +	channel_driver->minor_start = 0;
>> +	channel_driver->type = TTY_DRIVER_TYPE_SERIAL;
>> +	channel_driver->subtype = SERIAL_TYPE_NORMAL;
>> +	channel_driver->init_termios = tty_std_termios;
>> +	channel_driver->init_termios.c_cflag |= CLOCAL;
>> +	channel_driver->flags = TTY_DRIVER_REAL_RAW;
> 
> And this is unnecessary now.
> 
>> +
>> +	tty_set_operations(channel_driver, &dashtty_ops);
>> +	for (nport = 0; nport < NUM_TTY_CHANNELS; nport++) {
>> +		dport = &dashtty_ports[nport];
>> +		tty_port_init(&dport->port);
>> +		spin_lock_init(&dport->rx_lock);
>> +		mutex_init(&dport->xmit_lock);
>> +		/* the xmit buffer starts empty, i.e. completely written */
>> +		init_completion(&dport->xmit_empty);
>> +		complete(&dport->xmit_empty);
>> +	}
>> +
>> +	init_timer(&put_timer);
>> +	put_timer.function = dashtty_put_timer;
> 
> setup_timer()

Thanks, I've also changed the other timer to use this too

> 
>> +	init_waitqueue_head(&dashtty_waitqueue);
>> +	dashtty_thread = kthread_create(put_data, NULL, "ttyDA");
>> +	if (IS_ERR(dashtty_thread)) {
>> +		pr_err("Couldn't create dashtty thread\n");
>> +		ret = PTR_ERR(dashtty_thread);
>> +		goto err_free_driver;
>> +	}
>> +	kthread_bind(dashtty_thread, 0);
> 
> This deserves a comment why you bind it to CPU0...

Okay, comment added:
+	/*
+	 * Bind the writer thread to the boot CPU so it can't migrate.
+	 * DA channels are per-CPU and we want all channel I/O to be on a single
+	 * predictable CPU.
+	 */

> 
>> +	wake_up_process(dashtty_thread);
>> +
>> +	ret = tty_register_driver(channel_driver);
>> +
>> +	if (ret < 0) {
>> +		pr_err("Couldn't install dashtty driver: err %d\n",
>> +		       ret);
>> +		goto err_stop_kthread;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_stop_kthread:
>> +	kthread_stop(dashtty_thread);
>> +err_free_driver:
>> +	put_tty_driver(channel_driver);
>> +	return ret;
>> +}
>> +
>> +static void dashtty_exit(void)
>> +{
>> +	del_timer_sync(&put_timer);
>> +	kthread_stop(dashtty_thread);
>> +	del_timer_sync(&poll_timer);
>> +	tty_unregister_driver(channel_driver);
>> +	put_tty_driver(channel_driver);
> 
> No tty_port_destroy anywhere?

Ah yes, I've added a loop to do this before put_tty_driver() both here
and in the error handling of dashtty_init().

This driver isn't actually buildable as a module at the moment, but if
it was, would the module owner being set ensure that the ttys/ports are
closed down prior to dashtty_exit being called? (i.e. is it racy?)

> 
>> +}
>> +
>> +module_init(dashtty_init);
>> +module_exit(dashtty_exit);
> 
> thanks,
> 

Thanks for reviewing,

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]