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:   Tue, 17 Jan 2017 00:44:09 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

On Mon, Jan 16, 2017 at 11:00 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Mon, 16 Jan 2017, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 20:04 +0100, Luis R. Rodriguez wrote:
>> > Referring to commits on linux-next gitsums is not proper as they
>> > change
>> > every day, so you might as well leave them out.
>>
>> It's in maintainer's tree which has it the same. And most probably it
>> would go the same (since commit includes timestamps). I never heard
>> before of such issues (unless someone *rebased* tree for linux-next).
>
> Which is obviously the case. tip x86/platform has:
>
> de1c2540aa4f: x86/platform/intel-mid: Enable RTC on Intel Merrifield

Oh, didn't realize that. Thanks for pointing out.

>> ...also I have no idea when exactly it will happen during
>> initialization.
>
> That's a non argument. You really can figure that out ....
>
> Like you could have figured out that calling that function unconditially is
> not a brilliant idea....

Yes, that's correct. Just had no time to do that quickly.

>> I agree that's not nice looking piece of code, but this due to absence
>> of some stubs. IOAPIC code is really old one and misses stuff (proper
>> error codes, stubs for no IOAPIC case).
>
> That's a fixable problem and in no way a justification for horrible
> hackery.

I see your proposal below. Indeed it looks much cleaner.
Thanks!

> +static void __init intel_mid_legacy_rtc_init(void)
> +{
> +       struct irq_alloc_info info;
> +
> +       ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
> +       if (mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info)) {

I will fix this (> 0 is okay, = 0 I have to check) and give it a try.

> +               pr_info("Failed to allocate RTC interrupt. Disabling RTC\n");
> +               x86_platform.legacy.rtc = 0;
> +       }
> +}

> -static int __init intel_mid_legacy_rtc_init(void)
> -{
> -       struct irq_alloc_info info;
> -
> -       if (!x86_platform.legacy.rtc)
> -               return -ENODEV;
> -
> -       ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
> -       return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
> -}
> -
>  static int __init intel_mid_platform_init(void)
>  {
> -       intel_mid_legacy_rtc_init();
> -
>         sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
>         sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
>         return 0;

By the way, would be better to revert or dismiss that commit at all?

-- 
With Best Regards,
Andy Shevchenko