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

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

[David Brownell]

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
+
+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;
 	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;
+	}
 	spin_unlock_irq(&rtc_lock);
 
 	return retval;
@@ -532,6 +578,8 @@ cmos_nvram_write(struct kobject *kobj, s
 	cmos = dev_get_drvdata(container_of(kobj, struct device, kobj));
 	if (unlikely(off >= attr->size))
 		return -EFBIG;
+	if (unlikely(off < 0))
+		return -EINVAL;
 	if ((off + count) > attr->size)
 		count = attr->size - off;
 
@@ -540,15 +588,20 @@ cmos_nvram_write(struct kobject *kobj, s
 	 * here.  If userspace is smart enough to know what fields of
 	 * NVRAM to update, updating checksums is also part of its job.
 	 */
+	off += NVRAM_OFFSET;
 	spin_lock_irq(&rtc_lock);
-	for (retval = 0, off += NVRAM_OFFSET; count--; retval++, off++) {
+	for (retval = 0; count; count--, off++, retval++) {
 		/* don't trash RTC registers */
 		if (off == cmos->day_alrm
 				|| off == cmos->mon_alrm
 				|| off == cmos->century)
 			buf++;
-		else
+		else if (off < 128)
 			CMOS_WRITE(*buf++, off);
+		else if (can_bank2)
+			cmos_write_bank2(*buf++, off);
+		else
+			break;
 	}
 	spin_unlock_irq(&rtc_lock);
 
@@ -651,8 +704,8 @@ cmos_do_probe(struct device *dev, struct
 
 	/* Heuristic to deduce NVRAM size ... do what the legacy NVRAM
 	 * driver did, but don't reject unknown configs.   Old hardware
-	 * won't address 128 bytes, and for now we ignore the way newer
-	 * chips can address 256 bytes (using two more i/o ports).
+	 * won't address 128 bytes.  Newer chips have multiple banks,
+	 * though they may not be listed in one I/O resource.
 	 */
 #if	defined(CONFIG_ATARI)
 	address_space = 64;
@@ -662,6 +715,8 @@ cmos_do_probe(struct device *dev, struct
 #warning Assuming 128 bytes of RTC+NVRAM address space, not 64 bytes.
 	address_space = 128;
 #endif
+	if (can_bank2 && ports->end > (ports->start + 1))
+		address_space = 256;
 
 	/* For ACPI systems extension info comes from the FADT.  On others,
 	 * board specific setup provides it as appropriate.  Systems where
@@ -776,13 +831,14 @@ cmos_do_probe(struct device *dev, struct
 		goto cleanup2;
 	}
 
-	pr_info("%s: alarms up to one %s%s, %s irqs\n",
+	pr_info("%s: alarms up to one %s%s, %zd bytes nvram, %s irqs\n",
 			cmos_rtc.rtc->dev.bus_id,
 			cmos_rtc.mon_alrm
 				? "year"
 				: (cmos_rtc.day_alrm
 					? "month" : "day"),
 			cmos_rtc.century ? ", y3k" : "",
+			nvram.size,
 			cmos_rtc.set_filter
 				? IRQ_FILTER_TYPE
 				: (is_valid_irq(rtc_irq)

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

[Andrew Morton]

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;
> +	}

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

[David Brownell]

On Friday 12 September 2008, Andrew Morton wrote:
> > +#define can_bank2	true
> 
> It would be more idiomatic to make this upper-case: CAN_BANK2.

Think of it as:  "static const bool can_bank2 = true;" ...


> : 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 was chasing down a problem related to offsets.  Now fixed;
that bit could probably go away ... except that so long as the
interface allows negative/bogus values, it seems appropriate to
defend against them.  If the interface passed an unsigned type,
my paranoia-meter would not fire in this case.

 
> 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 ;)

In this case, size over 256 would be impossible.  A relevant
case study may be:

  http://thedailywtf.com/Articles/Rule-Number-One.aspx

:)