Skip to contentSkip to conversation

rtc: cmos: avoid taking rtc_lock for extended period of time

PATCH In review Dmitry Torokhov 2 participants 3 messages Oct 24, 2024 – Oct 25, 2024 Real time clock (RTC)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rtc: cmos: avoid taking rtc_lock for extended period of time

On my device reading entirety of /sys/devices/pnp0/00:03/cmos_nvram0/nvmem takes about 9 msec during which time interrupts are off on the CPU that does the read and the thread that performs the read can not be migrated or preempted by another higher priority thread (RT or not).

Allow readers and writers be preempted by taking and releasing rtc_lock spinlock for each individual byte read or written rather than once per read/write request.

---
 drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
drivers/rtc/rtc-cmos.c +15 -16
index 35dca2accbb8..e8f2fe0d8560 100644
@@ -645,18 +645,17 @@ static int cmos_nvram_read(void *priv, unsigned int off, void *val,
 	unsigned char *buf = val;
 
 	off += NVRAM_OFFSET;
-	spin_lock_irq(&rtc_lock);
-	for (; count; count--, off++) {
+	for (; count; count--, off++, buf++) {
+		guard(spinlock_irq)(&rtc_lock);
 		if (off < 128)
-			*buf++ = CMOS_READ(off);
+			*buf = CMOS_READ(off);
 		else if (can_bank2)
-			*buf++ = cmos_read_bank2(off);
+			*buf = cmos_read_bank2(off);
 		else
-			break;
+			return -EIO;
 	}
-	spin_unlock_irq(&rtc_lock);
 
-	return count ? -EIO : 0;
+	return count;
 }
 
 static int cmos_nvram_write(void *priv, unsigned int off, void *val,
@@ -671,23 +670,23 @@ static int cmos_nvram_write(void *priv, unsigned int off, void *val,
 	 * NVRAM to update, updating checksums is also part of its job.
 	 */
 	off += NVRAM_OFFSET;
-	spin_lock_irq(&rtc_lock);
-	for (; count; count--, off++) {
+	for (; count; count--, off++, buf++) {
 		/* don't trash RTC registers */
 		if (off == cmos->day_alrm
 				|| off == cmos->mon_alrm
 				|| off == cmos->century)
-			buf++;
-		else if (off < 128)
-			CMOS_WRITE(*buf++, off);
+			continue;
+
+		guard(spinlock_irq)(&rtc_lock);
+		if (off < 128)
+			CMOS_WRITE(*buf, off);
 		else if (can_bank2)
-			cmos_write_bank2(*buf++, off);
+			cmos_write_bank2(*buf, off);
 		else
-			break;
+			return -EIO;
 	}
-	spin_unlock_irq(&rtc_lock);
 
-	return count ? -EIO : 0;
+	return count;
 }
 
 /*----------------------------------------------------------------*/
Signature · 5 lines
2.47.0.163.g1226f6d8fa-goog


-- 
Dmitry
#1 lore raw

Dnia 24 października 2024 22:37:08 CEST, Dmitry Torokhov <[email protected]> napisał/a:

Quoted 8 lines

On my device reading entirety of /sys/devices/pnp0/00:03/cmos_nvram0/nvmem takes about 9 msec during which time interrupts are off on the CPU that does the read and the thread that performs the read can not be migrated or preempted by another higher priority thread (RT or not).

Allow readers and writers be preempted by taking and releasing rtc_lock spinlock for each individual byte read or written rather than once per read/write request.

Hello,
A nice idea!
(sorry for any formatting problems, I'm on a train right now)

Quoted 22 lines

---
drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------

1 file changed, 15 insertions(+), 16 deletions(-)
drivers/rtc/rtc-cmos.c +3 -3
index 35dca2accbb8..e8f2fe0d8560 100644
@@ -645,18 +645,17 @@ static int cmos_nvram_read(void *priv, unsigned int off, void *val,
 	unsigned char *buf = val;
 
 	off += NVRAM_OFFSET;
-	spin_lock_irq(&rtc_lock);
-	for (; count; count--, off++) {
+	for (; count; count--, off++, buf++) {
+		guard(spinlock_irq)(&rtc_lock);
 		if (off < 128)
-			*buf++ = CMOS_READ(off);
+			*buf = CMOS_READ(off);
 		else if (can_bank2)

drivers/rtc/rtc-cmos.c static int cmos_nvram_read(void *priv, unsigned int off, void *val, · line 658

Quoted 10 lines
drivers/rtc/rtc-cmos.c +3 -4
-			*buf++ = cmos_read_bank2(off);
+			*buf = cmos_read_bank2(off);
 		else
-			break;
+			return -EIO;
 	}
-	spin_unlock_irq(&rtc_lock);
 
-	return count ? -EIO : 0;
+	return count;

return 0;

when you are at it.

Quoted 23 lines
drivers/rtc/rtc-cmos.c +6 -5
 }
 
 static int cmos_nvram_write(void *priv, unsigned int off, void *val,
@@ -671,23 +670,23 @@ static int cmos_nvram_write(void *priv, unsigned int off, void *val,
 	 * NVRAM to update, updating checksums is also part of its job.
 	 */
 	off += NVRAM_OFFSET;
-	spin_lock_irq(&rtc_lock);
-	for (; count; count--, off++) {
+	for (; count; count--, off++, buf++) {
 		/* don't trash RTC registers */
 		if (off == cmos->day_alrm
 				|| off == cmos->mon_alrm
 				|| off == cmos->century)
-			buf++;
-		else if (off < 128)
-			CMOS_WRITE(*buf++, off);
+			continue;
+
+		guard(spinlock_irq)(&rtc_lock);
+		if (off < 128)
+			CMOS_WRITE(*buf, off);
 		else if (can_bank2)

drivers/rtc/rtc-cmos.c static int cmos_nvram_write(void *priv, unsigned int off, void *val, · line 689

Quoted 10 lines
drivers/rtc/rtc-cmos.c +3 -4
-			cmos_write_bank2(*buf++, off);
+			cmos_write_bank2(*buf, off);
 		else
-			break;
+			return -EIO;
 	}
-	spin_unlock_irq(&rtc_lock);
 
-	return count ? -EIO : 0;
+	return count;

return 0;

Quoted 3 lines
drivers/rtc/rtc-cmos.c +0 -0
 }
 
 /*----------------------------------------------------------------*/
#2 lore raw
Quoted 44 lines · Mateusz Jończyk

Dnia 24 października 2024 22:37:08 CEST, Dmitry Torokhov <[email protected]> napisał/a:

Quoted 8 lines

On my device reading entirety of /sys/devices/pnp0/00:03/cmos_nvram0/nvmem takes about 9 msec during which time interrupts are off on the CPU that does the read and the thread that performs the read can not be migrated or preempted by another higher priority thread (RT or not).

Allow readers and writers be preempted by taking and releasing rtc_lock spinlock for each individual byte read or written rather than once per read/write request.

Hello,
A nice idea!
(sorry for any formatting problems, I'm on a train right now)

Quoted 30 lines

---
drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------

1 file changed, 15 insertions(+), 16 deletions(-)
drivers/rtc/rtc-cmos.c +5 -6
index 35dca2accbb8..e8f2fe0d8560 100644
@@ -645,18 +645,17 @@ static int cmos_nvram_read(void *priv, unsigned int off, void *val,
 	unsigned char *buf = val;
 
 	off += NVRAM_OFFSET;
-	spin_lock_irq(&rtc_lock);
-	for (; count; count--, off++) {
+	for (; count; count--, off++, buf++) {
+		guard(spinlock_irq)(&rtc_lock);
 		if (off < 128)
-			*buf++ = CMOS_READ(off);
+			*buf = CMOS_READ(off);
 		else if (can_bank2)
-			*buf++ = cmos_read_bank2(off);
+			*buf = cmos_read_bank2(off);
 		else
-			break;
+			return -EIO;
 	}
-	spin_unlock_irq(&rtc_lock);
 
Quoted 4 lines

drivers/rtc/rtc-cmos.c static int cmos_nvram_read(void *priv, unsigned int off, void *val, · line 658

Quoted 2 lines
drivers/rtc/rtc-cmos.c +1 -1
-	return count ? -EIO : 0;
+	return count;

return 0;

Oh, yes, of course, thank you.

Signature · 1 line
Dmitry