Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sachi King <nakato@nakato.io>
To:     linux-gpio@vger.kernel.org, basavaraj.natikar@amd.com,
        "Limonciello, Mario" <mario.limonciello@amd.com>
Cc:     linux-kernel@vger.kernel.org,
        Shyam-sundar S-k <Shyam-sundar.S-k@amd.com>,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@amd.com>
Subject: Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe
Date:   Sat, 09 Oct 2021 14:42:58 +1100
Message-ID: <1647640.3P7QB8EEvc@youmu>
In-Reply-To: <8cf02fe2-252d-02b5-d227-9091bee57f76@amd.com>
References: <20211001161714.2053597-1-nakato@nakato.io> <20211001161714.2053597-2-nakato@nakato.io> <8cf02fe2-252d-02b5-d227-9091bee57f76@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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@amd.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.