Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 9 Jul 2018 10:30:22 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Evan Green <evgreen@chromium.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org, swboyd@chromium.org
Subject: Re: [PATCH] pinctrl: msm: Pass along set_wake failures
Message-ID: <20180709173022.GH2050@tuxbook-pro>
References: <20180619234349.166190-1-evgreen@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180619234349.166190-1-evgreen@chromium.org>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue 19 Jun 16:43 PDT 2018, Evan Green wrote:

> The MSM pinctrl driver quietly swallows errors that occur
> when trying to call .irq_set_wake. It should instead pass
> those failures up the chain so the caller can react to them.
> Swallowing the error for instance causes gpio_keys to think that
> it was able to successfully set a wake IRQ, when in fact it may
> not have been, causing the following warning on resume:
> 
> [   53.777819] Unbalanced IRQ 9 wake disable
> [   53.781979] WARNING: CPU: 0 PID: 1362 at kernel/irq/manage.c:623
> irq_set_irq_wake+0xac/0x12c
> [   53.794758] Modules linked in: spi_gpio spi_bitbang qcom_q6v5_pil
> qcom_common cfg80211 ip6table_filter smsc95xx usbnet mii
> [   54.016419] [<ffffff80081054b0>] irq_set_irq_wake+0xac/0x12c
> [   54.022252] [<ffffff80083c86a4>] msm_gpio_irq_set_wake+0x48/0x68
> [   54.028447] [<ffffff8008104d8c>] set_irq_wake_real+0x50/0x5c
> [   54.034275] [<ffffff80081054d0>] irq_set_irq_wake+0xcc/0x12c
> [   54.040104] [<ffffff800860fd00>] gpio_keys_resume+0x74/0xd8
> [   54.045846] [<ffffff800852018c>] platform_pm_resume+0x54/0x60
> [   54.051771] [<ffffff800852ceb4>] dpm_run_callback+0x104/0x210
> [   54.057694] [<ffffff800852d7cc>] device_resume+0x178/0x1b0
> [   54.063355] [<ffffff800852e938>] dpm_resume+0x1c4/0x38c
> [   54.068745] [<ffffff800852efac>] dpm_resume_end+0x20/0x34
> [   54.074315] [<ffffff80080fe700>] suspend_devices_and_enter+0x518/0x964
> [   54.081044] [<ffffff80080ff1dc>] pm_suspend+0x690/0x6e0
> [   54.086433] [<ffffff80080fd0f8>] state_store+0xd4/0xf8
> [   54.091733] [<ffffff80088a3af0>] kobj_attr_store+0x18/0x28
> [   54.097396] [<ffffff80082980a4>] sysfs_kf_write+0x5c/0x68
> [   54.102961] [<ffffff8008297050>] kernfs_fop_write+0x174/0x1b8
> [   54.108887] [<ffffff800821a9c0>] __vfs_write+0x58/0x160
> [   54.114276] [<ffffff800821acd4>] vfs_write+0xcc/0x184
> [   54.119487] [<ffffff800821af4c>] SyS_write+0x64/0xb4
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..d48a74ddbc1f 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -779,14 +779,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>  	unsigned long flags;
> +	int rc;
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	irq_set_irq_wake(pctrl->irq, on);
> +	rc = irq_set_irq_wake(pctrl->irq, on);
>  
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  
> -	return 0;
> +	return rc;

Sorry for not getting back to you in a timely manner Evan, I wanted to
read up more on the details of how this is supposed to work. I still
haven't done so, but here's my concern:

When we power down the SoC we're no longer powering either the TLMM or
the GIC, so the MPM or PDC is used to waking the system on some set of
triggers. As such set_wake on an individual pin or irq should be routed
to the MPM/PDC driver, which (in the PDC case) is implemented using
hierarchical irq domains.

As such I think that we shouldn't toggle the wake property of the
summary pin at all; i.e. the patch should remove that call rather than
propagating what I believe is a constant failure.

Regards,
Bjorn