| Message ID | 20180619234349.166190-1-evgreen@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Wed, Jun 20, 2018 at 1:45 AM Evan Green <evgreen@chromium.org> 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> Björn is this patch OK with you? Seems fine to me. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 9, 2018 at 10:27 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > 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 Hi Bjorn, That's okay, I always feel bad pinging. Thanks for the thoughtful response. Stephen and I are starting to think about how wake interrupts should work with regard to the PDC, and we're at a place where we're a bit unsure of the path forward. Our understanding is the downstream kernel had an interrupt hierarchy of GIC > PDC > TLMM & everybody else. In the downstream world PDC acted transparently, forwarding most requests directly onto the GIC, but quietly handling wake interrupts as well. With the upstream PDC driver, the #interrupt-cells got changed to 2, and it seemed like folks didn't like the idea that PDC was acting transparently. Correct me if I'm wrong there. So now we're sort of unsure about how to wire in PDC. If we make everybody an interrupt child of PDC, then we lose the ability to specify the third GIC parameter, and we're stuck expressing interrupts with respect to PDC pins, which is an awkward mental translation. In this world, does TLMM need to do direct-connect stuff to get wake-able GPIO interrupts working? It would kind of have a foot in both worlds, with its summary interrupt as a GIC interrupt but the wakeable ones as parented by PDC? So anyway, with regard to this patch, I'm happy to create a second spin that simply removes this function, but for me at least it brought up some larger questions we've been wrestling with. -Evan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10 2018 at 12:53 -0600, Evan Green wrote: >On Mon, Jul 9, 2018 at 10:27 AM Bjorn Andersson ><bjorn.andersson@linaro.org> wrote: >> >> 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 > >Hi Bjorn, >That's okay, I always feel bad pinging. Thanks for the thoughtful >response. Stephen and I are starting to think about how wake >interrupts should work with regard to the PDC, and we're at a place >where we're a bit unsure of the path forward. > >Our understanding is the downstream kernel had an interrupt hierarchy >of GIC > PDC > TLMM & everybody else. In the downstream world PDC >acted transparently, forwarding most requests directly onto the GIC, >but quietly handling wake interrupts as well. With the upstream PDC >driver, the #interrupt-cells got changed to 2, and it seemed like >folks didn't like the idea that PDC was acting transparently. Correct >me if I'm wrong there. So now we're sort of unsure about how to wire >in PDC. If we make everybody an interrupt child of PDC, then we lose >the ability to specify the third GIC parameter, and we're stuck >expressing interrupts with respect to PDC pins, which is an awkward >mental translation. Its an unfortunate side effect of the design. Drivers will have to request the PDC pin for wakeup IRQs. >In this world, does TLMM need to do direct-connect >stuff to get wake-able GPIO interrupts working? It would kind of have >a foot in both worlds, with its summary interrupt as a GIC interrupt >but the wakeable ones as parented by PDC? > With GPIOs, I am trying to hack the TLMM driver to request a PDC pin, when the IRQ associated with the GPIO is requested as a wakeup interrupt. In that case, the TLMM driver would do the GPIO->PDC pin translation and request it. There is no hierarchy between TLMM and PDC. Will try a RFC patch in a week or two. -- Lina >So anyway, with regard to this patch, I'm happy to create a second >spin that simply removes this function, but for me at least it brought >up some larger questions we've been wrestling with. >-Evan >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 1:38 PM Lina Iyer <ilina@codeaurora.org> wrote: > > On Tue, Jul 10 2018 at 12:53 -0600, Evan Green wrote: > >On Mon, Jul 9, 2018 at 10:27 AM Bjorn Andersson > ><bjorn.andersson@linaro.org> wrote: > >> > >> 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 > > > >Hi Bjorn, > >That's okay, I always feel bad pinging. Thanks for the thoughtful > >response. Stephen and I are starting to think about how wake > >interrupts should work with regard to the PDC, and we're at a place > >where we're a bit unsure of the path forward. > > > >Our understanding is the downstream kernel had an interrupt hierarchy > >of GIC > PDC > TLMM & everybody else. In the downstream world PDC > >acted transparently, forwarding most requests directly onto the GIC, > >but quietly handling wake interrupts as well. With the upstream PDC > >driver, the #interrupt-cells got changed to 2, and it seemed like > >folks didn't like the idea that PDC was acting transparently. Correct > >me if I'm wrong there. So now we're sort of unsure about how to wire > >in PDC. If we make everybody an interrupt child of PDC, then we lose > >the ability to specify the third GIC parameter, and we're stuck > >expressing interrupts with respect to PDC pins, which is an awkward > >mental translation. > Its an unfortunate side effect of the design. Drivers will have to > request the PDC pin for wakeup IRQs. Would they use the PDC pin to request their regular interrupt, and the PDC would turn around and ask the GIC for them, and also enable the wakeup interrupt? Or would devices have some sort of separate entry for wakeup interrupts? > > >In this world, does TLMM need to do direct-connect > >stuff to get wake-able GPIO interrupts working? It would kind of have > >a foot in both worlds, with its summary interrupt as a GIC interrupt > >but the wakeable ones as parented by PDC? > > > With GPIOs, I am trying to hack the TLMM driver to request a PDC pin, > when the IRQ associated with the GPIO is requested as a wakeup > interrupt. In that case, the TLMM driver would do the GPIO->PDC pin > translation and request it. There is no hierarchy between TLMM and PDC. > > Will try a RFC patch in a week or two. Thanks Lina. Looking forward to it. Please CC me. > > -- Lina > > >So anyway, with regard to this patch, I'm happy to create a second > >spin that simply removes this function, but for me at least it brought > >up some larger questions we've been wrestling with. Bjorn, Linus, If there's a spin of this you'd like to see, I'm happy to provide that. If not, that's fine too. -Evan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12 2018 at 10:31 -0600, Evan Green wrote: >On Tue, Jul 10, 2018 at 1:38 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> On Tue, Jul 10 2018 at 12:53 -0600, Evan Green wrote: >> >On Mon, Jul 9, 2018 at 10:27 AM Bjorn Andersson >> ><bjorn.andersson@linaro.org> wrote: >> >Our understanding is the downstream kernel had an interrupt hierarchy >> >of GIC > PDC > TLMM & everybody else. In the downstream world PDC >> >acted transparently, forwarding most requests directly onto the GIC, >> >but quietly handling wake interrupts as well. With the upstream PDC >> >driver, the #interrupt-cells got changed to 2, and it seemed like >> >folks didn't like the idea that PDC was acting transparently. Correct >> >me if I'm wrong there. So now we're sort of unsure about how to wire >> >in PDC. If we make everybody an interrupt child of PDC, then we lose >> >the ability to specify the third GIC parameter, and we're stuck >> >expressing interrupts with respect to PDC pins, which is an awkward >> >mental translation. >> Its an unfortunate side effect of the design. Drivers will have to >> request the PDC pin for wakeup IRQs. > >Would they use the PDC pin to request their regular interrupt, and the >PDC would turn around and ask the GIC for them, and also enable the >wakeup interrupt?> Yes, drivers would need to request a PDC pin since using the interrupts-extended format and then enable that interrupt was a wakeup interrupt. > Or would devices have some sort of separate entry for wakeup > interrupts? Not sure how you mean. If it's the DT you are asking, then yes, they would need to have a separate entry in DT. wake-device { interrupts-extended = <&pdc 2 IRQ_TYPE_LEVEL_HIGH>; }; See Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } static void msm_gpio_irq_handler(struct irq_desc *desc)
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(-)