linux-kernel - Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Jan 2017 20:04:37 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org, "Luis R . Rodriguez" <mcgrof@...nel.org>
Subject: Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

On Mon, Jan 16, 2017 at 07:23:45PM +0200, Andy Shevchenko wrote:
> Legacy RTC requires interrupt line 8 to be dedicated for it. On
> Intel MID platforms the legacy PIC is absent and in order to make RTC
> work we need to allocate interrupt separately.
> 
> Current solution brought by the commit
> 
>   82a51c38f199 ("x86/platform/intel-mid: Enable RTC on Intel Merrifield")

Referring to commits on linux-next gitsums is not proper as they change
every day, so you might as well leave them out.

> does it in a wrong place, 

There are other things wrong with it.. It had:

diff --git a/arch/x86/platform/intel-mid/mrfld.c b/arch/x86/platform/intel-mid/mrfld.c
index e0607c77a1bd..ae7bdeb0e507 100644
--- a/arch/x86/platform/intel-mid/mrfld.c
+++ b/arch/x86/platform/intel-mid/mrfld.c
@@ -91,6 +91,7 @@ static unsigned long __init tangier_calibrate_tsc(void)
 static void __init tangier_arch_setup(void)
 {
        x86_platform.calibrate_tsc = tangier_calibrate_tsc;
+       x86_platform.legacy.rtc = 1;
 }
 
 /* tangier arch ops */

You want to set quirks by setting the x86_platform.set_legacy_features given
then on x86_early_init_platform_quirks() we have:

        if (x86_platform.set_legacy_features)                                   
                x86_platform.set_legacy_features();  

For example see xen_dom0_set_legacy_features().

> and since it's done unconditionally for all
> x86 devices, some of them, e.g. PNP based, might get it wrong -- at the
> beginning x86_platform.legacy.rtc flag is set for all x86 devices.

Doing it the above way would also make it a quirk only for the needed
devices.

> 
> Move interrupt allocation to arch/x86/kernel/rtc.c module and allocate
> it for Intel MID platform devices only.
> 
> Fixes: 82a51c38f199 ("x86/platform/intel-mid: Enable RTC on Intel Merrifield")
> Cc: Luis R. Rodriguez <mcgrof@...nel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> - add headers and #ifdef to make kbuild bot happy
> - re-test on PNP platform with acpi=off, thus, update patch accordingly
> 
>  arch/x86/kernel/rtc.c             | 21 +++++++++++++++++++++
>  arch/x86/platform/intel-mid/sfi.c | 14 --------------
>  2 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 5b21cb7d84d6..bf4cb88b9186 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -12,7 +12,9 @@
>  #include <asm/vsyscall.h>
>  #include <asm/x86_init.h>
>  #include <asm/time.h>
> +#include <asm/hw_irq.h>
>  #include <asm/intel-mid.h>
> +#include <asm/io_apic.h>
>  #include <asm/setup.h>
>  
>  #ifdef CONFIG_X86_32
> @@ -155,6 +157,20 @@ void read_persistent_clock(struct timespec *ts)
>  	x86_platform.get_wallclock(ts);
>  }
>  
> +#ifdef CONFIG_X86_IO_APIC
> +static __init int allocate_rtc_cmos_irq(void)
> +{
> +	struct irq_alloc_info info;
> +
> +	if (!intel_mid_identify_cpu())
> +	       return 0;
> +
> +	ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
> +	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
> +}
> +#else
> +static inline int allocate_rtc_cmos_irq(void) { return 0; }
> +#endif
>  
>  static struct resource rtc_resources[] = {
>  	[0] = {
> @@ -178,6 +194,7 @@ static struct platform_device rtc_device = {
>  
>  static __init int add_rtc_cmos(void)
>  {
> +	int ret;
>  #ifdef CONFIG_PNP
>  	static const char * const ids[] __initconst =
>  	    { "PNP0b00", "PNP0b01", "PNP0b02", };
> @@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
>  	if (!x86_platform.legacy.rtc)
>  		return -ENODEV;
>  
> +	ret = allocate_rtc_cmos_irq();
> +	if (ret < 0)
> +		return ret;
> +

Ugh this is seriously ugly, can't we avoid this sort of thing with the
callback and then let the internal MID code do what it needs?

   Luis