[RFC PATCH 6/5] CELL rtas console port to hvc_console backend driver

[Milton Miller]

On Dec 8, 2005, at 4:31 AM, David Woodhouse wrote:

> --- linux-2.6.14/drivers/char/hvc_rtas.c~	2005-12-07 
> 18:12:59.000000000 +0100
> +++ linux-2.6.14/drivers/char/hvc_rtas.c	2005-12-07 18:15:39.000000000 
> +0100
> @@ -0,0 +1,161 @@
> +/*
> + * IBM RTAS driver interface to hvc_console.c
> + *
> + * (C) Copyright IBM Corporation 2001-2005
> + * (C) Copyright Red Hat, Inc. 2005
> + *
> + * Author(s): Maximino Augilar <IBM STI Design Center>
> + *          : Ryan S. Arnold <rsa at us.ibm.com>
> + *          : Utz Bacher <utz.bacher at de.ibm.com>
> + *          : David Woodhouse <dwmw2 at infradead.org>
> + *
> + *    inspired by drivers/char/hvc_console.c
> + *    written by Anton Blanchard and Paul Mackerras
> + *
> + * This program is free software; you can redistribute it and/or 
> modify
> + * it under the terms of the GNU General Public License as published 
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  
> 02111-1307 USA
> + */
> +
> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <asm/rtas.h>
> +#include <asm/irq.h>
> +#include "hvc_console.h"
> +
> +static uint32_t hvc_rtas_vtermno = 0;
> +struct hvc_struct *hvc_rtas_dev;
> +
> +#define RTASCONS_PUT_ATTEMPTS  16
> +
> +static int rtascons_put_char_token = -1;
> +static int rtascons_get_char_token = -1;
> +static int rtascons_put_delay;

rtas.h includes #define RTAS_UNKNOWN_SERVICE (-1)
how about using that?

