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

[Sachi King]

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@xxxxxxx>

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.