[PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

["Guilherme G. Piccoli" <gpiccoli@canonical.com>]

The rtc-cmos interrupt setting was changed in 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.

This patch changes this behavior by preventing shared interrupts if the
HPET-based IRQ handler is used instead of the regular cmos_interrupt()
one. 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.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---


Hi all, thanks for reading/reviewing this patch! One of the
alternatives I considered in case sharing interrupts are really
desirable is a new kernel parameter to rtc-cmos to allow
sharing interrupts, and default the IRQ setup to non-shared.

We could also disable sharing if "irqpoll" or "irqfixup" is set,
but this would somewhat "bypass" IRQ code API which I think would
be a bit ugly.

Please let me know your thoughts, and thanks in advance.

Guilherme


 drivers/rtc/rtc-cmos.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 033303708c8b..16416154eb00 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -710,6 +710,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 	unsigned char			rtc_control;
 	unsigned			address_space;
 	u32				flags = 0;
+	unsigned long			irq_flags;
 	struct nvmem_config nvmem_cfg = {
 		.name = "cmos_nvram",
 		.word_size = 1,
@@ -839,6 +840,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 
 		if (use_hpet_alarm()) {
 			rtc_cmos_int_handler = hpet_rtc_interrupt;
+			irq_flags = 0;
 			retval = hpet_register_irq_handler(cmos_interrupt);
 			if (retval) {
 				hpet_mask_rtc_irq_bit(RTC_IRQMASK);
@@ -846,11 +848,13 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 						" failed in rtc_init().");
 				goto cleanup1;
 			}
-		} else
+		} else {
 			rtc_cmos_int_handler = cmos_interrupt;
+			irq_flags = IRQF_SHARED;
+		}
 
 		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
-				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
+				irq_flags, 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.0