> +
> +static inline int hvc_rtas_write_console(uint32_t vtermno, const char 
> *buf, int count)
> +{
> +       int result = 0;
> +       int attempts = RTASCONS_PUT_ATTEMPTS;
> +       int done = 0;
> +
> +       /* if there is more than one character to be displayed, wait a 
> bit */
> +       for (; done < count && attempts; udelay(rtascons_put_delay)) {
> +               attempts--;

double - ?

> +               result = rtas_call(rtascons_put_char_token, 1, 1, 
> NULL, buf[done]);
> +
> +               if (!result) {
> +			attempts = RTASCONS_PUT_ATTEMPTS;
> +			done++;
> +		}
> +       }

The above loop delays after every character, regardless of intermediate
success, or having more than one character as indicated.

Also, I would move the initial attempts initialization to the first
clause of the for.  also done.

Perhaps move udelay to else of !result and attempts to the loop closure 
(or just postfix --- on the && to avoid off-by-one).


> +	/* the calling routine expects to receive the number of bytes sent */
> +	return done?:result;

Is the result a sane thing to return? Do we know it will be < 0?
How about -1 instead.  And then result doesn't need a initial 
assignment (do we care about calling this with count 0?)

> +}
> +
> +static inline int rtascons_get_char(void)
> +{
> +       int result;
> +
> +       if (rtas_call(rtascons_get_char_token, 0, 2, &result))
> +               result = -1;
> +
> +       return result;
> +}
> +
> +static int hvc_rtas_read_console(uint32_t vtermno, char *buf, int 
> count)
> +{
> +	unsigned long got;
> +	int c;	
> +	int i;
> +
> +	for (got = 0, i = 0; i < count; i++) {
> +	
> +		if (( c = rtascons_get_char() ) != -1) {
> +			buf[i] = c;
> +			++got;
> +		}
> +		else
> +			break;
> +	}
> +	return got;
> +}
> +
> +static struct hv_ops hvc_rtas_get_put_ops = {
> +	.get_chars = hvc_rtas_read_console,
> +	.put_chars = hvc_rtas_write_console,
> +};
> +
> +static int hvc_rtas_init(void)
> +{
> +	struct hvc_struct *hp;
> +
> +	if (rtascons_put_char_token == -1)
> +		rtascons_put_char_token = rtas_token("put-term-char");
> +	if (rtascons_put_char_token == -1)
> +		return -EIO;
> +
> +	if (rtascons_get_char_token == -1)
> +		rtascons_get_char_token = rtas_token("get-term-char");
> +	if (rtascons_get_char_token == -1)
> +		return -EIO;

RTAS_UNKNOWN_SERVICE

> +
> +	if (__onsim())
> +		rtascons_put_delay = 0;
> +	else
> +		rtascons_put_delay = 100;
> +

hmm... should this delay be a (writable) module parameter?

currently we get no delay until the tty subsystem initializes, which 
could lead to a lossy console during boot (albeit a faster boot).

> +	BUG_ON(hvc_rtas_dev);
> +
> +	/* Allocate an hvc_struct for the console device we instantiated
> +	 * earlier.  Save off hp so that we can return it on exit */
> +	hp = hvc_alloc(hvc_rtas_vtermno, NO_IRQ, &hvc_rtas_get_put_ops);
> +	if (IS_ERR(hp))
> +		return PTR_ERR(hp);
> +	hvc_rtas_dev = hp;
> +	return 0;
> +}
> +module_init(hvc_rtas_init);
> +
> +/* This will tear down the tty portion of the driver */
> +static void __exit hvc_rtas_exit(void)
> +{
> +	struct hvc_struct *hp_safe;
> +	/* Hopefully this isn't premature */
> +	if (!hvc_rtas_dev)
> +		return;
> +
> +	hp_safe = hvc_rtas_dev;
> +	hvc_rtas_dev = NULL;
> +

There seems to be a lot of defensive programming around the storing of 
this cookie (test for null on init, move to safe, etc).  Since there
is no interrupt callback involved I don't see the need.

> +	/* Really the fun isn't over until the worker thread breaks down and 
> the
> +	 * tty cleans up */
> +	hvc_remove(hp_safe);
> +}
> +module_exit(hvc_rtas_exit); /* before drivers/char/hvc_console.c */
> +
> +/* This will happen prior to module init.  There is no tty at this 
> time? */
> +static int hvc_rtas_console_init(void)
> +{
> +	rtascons_put_char_token = rtas_token("put-term-char");
> +	if (rtascons_put_char_token == -1)
> +		return -EIO;
> +	rtascons_get_char_token = rtas_token("get-term-char");
> +	if (rtascons_get_char_token == -1)
> +		return -EIO;

RTAS_UNKNOWN_SERVICE

> +
> +	hvc_instantiate(hvc_rtas_vtermno, 0, &hvc_rtas_get_put_ops );
> +	return 0;
> +}

Since (I assume) there is no cookie in the device tree, define a magic 
number instead of this never-set static variable.

The purpose is to match the driver data between console and tty init.
If all drivers use the same number there will be confusion if two 
drivers think they can init.

> +console_initcall(hvc_rtas_console_init);
> --- linux-2.6.14/drivers/char/Makefile~	2005-12-07 17:47:05.000000000 
> +0100
> +++ linux-2.6.14/drivers/char/Makefile	2005-12-07 18:12:07.000000000 
> +0100
> @@ -43,6 +43,7 @@ obj-$(CONFIG_RIO)		+= rio/ generic_seria
>  obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
>  obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
>  obj-$(CONFIG_HVC_FSS)		+= hvc_fss.o
> +obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
>  obj-$(CONFIG_RAW_DRIVER)	+= raw.o
>  obj-$(CONFIG_SGI_SNSC)		+= snsc.o snsc_event.o
>  obj-$(CONFIG_MMTIMER)		+= mmtimer.o
> --- linux-2.6.14/drivers/char/Kconfig~	2005-12-07 17:47:05.000000000 
> +0100
> +++ linux-2.6.14/drivers/char/Kconfig	2005-12-07 18:17:14.000000000 
> +0100
> @@ -575,6 +575,13 @@ config HVC_FSS
>  	  IBM Full System Simulator Console device driver which makes use of
>  	  the HVC_DRIVER front end.
>
> +config HVC_RTAS
> +	bool "IBM RTAS Console support"
> +	depends on PPC_RTAS
> +	select HVC_DRIVER
> +	help
> +	  IBM Console device driver which makes use of RTAS
> +
>  config HVCS
>  	tristate "IBM Hypervisor Virtual Console Server support"
>  	depends on PPC_PSERIES
>
> -- 
> dwmw2
>

milton