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

From: Andrew Morton
Date: Fri Sep 12 2008 - 14:56:23 EST


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

> From: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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;
> + }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/