Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ
@ 2020-01-23 13:14 Andy Shevchenko
  2020-01-23 13:14 ` [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-01-23 13:14 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Andy Shevchenko, Guilherme G . Piccoli, Hans de Goede

As reported by Guilherme G. Piccoli:

---8<---8<---8<---

The rtc-cmos interrupt setting was changed in the commit 079062b28fb4
("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order
to allow shared interrupts; according to that commit's description,
some machine got kernel warnings due to the interrupt line being shared
between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ sharing
that time.

After the aforementioned commit though it was observed a huge increase
in lost HPET interrupts in some systems, observed through the following
kernel message:

[...] hpet1: lost 35 rtc interrupts

After investigation, it was narrowed down to the shared interrupts
usage when having the kernel option "irqpoll" enabled. In this case,
all IRQ handlers are called for non-timer interrupts, if such handlers
are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
message after doing work - lots of readl/writel to HPET registers, which
are known to be slow.

Although "irqpoll" is not a default kernel option, it's used in some contexts,
one being the kdump kernel (which is an already "impaired" kernel usually
running with 1 CPU available), so the performance burden could be considerable.
Also, the same issue would happen (in a shorter extent though) when using
"irqfixup" kernel option.

In a quick experiment, a virtual machine with uptime of 2 minutes produced
>300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
sharing interrupts this number reduced to 1 interrupt. Machines with more
hardware than a VM should generate even more unnecessary HPET interrupts
in this scenario.

---8<---8<---8<---

After looking into the rtc-cmos driver history and DSDT table from
the Microsoft Surface 3, we may notice that Hans de Goede submitted
a correct fix (see dependency below). Thus, we simply revert
the culprit commit.

Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch")
Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs")
Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: Added tags, rework scissors to avoid cutting commit message (Guilherme)
 drivers/rtc/rtc-cmos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 033303708c8b..cb28bbdc9e17 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 			rtc_cmos_int_handler = cmos_interrupt;
 
 		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
-				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
+				0, dev_name(&cmos_rtc.rtc->dev),
 				cmos_rtc.rtc);
 		if (retval < 0) {
 			dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86
  2020-01-23 13:14 [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
@ 2020-01-23 13:14 ` Andy Shevchenko
  2020-01-25 20:56   ` Alexandre Belloni
  2020-01-23 13:14 ` [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
  2020-01-25 20:56 ` [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Alexandre Belloni
  2 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-01-23 13:14 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Andy Shevchenko, Hans de Goede, Guilherme G . Piccoli

When legacy devices are present on x86 machine, the RTC IRQ has
a dedicated pre-defined value. Use it instead of hard coded number.

Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: Added tags
 drivers/rtc/rtc-cmos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index cb28bbdc9e17..aee55b658f85 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -1305,7 +1305,7 @@ static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		 * hardcode it on systems with a legacy PIC.
 		 */
 		if (nr_legacy_irqs())
-			irq = 8;
+			irq = RTC_IRQ;
 #endif
 		return cmos_do_probe(&pnp->dev,
 				pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper
  2020-01-23 13:14 [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
  2020-01-23 13:14 ` [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
@ 2020-01-23 13:14 ` Andy Shevchenko
  2020-01-25 20:57   ` Alexandre Belloni
  2020-01-25 20:56 ` [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Alexandre Belloni
  2 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-01-23 13:14 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Andy Shevchenko, Hans de Goede, Guilherme G . Piccoli

Refactor code by using the new dmi_get_bios_year() helper instead of
open coding its functionality. This also makes logic slightly clearer.

No changes intended.

Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: Added tags
 drivers/rtc/rtc-cmos.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index aee55b658f85..b795fe4cbd2e 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -1197,8 +1197,6 @@ static void rtc_wake_off(struct device *dev)
 /* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */
 static void use_acpi_alarm_quirks(void)
 {
-	int year;
-
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
 		return;
 
@@ -1208,8 +1206,10 @@ static void use_acpi_alarm_quirks(void)
 	if (!is_hpet_enabled())
 		return;
 
-	if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) && year >= 2015)
-		use_acpi_alarm = true;
+	if (dmi_get_bios_year() < 2015)
+		return;
+
+	use_acpi_alarm = true;
 }
 #else
 static inline void use_acpi_alarm_quirks(void) { }
-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-23 13:14 [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
  2020-01-23 13:14 ` [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
  2020-01-23 13:14 ` [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
@ 2020-01-25 20:56 ` Alexandre Belloni
  2 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2020-01-25 20:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alessandro Zummo, linux-rtc, Guilherme G . Piccoli, Hans de Goede

On 23/01/2020 15:14:35+0200, Andy Shevchenko wrote:
> As reported by Guilherme G. Piccoli:
> 
> ---8<---8<---8<---
> 
> The rtc-cmos interrupt setting was changed in the commit 079062b28fb4
> ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order
> to allow shared interrupts; according to that commit's description,
> some machine got kernel warnings due to the interrupt line being shared
> between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ sharing
> that time.
> 
> After the aforementioned commit though it was observed a huge increase
> in lost HPET interrupts in some systems, observed through the following
> kernel message:
> 
> [...] hpet1: lost 35 rtc interrupts
> 
> After investigation, it was narrowed down to the shared interrupts
> usage when having the kernel option "irqpoll" enabled. In this case,
> all IRQ handlers are called for non-timer interrupts, if such handlers
> are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
> hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
> message after doing work - lots of readl/writel to HPET registers, which
> are known to be slow.
> 
> Although "irqpoll" is not a default kernel option, it's used in some contexts,
> one being the kdump kernel (which is an already "impaired" kernel usually
> running with 1 CPU available), so the performance burden could be considerable.
> Also, the same issue would happen (in a shorter extent though) when using
> "irqfixup" kernel option.
> 
> In a quick experiment, a virtual machine with uptime of 2 minutes produced
> >300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
> sharing interrupts this number reduced to 1 interrupt. Machines with more
> hardware than a VM should generate even more unnecessary HPET interrupts
> in this scenario.
> 
> ---8<---8<---8<---
> 
> After looking into the rtc-cmos driver history and DSDT table from
> the Microsoft Surface 3, we may notice that Hans de Goede submitted
> a correct fix (see dependency below). Thus, we simply revert
> the culprit commit.
> 
> Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch")
> Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs")
> Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> v2: Added tags, rework scissors to avoid cutting commit message (Guilherme)
>  drivers/rtc/rtc-cmos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86
  2020-01-23 13:14 ` [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
@ 2020-01-25 20:56   ` Alexandre Belloni
  0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2020-01-25 20:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alessandro Zummo, linux-rtc, Hans de Goede, Guilherme G . Piccoli

On 23/01/2020 15:14:36+0200, Andy Shevchenko wrote:
> When legacy devices are present on x86 machine, the RTC IRQ has
> a dedicated pre-defined value. Use it instead of hard coded number.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> v2: Added tags
>  drivers/rtc/rtc-cmos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper
  2020-01-23 13:14 ` [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
@ 2020-01-25 20:57   ` Alexandre Belloni
  0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2020-01-25 20:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alessandro Zummo, linux-rtc, Hans de Goede, Guilherme G . Piccoli

On 23/01/2020 15:14:37+0200, Andy Shevchenko wrote:
> Refactor code by using the new dmi_get_bios_year() helper instead of
> open coding its functionality. This also makes logic slightly clearer.
> 
> No changes intended.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> v2: Added tags
>  drivers/rtc/rtc-cmos.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 13:14 [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
2020-01-23 13:14 ` [PATCH v2 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
2020-01-25 20:56   ` Alexandre Belloni
2020-01-23 13:14 ` [PATCH v2 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
2020-01-25 20:57   ` Alexandre Belloni
2020-01-25 20:56 ` [PATCH v2 1/3] rtc: cmos: Stop using shared IRQ Alexandre Belloni

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git