[PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time

Dmitry Torokhov posted 1 patch 1 month ago
There is a newer version of this series
drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
[PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
Posted by Dmitry Torokhov 1 month ago
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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 35dca2accbb8..e8f2fe0d8560 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -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;
 }
 
 /*----------------------------------------------------------------*/
-- 
2.47.0.163.g1226f6d8fa-goog


-- 
Dmitry
Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
Posted by Mateusz Jończyk 1 month ago
Dnia 24 października 2024 22:37:08 CEST, Dmitry Torokhov <dmitry.torokhov@gmail.com> napisał/a:
>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) 

>
>Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>---
> drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
>index 35dca2accbb8..e8f2fe0d8560 100644
>--- a/drivers/rtc/rtc-cmos.c
>+++ b/drivers/rtc/rtc-cmos.c
>@@ -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;

return 0;

when you are at it. 

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

return 0;

> }
> 
> /*----------------------------------------------------------------*/
Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
Posted by Dmitry Torokhov 1 month ago
On Fri, Oct 25, 2024 at 08:01:37AM +0200, Mateusz Jończyk wrote:
> Dnia 24 października 2024 22:37:08 CEST, Dmitry Torokhov <dmitry.torokhov@gmail.com> napisał/a:
> >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) 
> 
> >
> >Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >---
> > drivers/rtc/rtc-cmos.c | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> >index 35dca2accbb8..e8f2fe0d8560 100644
> >--- a/drivers/rtc/rtc-cmos.c
> >+++ b/drivers/rtc/rtc-cmos.c
> >@@ -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;
> 
> return 0;

Oh, yes, of course, thank you.

-- 
Dmitry