Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 22-01-2020 17:57, Hans de Goede wrote:
> Hi,
> 
> On 22-01-2020 15:45, Andy Shevchenko wrote:
>> As reported by Guilherme G. Piccoli:
>>
>> --- 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< ---
>>
>> 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>
> 
> Entire series looks good to me:
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

That should have been:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Of course, sorry.

Regards,

Hans


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