* [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
@ 2024-10-24 20:37 Dmitry Torokhov
2024-10-25 6:01 ` Mateusz Jończyk
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2024-10-24 20:37 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Mateusz Jończyk, Mario Limonciello, Dan Carpenter,
Joy Chakraborty, linux-rtc, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
2024-10-24 20:37 [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time Dmitry Torokhov
@ 2024-10-25 6:01 ` Mateusz Jończyk
2024-10-25 20:07 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Mateusz Jończyk @ 2024-10-25 6:01 UTC (permalink / raw)
To: Dmitry Torokhov, Alexandre Belloni
Cc: Mario Limonciello, Dan Carpenter, Joy Chakraborty, linux-rtc,
linux-kernel
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;
> }
>
> /*----------------------------------------------------------------*/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
2024-10-25 6:01 ` Mateusz Jończyk
@ 2024-10-25 20:07 ` Dmitry Torokhov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2024-10-25 20:07 UTC (permalink / raw)
To: Mateusz Jończyk
Cc: Alexandre Belloni, Mario Limonciello, Dan Carpenter,
Joy Chakraborty, linux-rtc, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
@ 2024-10-25 20:14 Dmitry Torokhov
2024-10-25 20:52 ` Mateusz Jończyk
2024-10-31 23:19 ` Alexandre Belloni
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2024-10-25 20:14 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Mateusz Jończyk, Mario Limonciello, Dan Carpenter,
Joy Chakraborty, linux-rtc, linux-kernel
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>
---
v2: use "return 0;" instead of "return count;" (which should be the same
behavior-wise because count is 0 by that point) to show intent
clearer (per feedback from Mateusz Jończyk)
v1: https://lore.kernel.org/r/Zxqv9KYnBdtnuqox@google.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..5849d2970bba 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 0;
}
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 0;
}
/*----------------------------------------------------------------*/
--
2.47.0.163.g1226f6d8fa-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
2024-10-25 20:14 Dmitry Torokhov
@ 2024-10-25 20:52 ` Mateusz Jończyk
2024-10-31 23:19 ` Alexandre Belloni
1 sibling, 0 replies; 6+ messages in thread
From: Mateusz Jończyk @ 2024-10-25 20:52 UTC (permalink / raw)
To: Dmitry Torokhov, Alexandre Belloni
Cc: Mario Limonciello, Dan Carpenter, Joy Chakraborty, linux-rtc,
linux-kernel
W dniu 25.10.2024 o 22:14, Dmitry Torokhov pisze:
> 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>
Reviewed-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
FWIW, there is a similar latency problem in hpet_rtc_interrupt() in arch/x86/kernel/hpet.c as mc146818_get_time() can sleep for up to 10ms. It should be possible to avoid using full
mc146818_get_time() there and implement only what's needed (including aborting early if RTC_UIP is set etc.).
Greetings,
Mateusz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time
2024-10-25 20:14 Dmitry Torokhov
2024-10-25 20:52 ` Mateusz Jończyk
@ 2024-10-31 23:19 ` Alexandre Belloni
1 sibling, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2024-10-31 23:19 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mateusz Jończyk, Mario Limonciello, Dan Carpenter,
Joy Chakraborty, linux-rtc, linux-kernel
On Fri, 25 Oct 2024 13:14:57 -0700, Dmitry Torokhov wrote:
> 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.
>
> [...]
Applied, thanks!
[1/1] rtc: cmos: avoid taking rtc_lock for extended period of time
https://git.kernel.org/abelloni/c/0a6efab33eab
Best regards,
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-31 23:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-24 20:37 [PATCH] rtc: cmos: avoid taking rtc_lock for extended period of time Dmitry Torokhov
2024-10-25 6:01 ` Mateusz Jończyk
2024-10-25 20:07 ` Dmitry Torokhov
-- strict thread matches above, loose matches on Subject: below --
2024-10-25 20:14 Dmitry Torokhov
2024-10-25 20:52 ` Mateusz Jończyk
2024-10-31 23:19 ` Alexandre Belloni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).