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:
signature.asc
Description: OpenPGP digital signature
