Re: [patch 2.6.27 mmotm] rtc-cmos: export second NVRAM bank

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 12 Sep 2008 09:57:55 -0700
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Teach rtc-cmos about the second bank of registers found on most
> modern x86 systems, giving access to 128 bytes more NVRAM.
> 
> This version only sees that extra NVRAM when both register banks
> are provided as part of *one* PNP resource.  Since BIOS on some
> systems presents them using two IO resources, and nothing merges
> them, this can't always show all the NVRAM.  (We're supposed to
> be able to use PNP id PNP0b01 too, but BIOS tables doesn't often
> seem to use that particular option.)
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> For 2.6.28; applies after other pending patches
> 
>  drivers/rtc/rtc-cmos.c |   70 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 7 deletions(-)
> 
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -153,6 +153,43 @@ static inline int hpet_unregister_irq_ha
>  
>  /*----------------------------------------------------------------*/
>  
> +#ifdef RTC_PORT
> +
> +/* Most newer x86 systems have two register banks, the first used
> + * for RTC and NVRAM and the second only for NVRAM.  Caller must
> + * own rtc_lock ... and we won't worry about access during NMI.
> + */
> +#define can_bank2	true

It would be more idiomatic to make this upper-case: CAN_BANK2.

> +static inline unsigned char cmos_read_bank2(unsigned char addr)
> +{
> +	outb(addr, RTC_PORT(2));
> +	return inb(RTC_PORT(3));
> +}
> +
> +static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
> +{
> +	outb(addr, RTC_PORT(2));
> +	outb(val, RTC_PORT(2));
> +}
> +
> +#else
> +
> +#define can_bank2	false
> +
> +static inline unsigned char cmos_read_bank2(unsigned char addr)
> +{
> +	return 0;
> +}
> +
> +static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
> +{
> +}
> +
> +#endif
> +
> +/*----------------------------------------------------------------*/
> +
>  static int cmos_read_time(struct device *dev, struct rtc_time *t)
>  {
>  	/* REVISIT:  if the clock has a "century" register, use
> @@ -511,12 +548,21 @@ cmos_nvram_read(struct kobject *kobj, st
>  
>  	if (unlikely(off >= attr->size))
>  		return 0;
> +	if (unlikely(off < 0))
> +		return -EINVAL;
>  	if ((off + count) > attr->size)
>  		count = attr->size - off;
> +	off += NVRAM_OFFSET;

hm, now what's happening in here.

: static ssize_t
: cmos_nvram_read(struct kobject *kobj, struct bin_attribute *attr,
: 		char *buf, loff_t off, size_t count)
: {
: 	int	retval;
: 
: 	if (unlikely(off >= attr->size))
: 		return 0;
: 	if (unlikely(off < 0))
: 		return -EINVAL;
: 	if ((off + count) > attr->size)
: 		count = attr->size - off;
: 
: 	off += NVRAM_OFFSET;
: 

The VFS will (hopefully) prevent ->read methods from being called with
a negative file offset.  What prompted the additional test for that?

I did't look at it exhaustively but I suspect that the above code won't
work right if attr->size has a value of around (2^31 - 42) and `offset'
is (2^31 - 54) and NVRAM_OFFSET==54.  Or something like that.  It looks
holey ;)

Of course, the assumption that attr->size is not insanely large is a
good one, but still..

>  	spin_lock_irq(&rtc_lock);
> -	for (retval = 0, off += NVRAM_OFFSET; count--; retval++, off++)
> -		*buf++ = CMOS_READ(off);
> +	for (retval = 0; count; count--, off++, retval++) {
> +		if (off < 128)
> +			*buf++ = CMOS_READ(off);
> +		else if (can_bank2)
> +			*buf++ = cmos_read_bank2(off);
> +		else
> +			break;
> +	}