linux-kernel - Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

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]

Message-ID: <1647640.3P7QB8EEvc@youmu>
Date:   Sat, 09 Oct 2021 14:42:58 +1100
From:   Sachi King <nakato@...ato.io>
To:     linux-gpio@...r.kernel.org, basavaraj.natikar@....com,
        "Limonciello, Mario" <mario.limonciello@....com>
Cc:     linux-kernel@...r.kernel.org,
        Shyam-sundar S-k <Shyam-sundar.S-k@....com>,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>
Subject: Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

On Saturday, 9 October 2021 05:19:17 AEDT Limonciello, Mario wrote:
> On 10/1/2021 11:17, Sachi King wrote:
> > Some systems such as the Microsoft Surface Laptop 4 leave interrupts
> > enabled and configured for use in sleep states on boot, which cause
> > unexpected behaviour such as spurious wakes and failed resumes in
> > s2idle states.
> > 
> > As interrupts should not be enabled until they are claimed and
> > explicitly enabled, disabling any interrupts mistakenly left enabled by
> > firmware should be safe.
> > 
> 
> So I did test this on a handful of platforms and confirmed that the 
> events declared in _AEI are still being enabled and passed properly.
> 
> So if no other changes needed you can add my:
> Tested-by: Mario Limonciello <mario.limonciello@....com>

I've sent a second version of the patch to remove the duplicate
INTERRUPT_MASK_OFF.  I also fixed a number of style and whitespace issues.

Would you like to test this again?


> > diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> > index c001f2ed20f8..aa4136cd312d 100644
> > --- a/drivers/pinctrl/pinctrl-amd.c
> > +++ b/drivers/pinctrl/pinctrl-amd.c
> > @@ -967,6 +993,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
> >   		return PTR_ERR(gpio_dev->pctrl);
> >   	}
> >   
> > +	/* Disable and mask interrupts */
> > +	amd_gpio_irq_init(gpio_dev);
> > +
> 
> As the pinctrl device was just registered, I do wonder if this actually 
> needs a mutex in case another thread tries to enable the pins at the 
> same time.  I might be wrong here though and things are OK because the 
> pin range isn't added until later on in probe.

If we need to add a mutex then I think it would be more safe to rework
the amd_gpio_init_irq function to not depend on amd_gpio and move it
before devm_pinctrl_register, as I'm not sure a mutex around
amd_gpio_irq_init would save us from a race condition occurring between the
devm_pinctrl_register and getting the mutex following that. I'm hoping to
avoid that however as it would be a bit messy without pin_desc_get.