* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:01 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-18 17:01 UTC (permalink / raw)
To: linux-arm-kernel
+ Tony
On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..c2b973c738fe 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -36,6 +36,7 @@
> #include <linux/pci_ids.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
>
> @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> -
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> * @rockchip: PCIe port information
> @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wake");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
Did you test that this works out correctly as a level-triggered
interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
keep firing. See:
static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
{
struct wake_irq *wirq = _wirq;
int res;
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
}
...
This could cause some kind of an IRQ storm, including a lockup or
significant slowdown, I think.
BTW, in another context, Tony suggested we might need to fix up the IRQ flags
like this:
int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
{
...
err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
- IRQF_ONESHOT, dev_name(dev), wirq);
+ IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
But IIUC, that's not actually necessary, because __setup_irq()
automatically configures the trigger type if the driver didn't request
one explicitly.
Brian
> + if (err)
> + dev_err(dev, "failed to setup PCIe wake IRQ\n");
> + }
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> +
> pci_stop_root_bus(rockchip->root_bus);
> pci_remove_root_bus(rockchip->root_bus);
> pci_unmap_iospace(rockchip->io);
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:01 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-18 17:01 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, bhelgaas, shawn.lin, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel, Tony Lindgren
+ Tony
On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..c2b973c738fe 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -36,6 +36,7 @@
> #include <linux/pci_ids.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
>
> @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> -
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> * @rockchip: PCIe port information
> @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wake");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
Did you test that this works out correctly as a level-triggered
interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
keep firing. See:
static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
{
struct wake_irq *wirq = _wirq;
int res;
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
}
...
This could cause some kind of an IRQ storm, including a lockup or
significant slowdown, I think.
BTW, in another context, Tony suggested we might need to fix up the IRQ flags
like this:
int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
{
...
err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
- IRQF_ONESHOT, dev_name(dev), wirq);
+ IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
But IIUC, that's not actually necessary, because __setup_irq()
automatically configures the trigger type if the driver didn't request
one explicitly.
Brian
> + if (err)
> + dev_err(dev, "failed to setup PCIe wake IRQ\n");
> + }
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> +
> pci_stop_root_bus(rockchip->root_bus);
> pci_remove_root_bus(rockchip->root_bus);
> pci_unmap_iospace(rockchip->io);
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 17:01 ` Brian Norris
(?)
@ 2017-08-18 17:07 ` Brian Norris
-1 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-18 17:07 UTC (permalink / raw)
To: Jeffy Chen
Cc: Heiko Stuebner, Tony Lindgren, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
On Fri, Aug 18, 2017 at 10:01:07AM -0700, Brian Norris wrote:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
Oops, I forgot you sent a v3. Same questions/comments apply there
though.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread
* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:07 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-18 17:07 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 18, 2017 at 10:01:07AM -0700, Brian Norris wrote:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
Oops, I forgot you sent a v3. Same questions/comments apply there
though.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:07 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-18 17:07 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, bhelgaas, shawn.lin, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel, Tony Lindgren
On Fri, Aug 18, 2017 at 10:01:07AM -0700, Brian Norris wrote:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
Oops, I forgot you sent a v3. Same questions/comments apply there
though.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 17:01 ` Brian Norris
(?)
@ 2017-08-18 17:47 ` jeffy
-1 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 17:47 UTC (permalink / raw)
To: Brian Norris
Cc: Heiko Stuebner, Tony Lindgren, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
Hi Brian,
On 08/19/2017 01:01 AM, Brian Norris wrote:
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
hmmm, right, but as i replied at cros partner issue, this irq handle
might not be called actually...
in my test on cros 4.4 kernel, it would break by irq_may_run(returning
false):
static bool irq_may_run(struct irq_desc *desc)
{
...
/*
* If the interrupt is an armed wakeup source, mark it pending
* and suspended, disable it and notify the pm core about the
* event.
*/
if (irq_pm_check_wakeup(desc))
return false;
bool irq_pm_check_wakeup(struct irq_desc *desc)
{
if (irqd_is_wakeup_armed(&desc->irq_data)) {
irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
desc->depth++;
irq_disable(desc); <--- disabled here
pm_system_irq_wakeup(irq_desc_get_irq(desc));
return true;
and for irqd_is_wakeup_armed:
static bool suspend_device_irq(struct irq_desc *desc)
{
...
if (irqd_is_wakeup_set(&desc->irq_data)) {
irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); <-- set
irqd_is_wakeup_armed here
void dpm_noirq_begin(void)
{
cpuidle_pause();
device_wakeup_arm_wake_irqs();
suspend_device_irqs();
so unless we get an irq between device_wakeup_arm_wake_irqs and
suspend_device_irq, the irq_pm_check_wakeup would not let us get to
handle_threaded_wake_irq...
>
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
actually this would not work...irq_get_trigger_type would return zero
due to a bug which we have a patch for it already:
9908207 New [tip:irq/urgent] genirq: Restore trigger settings
in irq_modify_status()
BTW, using dev_name for the name of this wake irq seems not very
convenient...maybe add a ":wake" suffix?
>
> Brian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:47 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 17:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Brian,
On 08/19/2017 01:01 AM, Brian Norris wrote:
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
hmmm, right, but as i replied at cros partner issue, this irq handle
might not be called actually...
in my test on cros 4.4 kernel, it would break by irq_may_run(returning
false):
static bool irq_may_run(struct irq_desc *desc)
{
...
/*
* If the interrupt is an armed wakeup source, mark it pending
* and suspended, disable it and notify the pm core about the
* event.
*/
if (irq_pm_check_wakeup(desc))
return false;
bool irq_pm_check_wakeup(struct irq_desc *desc)
{
if (irqd_is_wakeup_armed(&desc->irq_data)) {
irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
desc->depth++;
irq_disable(desc); <--- disabled here
pm_system_irq_wakeup(irq_desc_get_irq(desc));
return true;
and for irqd_is_wakeup_armed:
static bool suspend_device_irq(struct irq_desc *desc)
{
...
if (irqd_is_wakeup_set(&desc->irq_data)) {
irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); <-- set
irqd_is_wakeup_armed here
void dpm_noirq_begin(void)
{
cpuidle_pause();
device_wakeup_arm_wake_irqs();
suspend_device_irqs();
so unless we get an irq between device_wakeup_arm_wake_irqs and
suspend_device_irq, the irq_pm_check_wakeup would not let us get to
handle_threaded_wake_irq...
>
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
actually this would not work...irq_get_trigger_type would return zero
due to a bug which we have a patch for it already:
9908207 New [tip:irq/urgent] genirq: Restore trigger settings
in irq_modify_status()
BTW, using dev_name for the name of this wake irq seems not very
convenient...maybe add a ":wake" suffix?
>
> Brian
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 17:47 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 17:47 UTC (permalink / raw)
To: Brian Norris
Cc: linux-kernel, bhelgaas, shawn.lin, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel, Tony Lindgren
Hi Brian,
On 08/19/2017 01:01 AM, Brian Norris wrote:
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
hmmm, right, but as i replied at cros partner issue, this irq handle
might not be called actually...
in my test on cros 4.4 kernel, it would break by irq_may_run(returning
false):
static bool irq_may_run(struct irq_desc *desc)
{
...
/*
* If the interrupt is an armed wakeup source, mark it pending
* and suspended, disable it and notify the pm core about the
* event.
*/
if (irq_pm_check_wakeup(desc))
return false;
bool irq_pm_check_wakeup(struct irq_desc *desc)
{
if (irqd_is_wakeup_armed(&desc->irq_data)) {
irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
desc->depth++;
irq_disable(desc); <--- disabled here
pm_system_irq_wakeup(irq_desc_get_irq(desc));
return true;
and for irqd_is_wakeup_armed:
static bool suspend_device_irq(struct irq_desc *desc)
{
...
if (irqd_is_wakeup_set(&desc->irq_data)) {
irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); <-- set
irqd_is_wakeup_armed here
void dpm_noirq_begin(void)
{
cpuidle_pause();
device_wakeup_arm_wake_irqs();
suspend_device_irqs();
so unless we get an irq between device_wakeup_arm_wake_irqs and
suspend_device_irq, the irq_pm_check_wakeup would not let us get to
handle_threaded_wake_irq...
>
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
actually this would not work...irq_get_trigger_type would return zero
due to a bug which we have a patch for it already:
9908207 New [tip:irq/urgent] genirq: Restore trigger settings
in irq_modify_status()
BTW, using dev_name for the name of this wake irq seems not very
convenient...maybe add a ":wake" suffix?
>
> Brian
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 17:47 ` jeffy
(?)
@ 2017-08-18 18:28 ` Tony Lindgren
-1 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:28 UTC (permalink / raw)
To: jeffy
Cc: Heiko Stuebner, linux-pci, shawn.lin, Brian Norris, dianders,
linux-kernel, linux-rockchip, bhelgaas, linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 11:05]:
> On 08/19/2017 01:01 AM, Brian Norris wrote:
> > BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> > like this:
> >
> > int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > {
> > ...
> > err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> > - IRQF_ONESHOT, dev_name(dev), wirq);
> > + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
> >
> > But IIUC, that's not actually necessary, because __setup_irq()
> > automatically configures the trigger type if the driver didn't request
> > one explicitly.
>
> actually this would not work...irq_get_trigger_type would return zero due to
> a bug which we have a patch for it already:
>
> 9908207 New [tip:irq/urgent] genirq: Restore trigger settings in
> irq_modify_status()
Thanks for that information. So it seems we can leave out the
irq_get_trigger_type() here then? Might be worth checking
that it really does get populated though :)
> BTW, using dev_name for the name of this wake irq seems not very
> convenient...maybe add a ":wake" suffix?
Good idea, will take a look.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 18:28 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:28 UTC (permalink / raw)
To: linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 11:05]:
> On 08/19/2017 01:01 AM, Brian Norris wrote:
> > BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> > like this:
> >
> > int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > {
> > ...
> > err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> > - IRQF_ONESHOT, dev_name(dev), wirq);
> > + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
> >
> > But IIUC, that's not actually necessary, because __setup_irq()
> > automatically configures the trigger type if the driver didn't request
> > one explicitly.
>
> actually this would not work...irq_get_trigger_type would return zero due to
> a bug which we have a patch for it already:
>
> 9908207 New [tip:irq/urgent] genirq: Restore trigger settings in
> irq_modify_status()
Thanks for that information. So it seems we can leave out the
irq_get_trigger_type() here then? Might be worth checking
that it really does get populated though :)
> BTW, using dev_name for the name of this wake irq seems not very
> convenient...maybe add a ":wake" suffix?
Good idea, will take a look.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 18:28 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:28 UTC (permalink / raw)
To: jeffy
Cc: Brian Norris, Heiko Stuebner, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 11:05]:
> On 08/19/2017 01:01 AM, Brian Norris wrote:
> > BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> > like this:
> >
> > int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > {
> > ...
> > err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> > - IRQF_ONESHOT, dev_name(dev), wirq);
> > + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
> >
> > But IIUC, that's not actually necessary, because __setup_irq()
> > automatically configures the trigger type if the driver didn't request
> > one explicitly.
>
> actually this would not work...irq_get_trigger_type would return zero due to
> a bug which we have a patch for it already:
>
> 9908207 New [tip:irq/urgent] genirq: Restore trigger settings in
> irq_modify_status()
Thanks for that information. So it seems we can leave out the
irq_get_trigger_type() here then? Might be worth checking
that it really does get populated though :)
> BTW, using dev_name for the name of this wake irq seems not very
> convenient...maybe add a ":wake" suffix?
Good idea, will take a look.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 17:01 ` Brian Norris
(?)
@ 2017-08-18 18:14 ` Tony Lindgren
-1 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:14 UTC (permalink / raw)
To: Brian Norris
Cc: Heiko Stuebner, linux-pci, shawn.lin, Jeffy Chen, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
* Brian Norris <briannorris@chromium.org> [170818 10:01]:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 7bb9870f6d8c..c2b973c738fe 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -36,6 +36,7 @@
> > #include <linux/pci_ids.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_wakeirq.h>
> > #include <linux/reset.h>
> > #include <linux/regmap.h>
> >
> > @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> > chained_irq_exit(chip, desc);
> > }
> >
> > -
> > /**
> > * rockchip_pcie_parse_dt - Parse Device Tree
> > * @rockchip: PCIe port information
> > @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wake");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
Hmm yeah that should be checked. The test cases I have are all
edge interrupts where there is no status whatsoever after the
wake-up event except which irq fired.
To me it seems that the wakeirq consumer driver should be able
to clear the level wakeirq in it's runtime_resume, or even better,
maybe all it takes is just let the consumer driver's irq handler
clear the level wakeirq when it runs after runtime_resume.
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
OK so we should make sure that the triggering is parsed
properly when passed in from device tree.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 18:14 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:14 UTC (permalink / raw)
To: linux-arm-kernel
* Brian Norris <briannorris@chromium.org> [170818 10:01]:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 7bb9870f6d8c..c2b973c738fe 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -36,6 +36,7 @@
> > #include <linux/pci_ids.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_wakeirq.h>
> > #include <linux/reset.h>
> > #include <linux/regmap.h>
> >
> > @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> > chained_irq_exit(chip, desc);
> > }
> >
> > -
> > /**
> > * rockchip_pcie_parse_dt - Parse Device Tree
> > * @rockchip: PCIe port information
> > @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wake");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
Hmm yeah that should be checked. The test cases I have are all
edge interrupts where there is no status whatsoever after the
wake-up event except which irq fired.
To me it seems that the wakeirq consumer driver should be able
to clear the level wakeirq in it's runtime_resume, or even better,
maybe all it takes is just let the consumer driver's irq handler
clear the level wakeirq when it runs after runtime_resume.
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
OK so we should make sure that the triggering is parsed
properly when passed in from device tree.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 18:14 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-18 18:14 UTC (permalink / raw)
To: Brian Norris
Cc: Jeffy Chen, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
* Brian Norris <briannorris@chromium.org> [170818 10:01]:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 7bb9870f6d8c..c2b973c738fe 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -36,6 +36,7 @@
> > #include <linux/pci_ids.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_wakeirq.h>
> > #include <linux/reset.h>
> > #include <linux/regmap.h>
> >
> > @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> > chained_irq_exit(chip, desc);
> > }
> >
> > -
> > /**
> > * rockchip_pcie_parse_dt - Parse Device Tree
> > * @rockchip: PCIe port information
> > @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wake");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
Hmm yeah that should be checked. The test cases I have are all
edge interrupts where there is no status whatsoever after the
wake-up event except which irq fired.
To me it seems that the wakeirq consumer driver should be able
to clear the level wakeirq in it's runtime_resume, or even better,
maybe all it takes is just let the consumer driver's irq handler
clear the level wakeirq when it runs after runtime_resume.
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
OK so we should make sure that the triggering is parsed
properly when passed in from device tree.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 18:14 ` Tony Lindgren
(?)
@ 2017-08-18 20:05 ` jeffy
-1 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 20:05 UTC (permalink / raw)
To: Tony Lindgren, Brian Norris
Cc: Heiko Stuebner, linux-pci, shawn.lin, linux-kernel, dianders,
linux-rockchip, bhelgaas, linux-arm-kernel
Hi guys,
On 08/19/2017 02:14 AM, Tony Lindgren wrote:
>> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
>> >{
>> > struct wake_irq *wirq = _wirq;
>> > int res;
>> >
>> > /* Maybe abort suspend? */
>> > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
>> > pm_wakeup_event(wirq->dev, 0);
>> >
>> > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
>> > }
>> >...
>> >
>> >This could cause some kind of an IRQ storm, including a lockup or
>> >significant slowdown, I think.
> Hmm yeah that should be checked. The test cases I have are all
> edge interrupts where there is no status whatsoever after the
> wake-up event except which irq fired.
>
> To me it seems that the wakeirq consumer driver should be able
> to clear the level wakeirq in it's runtime_resume, or even better,
> maybe all it takes is just let the consumer driver's irq handler
> clear the level wakeirq when it runs after runtime_resume.
>
i did some tests about it:
[ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
[ 70.335932] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.335965] suspend_device_irq
[ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
...<--- no wake irq handler log
[ 70.336173] resume_irqs <-- enable wake irq
[ 70.336480] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
...<--- no wake irq handler log
so it would indeed possible to get irq storm in
device_wakeup_arm_wake_irqs to suspend_device_irq
and resume_irqs to device_wakeup_disarm_wake_irqs.
a simple workaround would be:
enable_irq_wake
suspend_device_irq
enable_irq
...irq fired, irq_pm_check_wakeup disabled irq
disable_irq
resume_irqs
disable_irq_wake
and i have a hacky patch for that, which works well:
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -1308,6 +1308,8 @@ static int __maybe_unused
rockchip_pcie_suspend_noirq(struct
device *dev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_enable_wake_irq(dev);
+
return ret;
}
@@ -1316,6 +1318,8 @@ static int __maybe_unused
rockchip_pcie_resume_noirq(struct d
evice *dev)
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
int err;
+ dev_pm_disable_wake_irq(dev);
+
@ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
return;
if (device_may_wakeup(wirq->dev)) {
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
enable_irq(wirq->irq);
enable_irq_wake(wirq->irq);
@@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
if (device_may_wakeup(wirq->dev)) {
disable_irq_wake(wirq->irq);
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
disable_irq_nosync(wirq->irq);
}
which is basically move enable_irq and disable_irq to driver noirq
stages, to avoid:
1/ not disabled by irq_pm_check_wakeup when it first fired
2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
with that hack, i no longer saw the irq storm, and the irq handler would
never be called:
[ 9.693385] device_wakeup_arm_wake_irqs
[ 9.697130] suspend_device_irq
<--- suspend noirq, enable wake irq
[ 9.716569] irq_pm_check_wakeup disable the wake irq
<--- resume noirq, disable wake irq
[ 9.968115] resume_irqs <-- enable wake irq(not actually enable,
since disabled twice)
[ 10.193072] device_wakeup_disarm_wake_irqs
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 20:05 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 20:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi guys,
On 08/19/2017 02:14 AM, Tony Lindgren wrote:
>> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
>> >{
>> > struct wake_irq *wirq = _wirq;
>> > int res;
>> >
>> > /* Maybe abort suspend? */
>> > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
>> > pm_wakeup_event(wirq->dev, 0);
>> >
>> > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
>> > }
>> >...
>> >
>> >This could cause some kind of an IRQ storm, including a lockup or
>> >significant slowdown, I think.
> Hmm yeah that should be checked. The test cases I have are all
> edge interrupts where there is no status whatsoever after the
> wake-up event except which irq fired.
>
> To me it seems that the wakeirq consumer driver should be able
> to clear the level wakeirq in it's runtime_resume, or even better,
> maybe all it takes is just let the consumer driver's irq handler
> clear the level wakeirq when it runs after runtime_resume.
>
i did some tests about it:
[ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
[ 70.335932] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.335965] suspend_device_irq
[ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
...<--- no wake irq handler log
[ 70.336173] resume_irqs <-- enable wake irq
[ 70.336480] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
...<--- no wake irq handler log
so it would indeed possible to get irq storm in
device_wakeup_arm_wake_irqs to suspend_device_irq
and resume_irqs to device_wakeup_disarm_wake_irqs.
a simple workaround would be:
enable_irq_wake
suspend_device_irq
enable_irq
...irq fired, irq_pm_check_wakeup disabled irq
disable_irq
resume_irqs
disable_irq_wake
and i have a hacky patch for that, which works well:
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -1308,6 +1308,8 @@ static int __maybe_unused
rockchip_pcie_suspend_noirq(struct
device *dev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_enable_wake_irq(dev);
+
return ret;
}
@@ -1316,6 +1318,8 @@ static int __maybe_unused
rockchip_pcie_resume_noirq(struct d
evice *dev)
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
int err;
+ dev_pm_disable_wake_irq(dev);
+
@ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
return;
if (device_may_wakeup(wirq->dev)) {
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
enable_irq(wirq->irq);
enable_irq_wake(wirq->irq);
@@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
if (device_may_wakeup(wirq->dev)) {
disable_irq_wake(wirq->irq);
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
disable_irq_nosync(wirq->irq);
}
which is basically move enable_irq and disable_irq to driver noirq
stages, to avoid:
1/ not disabled by irq_pm_check_wakeup when it first fired
2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
with that hack, i no longer saw the irq storm, and the irq handler would
never be called:
[ 9.693385] device_wakeup_arm_wake_irqs
[ 9.697130] suspend_device_irq
<--- suspend noirq, enable wake irq
[ 9.716569] irq_pm_check_wakeup disable the wake irq
<--- resume noirq, disable wake irq
[ 9.968115] resume_irqs <-- enable wake irq(not actually enable,
since disabled twice)
[ 10.193072] device_wakeup_disarm_wake_irqs
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-18 20:05 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-18 20:05 UTC (permalink / raw)
To: Tony Lindgren, Brian Norris
Cc: linux-kernel, bhelgaas, shawn.lin, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel
Hi guys,
On 08/19/2017 02:14 AM, Tony Lindgren wrote:
>> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
>> >{
>> > struct wake_irq *wirq = _wirq;
>> > int res;
>> >
>> > /* Maybe abort suspend? */
>> > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
>> > pm_wakeup_event(wirq->dev, 0);
>> >
>> > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
>> > }
>> >...
>> >
>> >This could cause some kind of an IRQ storm, including a lockup or
>> >significant slowdown, I think.
> Hmm yeah that should be checked. The test cases I have are all
> edge interrupts where there is no status whatsoever after the
> wake-up event except which irq fired.
>
> To me it seems that the wakeirq consumer driver should be able
> to clear the level wakeirq in it's runtime_resume, or even better,
> maybe all it takes is just let the consumer driver's irq handler
> clear the level wakeirq when it runs after runtime_resume.
>
i did some tests about it:
[ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
[ 70.335932] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.335965] suspend_device_irq
[ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
...<--- no wake irq handler log
[ 70.336173] resume_irqs <-- enable wake irq
[ 70.336480] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
...<--- no wake irq handler log
so it would indeed possible to get irq storm in
device_wakeup_arm_wake_irqs to suspend_device_irq
and resume_irqs to device_wakeup_disarm_wake_irqs.
a simple workaround would be:
enable_irq_wake
suspend_device_irq
enable_irq
...irq fired, irq_pm_check_wakeup disabled irq
disable_irq
resume_irqs
disable_irq_wake
and i have a hacky patch for that, which works well:
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -1308,6 +1308,8 @@ static int __maybe_unused
rockchip_pcie_suspend_noirq(struct
device *dev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_enable_wake_irq(dev);
+
return ret;
}
@@ -1316,6 +1318,8 @@ static int __maybe_unused
rockchip_pcie_resume_noirq(struct d
evice *dev)
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
int err;
+ dev_pm_disable_wake_irq(dev);
+
@ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
return;
if (device_may_wakeup(wirq->dev)) {
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
enable_irq(wirq->irq);
enable_irq_wake(wirq->irq);
@@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
if (device_may_wakeup(wirq->dev)) {
disable_irq_wake(wirq->irq);
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
disable_irq_nosync(wirq->irq);
}
which is basically move enable_irq and disable_irq to driver noirq
stages, to avoid:
1/ not disabled by irq_pm_check_wakeup when it first fired
2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
with that hack, i no longer saw the irq storm, and the irq handler would
never be called:
[ 9.693385] device_wakeup_arm_wake_irqs
[ 9.697130] suspend_device_irq
<--- suspend noirq, enable wake irq
[ 9.716569] irq_pm_check_wakeup disable the wake irq
<--- resume noirq, disable wake irq
[ 9.968115] resume_irqs <-- enable wake irq(not actually enable,
since disabled twice)
[ 10.193072] device_wakeup_disarm_wake_irqs
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-18 20:05 ` jeffy
(?)
@ 2017-08-22 17:26 ` Tony Lindgren
-1 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-22 17:26 UTC (permalink / raw)
To: jeffy
Cc: Heiko Stuebner, linux-pci, shawn.lin, Brian Norris, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 13:05]:
> Hi guys,
>
> On 08/19/2017 02:14 AM, Tony Lindgren wrote:
> > > static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> > > >{
> > > > struct wake_irq *wirq = _wirq;
> > > > int res;
> > > >
> > > > /* Maybe abort suspend? */
> > > > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> > > > pm_wakeup_event(wirq->dev, 0);
> > > >
> > > > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> > > > }
> > > >...
> > > >
> > > >This could cause some kind of an IRQ storm, including a lockup or
> > > >significant slowdown, I think.
> > Hmm yeah that should be checked. The test cases I have are all
> > edge interrupts where there is no status whatsoever after the
> > wake-up event except which irq fired.
> >
> > To me it seems that the wakeirq consumer driver should be able
> > to clear the level wakeirq in it's runtime_resume, or even better,
> > maybe all it takes is just let the consumer driver's irq handler
> > clear the level wakeirq when it runs after runtime_resume.
> >
>
> i did some tests about it:
> [ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
> [ 70.335932] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.335965] suspend_device_irq
> [ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
> ...<--- no wake irq handler log
> [ 70.336173] resume_irqs <-- enable wake irq
> [ 70.336480] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
> ...<--- no wake irq handler log
>
>
> so it would indeed possible to get irq storm in device_wakeup_arm_wake_irqs
> to suspend_device_irq
> and resume_irqs to device_wakeup_disarm_wake_irqs.
>
>
> a simple workaround would be:
> enable_irq_wake
> suspend_device_irq
> enable_irq
> ...irq fired, irq_pm_check_wakeup disabled irq
> disable_irq
> resume_irqs
> disable_irq_wake
>
>
>
>
> and i have a hacky patch for that, which works well:
>
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -1308,6 +1308,8 @@ static int __maybe_unused
> rockchip_pcie_suspend_noirq(struct
> device *dev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_enable_wake_irq(dev);
> +
> return ret;
> }
>
> @@ -1316,6 +1318,8 @@ static int __maybe_unused
> rockchip_pcie_resume_noirq(struct d
> evice *dev)
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> int err;
>
> + dev_pm_disable_wake_irq(dev);
> +
>
>
> @ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> return;
>
> if (device_may_wakeup(wirq->dev)) {
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> enable_irq(wirq->irq);
>
> enable_irq_wake(wirq->irq);
> @@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> if (device_may_wakeup(wirq->dev)) {
> disable_irq_wake(wirq->irq);
>
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> disable_irq_nosync(wirq->irq);
> }
>
>
>
> which is basically move enable_irq and disable_irq to driver noirq stages,
> to avoid:
> 1/ not disabled by irq_pm_check_wakeup when it first fired
> 2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
>
>
> with that hack, i no longer saw the irq storm, and the irq handler would
> never be called:
>
> [ 9.693385] device_wakeup_arm_wake_irqs
> [ 9.697130] suspend_device_irq
> <--- suspend noirq, enable wake irq
> [ 9.716569] irq_pm_check_wakeup disable the wake irq
> <--- resume noirq, disable wake irq
> [ 9.968115] resume_irqs <-- enable wake irq(not actually enable, since
> disabled twice)
> [ 10.193072] device_wakeup_disarm_wake_irqs
OK, let's fix any wakeriq ordering issues to make it more
usable. Sounds like in your case the wakeirq needs to be enabled
late and disabled early, while in my test cases I can keep it
enabled basically any time.
If this is for suspend/resume, You could just register the
wakeirq on suspend and then remove it on resume. We do have at
least network drivers doing device_init_wakeup(dev, true) and
device_init_wakeup(dev, false) as needed for WOL, see for example
bfin_mac_ethtool_setwol().
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-22 17:26 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-22 17:26 UTC (permalink / raw)
To: linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 13:05]:
> Hi guys,
>
> On 08/19/2017 02:14 AM, Tony Lindgren wrote:
> > > static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> > > >{
> > > > struct wake_irq *wirq = _wirq;
> > > > int res;
> > > >
> > > > /* Maybe abort suspend? */
> > > > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> > > > pm_wakeup_event(wirq->dev, 0);
> > > >
> > > > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> > > > }
> > > >...
> > > >
> > > >This could cause some kind of an IRQ storm, including a lockup or
> > > >significant slowdown, I think.
> > Hmm yeah that should be checked. The test cases I have are all
> > edge interrupts where there is no status whatsoever after the
> > wake-up event except which irq fired.
> >
> > To me it seems that the wakeirq consumer driver should be able
> > to clear the level wakeirq in it's runtime_resume, or even better,
> > maybe all it takes is just let the consumer driver's irq handler
> > clear the level wakeirq when it runs after runtime_resume.
> >
>
> i did some tests about it:
> [ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
> [ 70.335932] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.335965] suspend_device_irq
> [ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
> ...<--- no wake irq handler log
> [ 70.336173] resume_irqs <-- enable wake irq
> [ 70.336480] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
> ...<--- no wake irq handler log
>
>
> so it would indeed possible to get irq storm in device_wakeup_arm_wake_irqs
> to suspend_device_irq
> and resume_irqs to device_wakeup_disarm_wake_irqs.
>
>
> a simple workaround would be:
> enable_irq_wake
> suspend_device_irq
> enable_irq
> ...irq fired, irq_pm_check_wakeup disabled irq
> disable_irq
> resume_irqs
> disable_irq_wake
>
>
>
>
> and i have a hacky patch for that, which works well:
>
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -1308,6 +1308,8 @@ static int __maybe_unused
> rockchip_pcie_suspend_noirq(struct
> device *dev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_enable_wake_irq(dev);
> +
> return ret;
> }
>
> @@ -1316,6 +1318,8 @@ static int __maybe_unused
> rockchip_pcie_resume_noirq(struct d
> evice *dev)
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> int err;
>
> + dev_pm_disable_wake_irq(dev);
> +
>
>
> @ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> return;
>
> if (device_may_wakeup(wirq->dev)) {
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> enable_irq(wirq->irq);
>
> enable_irq_wake(wirq->irq);
> @@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> if (device_may_wakeup(wirq->dev)) {
> disable_irq_wake(wirq->irq);
>
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> disable_irq_nosync(wirq->irq);
> }
>
>
>
> which is basically move enable_irq and disable_irq to driver noirq stages,
> to avoid:
> 1/ not disabled by irq_pm_check_wakeup when it first fired
> 2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
>
>
> with that hack, i no longer saw the irq storm, and the irq handler would
> never be called:
>
> [ 9.693385] device_wakeup_arm_wake_irqs
> [ 9.697130] suspend_device_irq
> <--- suspend noirq, enable wake irq
> [ 9.716569] irq_pm_check_wakeup disable the wake irq
> <--- resume noirq, disable wake irq
> [ 9.968115] resume_irqs <-- enable wake irq(not actually enable, since
> disabled twice)
> [ 10.193072] device_wakeup_disarm_wake_irqs
OK, let's fix any wakeriq ordering issues to make it more
usable. Sounds like in your case the wakeirq needs to be enabled
late and disabled early, while in my test cases I can keep it
enabled basically any time.
If this is for suspend/resume, You could just register the
wakeirq on suspend and then remove it on resume. We do have at
least network drivers doing device_init_wakeup(dev, true) and
device_init_wakeup(dev, false) as needed for WOL, see for example
bfin_mac_ethtool_setwol().
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-22 17:26 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-08-22 17:26 UTC (permalink / raw)
To: jeffy
Cc: Brian Norris, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
* jeffy <jeffy.chen@rock-chips.com> [170818 13:05]:
> Hi guys,
>
> On 08/19/2017 02:14 AM, Tony Lindgren wrote:
> > > static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> > > >{
> > > > struct wake_irq *wirq = _wirq;
> > > > int res;
> > > >
> > > > /* Maybe abort suspend? */
> > > > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> > > > pm_wakeup_event(wirq->dev, 0);
> > > >
> > > > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> > > > }
> > > >...
> > > >
> > > >This could cause some kind of an IRQ storm, including a lockup or
> > > >significant slowdown, I think.
> > Hmm yeah that should be checked. The test cases I have are all
> > edge interrupts where there is no status whatsoever after the
> > wake-up event except which irq fired.
> >
> > To me it seems that the wakeirq consumer driver should be able
> > to clear the level wakeirq in it's runtime_resume, or even better,
> > maybe all it takes is just let the consumer driver's irq handler
> > clear the level wakeirq when it runs after runtime_resume.
> >
>
> i did some tests about it:
> [ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
> [ 70.335932] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.335965] suspend_device_irq
> [ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
> ...<--- no wake irq handler log
> [ 70.336173] resume_irqs <-- enable wake irq
> [ 70.336480] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
> ...<--- no wake irq handler log
>
>
> so it would indeed possible to get irq storm in device_wakeup_arm_wake_irqs
> to suspend_device_irq
> and resume_irqs to device_wakeup_disarm_wake_irqs.
>
>
> a simple workaround would be:
> enable_irq_wake
> suspend_device_irq
> enable_irq
> ...irq fired, irq_pm_check_wakeup disabled irq
> disable_irq
> resume_irqs
> disable_irq_wake
>
>
>
>
> and i have a hacky patch for that, which works well:
>
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -1308,6 +1308,8 @@ static int __maybe_unused
> rockchip_pcie_suspend_noirq(struct
> device *dev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_enable_wake_irq(dev);
> +
> return ret;
> }
>
> @@ -1316,6 +1318,8 @@ static int __maybe_unused
> rockchip_pcie_resume_noirq(struct d
> evice *dev)
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> int err;
>
> + dev_pm_disable_wake_irq(dev);
> +
>
>
> @ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> return;
>
> if (device_may_wakeup(wirq->dev)) {
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> enable_irq(wirq->irq);
>
> enable_irq_wake(wirq->irq);
> @@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> if (device_may_wakeup(wirq->dev)) {
> disable_irq_wake(wirq->irq);
>
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> disable_irq_nosync(wirq->irq);
> }
>
>
>
> which is basically move enable_irq and disable_irq to driver noirq stages,
> to avoid:
> 1/ not disabled by irq_pm_check_wakeup when it first fired
> 2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
>
>
> with that hack, i no longer saw the irq storm, and the irq handler would
> never be called:
>
> [ 9.693385] device_wakeup_arm_wake_irqs
> [ 9.697130] suspend_device_irq
> <--- suspend noirq, enable wake irq
> [ 9.716569] irq_pm_check_wakeup disable the wake irq
> <--- resume noirq, disable wake irq
> [ 9.968115] resume_irqs <-- enable wake irq(not actually enable, since
> disabled twice)
> [ 10.193072] device_wakeup_disarm_wake_irqs
OK, let's fix any wakeriq ordering issues to make it more
usable. Sounds like in your case the wakeirq needs to be enabled
late and disabled early, while in my test cases I can keep it
enabled basically any time.
If this is for suspend/resume, You could just register the
wakeirq on suspend and then remove it on resume. We do have at
least network drivers doing device_init_wakeup(dev, true) and
device_init_wakeup(dev, false) as needed for WOL, see for example
bfin_mac_ethtool_setwol().
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-22 17:26 ` Tony Lindgren
@ 2017-08-23 1:32 ` jeffy
-1 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-23 1:32 UTC (permalink / raw)
To: Tony Lindgren
Cc: Brian Norris, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
Hi Tony,
On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> OK, let's fix any wakeriq ordering issues to make it more
> usable. Sounds like in your case the wakeirq needs to be enabled
> late and disabled early, while in my test cases I can keep it
> enabled basically any time.
yes, in my case it's a level triggered irq, which needs to be disabled
when receive it(by irq_pm_check_wakeup(my hack) or inside of the custom
irq handler)
and for eage irq, maybe we should enable it right after(or before) the
driver activate wake function(for example activate WOWLAN or WOLAN),
otherwise would it be possible to miss some irqs(triggered before we
actually enable the wake irq)?
>
> If this is for suspend/resume, You could just register the
> wakeirq on suspend and then remove it on resume. We do have at
> least network drivers doing device_init_wakeup(dev, true) and
> device_init_wakeup(dev, false) as needed for WOL, see for example
> bfin_mac_ethtool_setwol().
>
> Regards,
>
> Tony
>
>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-23 1:32 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-23 1:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tony,
On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> OK, let's fix any wakeriq ordering issues to make it more
> usable. Sounds like in your case the wakeirq needs to be enabled
> late and disabled early, while in my test cases I can keep it
> enabled basically any time.
yes, in my case it's a level triggered irq, which needs to be disabled
when receive it(by irq_pm_check_wakeup(my hack) or inside of the custom
irq handler)
and for eage irq, maybe we should enable it right after(or before) the
driver activate wake function(for example activate WOWLAN or WOLAN),
otherwise would it be possible to miss some irqs(triggered before we
actually enable the wake irq)?
>
> If this is for suspend/resume, You could just register the
> wakeirq on suspend and then remove it on resume. We do have at
> least network drivers doing device_init_wakeup(dev, true) and
> device_init_wakeup(dev, false) as needed for WOL, see for example
> bfin_mac_ethtool_setwol().
>
> Regards,
>
> Tony
>
>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-23 1:32 ` jeffy
(?)
@ 2017-08-23 1:57 ` Brian Norris
-1 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-23 1:57 UTC (permalink / raw)
To: jeffy
Cc: Heiko Stuebner, Tony Lindgren, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
Hi Jeffy,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
I already mentioned this: for the PCI case, the specification explicitly
says that the WAKE# pin must remain asserted until the system wakes and
resets the link. So we don't have this problem.
But it is probably still useful to make sure there's a well-defined
point at which these interrupts are armed, so that if a device driver
does care, it can account for that. Just before suspend_noirq (as it is
today) is probably fine, so if there's some device-level handling that
needs to happen before we get to suspend (but after the wakeirq is
armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Brian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-23 1:57 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-23 1:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Jeffy,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
I already mentioned this: for the PCI case, the specification explicitly
says that the WAKE# pin must remain asserted until the system wakes and
resets the link. So we don't have this problem.
But it is probably still useful to make sure there's a well-defined
point at which these interrupts are armed, so that if a device driver
does care, it can account for that. Just before suspend_noirq (as it is
today) is probably fine, so if there's some device-level handling that
needs to happen before we get to suspend (but after the wakeirq is
armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Brian
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-23 1:57 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-08-23 1:57 UTC (permalink / raw)
To: jeffy
Cc: Tony Lindgren, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
Hi Jeffy,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
I already mentioned this: for the PCI case, the specification explicitly
says that the WAKE# pin must remain asserted until the system wakes and
resets the link. So we don't have this problem.
But it is probably still useful to make sure there's a well-defined
point at which these interrupts are armed, so that if a device driver
does care, it can account for that. Just before suspend_noirq (as it is
today) is probably fine, so if there's some device-level handling that
needs to happen before we get to suspend (but after the wakeirq is
armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Brian
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-23 1:57 ` Brian Norris
(?)
@ 2017-08-23 2:16 ` jeffy
-1 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-23 2:16 UTC (permalink / raw)
To: Brian Norris
Cc: Heiko Stuebner, Tony Lindgren, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
Hi Brian,
On 08/23/2017 09:57 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>> and for eage irq, maybe we should enable it right after(or before)
>> the driver activate wake function(for example activate WOWLAN or
>> WOLAN), otherwise would it be possible to miss some irqs(triggered
>> before we actually enable the wake irq)?
>
> I already mentioned this: for the PCI case, the specification explicitly
> says that the WAKE# pin must remain asserted until the system wakes and
> resets the link. So we don't have this problem.
Sorry, i means for other use cases of wakeirq, for example sdio wifi
>
> But it is probably still useful to make sure there's a well-defined
> point at which these interrupts are armed, so that if a device driver
> does care, it can account for that. Just before suspend_noirq (as it is
> today) is probably fine, so if there's some device-level handling that
> needs to happen before we get to suspend (but after the wakeirq is
> armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Yes, then we may need to handle "disable level irq" job in the irq
handler(or runtime resume callback as current wakeirq API suggested) for
irqs received before suspend devices irqs.
>
> Brian
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-23 2:16 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-23 2:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi Brian,
On 08/23/2017 09:57 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>> and for eage irq, maybe we should enable it right after(or before)
>> the driver activate wake function(for example activate WOWLAN or
>> WOLAN), otherwise would it be possible to miss some irqs(triggered
>> before we actually enable the wake irq)?
>
> I already mentioned this: for the PCI case, the specification explicitly
> says that the WAKE# pin must remain asserted until the system wakes and
> resets the link. So we don't have this problem.
Sorry, i means for other use cases of wakeirq, for example sdio wifi
>
> But it is probably still useful to make sure there's a well-defined
> point at which these interrupts are armed, so that if a device driver
> does care, it can account for that. Just before suspend_noirq (as it is
> today) is probably fine, so if there's some device-level handling that
> needs to happen before we get to suspend (but after the wakeirq is
> armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Yes, then we may need to handle "disable level irq" job in the irq
handler(or runtime resume callback as current wakeirq API suggested) for
irqs received before suspend devices irqs.
>
> Brian
>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-08-23 2:16 ` jeffy
0 siblings, 0 replies; 55+ messages in thread
From: jeffy @ 2017-08-23 2:16 UTC (permalink / raw)
To: Brian Norris
Cc: Tony Lindgren, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
Hi Brian,
On 08/23/2017 09:57 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>> and for eage irq, maybe we should enable it right after(or before)
>> the driver activate wake function(for example activate WOWLAN or
>> WOLAN), otherwise would it be possible to miss some irqs(triggered
>> before we actually enable the wake irq)?
>
> I already mentioned this: for the PCI case, the specification explicitly
> says that the WAKE# pin must remain asserted until the system wakes and
> resets the link. So we don't have this problem.
Sorry, i means for other use cases of wakeirq, for example sdio wifi
>
> But it is probably still useful to make sure there's a well-defined
> point at which these interrupts are armed, so that if a device driver
> does care, it can account for that. Just before suspend_noirq (as it is
> today) is probably fine, so if there's some device-level handling that
> needs to happen before we get to suspend (but after the wakeirq is
> armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Yes, then we may need to handle "disable level irq" job in the irq
handler(or runtime resume callback as current wakeirq API suggested) for
irqs received before suspend devices irqs.
>
> Brian
>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-08-23 1:32 ` jeffy
(?)
@ 2017-12-19 0:48 ` Brian Norris
-1 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-19 0:48 UTC (permalink / raw)
To: jeffy
Cc: Heiko Stuebner, Tony Lindgren, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
Hi Jeffy, Tony,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> Hi Tony,
>
> On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> >OK, let's fix any wakeriq ordering issues to make it more
> >usable. Sounds like in your case the wakeirq needs to be enabled
> >late and disabled early, while in my test cases I can keep it
> >enabled basically any time.
>
> yes, in my case it's a level triggered irq, which needs to be
> disabled when receive it(by irq_pm_check_wakeup(my hack) or inside
> of the custom irq handler)
>
>
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
Did this problem ever get resolved? To be clear, I believe the problem
at hand is:
(a) in suspend/resume (not runtime PM; we may not even have runtime PM
support for most PCI devices)
(b) using a level-triggered signal (PCI WAKE# is active low, and it's
nice to avoid certain races by treating it as level-triggered)
And with the current wakeirq code (and the latest version of Jeffy's
patch series, IIUC), I believe the above case can still trigger an
interrupt storm of sorts (it's not usually unrecoverably-stormy, since
it's a threaded IRQ, and we make "enough" progress).
I don't see how "ordering" can really resolve this problem, unless the
ordering is configured such that the interrupt handler never runs (e.g.,
we disable the IRQ before we get out of any "noirq" phase).
Options I can think of:
(1) implement runtime PM callbacks for all PCI devices, where we clear
any PME status and ensure WAKE# stops asserting [1]
(2) synchronize a device's resume() with the dedicated wake IRQ
(3) skip using the dedicated wake IRQ infrastructure and write our own
interrupt handler for this PCI/PM function
Option (1) seems pretty strange; we don't actually want to manage these
devices with runtime PM.
Option (2) could work, but it would probably require sharing more of the
core suspend/resume internals between
drivers/base/power/{wakeirq,main}.c, which may not be desirable. Among
other problems, that seems more fragile.
Option (3) is easy enough, and we already did that once for the first
pass at poorly implementing this WAKE# logic within the mwifiex driver
:)
> >If this is for suspend/resume, You could just register the
> >wakeirq on suspend and then remove it on resume. We do have at
> >least network drivers doing device_init_wakeup(dev, true) and
> >device_init_wakeup(dev, false) as needed for WOL, see for example
> >bfin_mac_ethtool_setwol().
I don't see how that would be good enough. You still have a window of
time while the driver hasn't finished resuming, in which the interrupt
handler might trigger many times.
Brian
[1] Then we also need to fixup handle_threaded_wake_irq(). Currently it
will not even try to resume the device:
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- we exit here
}
/* We don't want RPM_ASYNC or RPM_NOWAIT here */
res = pm_runtime_resume(wirq->dev);
...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-19 0:48 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-19 0:48 UTC (permalink / raw)
To: linux-arm-kernel
Hi Jeffy, Tony,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> Hi Tony,
>
> On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> >OK, let's fix any wakeriq ordering issues to make it more
> >usable. Sounds like in your case the wakeirq needs to be enabled
> >late and disabled early, while in my test cases I can keep it
> >enabled basically any time.
>
> yes, in my case it's a level triggered irq, which needs to be
> disabled when receive it(by irq_pm_check_wakeup(my hack) or inside
> of the custom irq handler)
>
>
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
Did this problem ever get resolved? To be clear, I believe the problem
at hand is:
(a) in suspend/resume (not runtime PM; we may not even have runtime PM
support for most PCI devices)
(b) using a level-triggered signal (PCI WAKE# is active low, and it's
nice to avoid certain races by treating it as level-triggered)
And with the current wakeirq code (and the latest version of Jeffy's
patch series, IIUC), I believe the above case can still trigger an
interrupt storm of sorts (it's not usually unrecoverably-stormy, since
it's a threaded IRQ, and we make "enough" progress).
I don't see how "ordering" can really resolve this problem, unless the
ordering is configured such that the interrupt handler never runs (e.g.,
we disable the IRQ before we get out of any "noirq" phase).
Options I can think of:
(1) implement runtime PM callbacks for all PCI devices, where we clear
any PME status and ensure WAKE# stops asserting [1]
(2) synchronize a device's resume() with the dedicated wake IRQ
(3) skip using the dedicated wake IRQ infrastructure and write our own
interrupt handler for this PCI/PM function
Option (1) seems pretty strange; we don't actually want to manage these
devices with runtime PM.
Option (2) could work, but it would probably require sharing more of the
core suspend/resume internals between
drivers/base/power/{wakeirq,main}.c, which may not be desirable. Among
other problems, that seems more fragile.
Option (3) is easy enough, and we already did that once for the first
pass at poorly implementing this WAKE# logic within the mwifiex driver
:)
> >If this is for suspend/resume, You could just register the
> >wakeirq on suspend and then remove it on resume. We do have at
> >least network drivers doing device_init_wakeup(dev, true) and
> >device_init_wakeup(dev, false) as needed for WOL, see for example
> >bfin_mac_ethtool_setwol().
I don't see how that would be good enough. You still have a window of
time while the driver hasn't finished resuming, in which the interrupt
handler might trigger many times.
Brian
[1] Then we also need to fixup handle_threaded_wake_irq(). Currently it
will not even try to resume the device:
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- we exit here
}
/* We don't want RPM_ASYNC or RPM_NOWAIT here */
res = pm_runtime_resume(wirq->dev);
...
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-19 0:48 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-19 0:48 UTC (permalink / raw)
To: jeffy
Cc: Tony Lindgren, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
Hi Jeffy, Tony,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> Hi Tony,
>
> On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> >OK, let's fix any wakeriq ordering issues to make it more
> >usable. Sounds like in your case the wakeirq needs to be enabled
> >late and disabled early, while in my test cases I can keep it
> >enabled basically any time.
>
> yes, in my case it's a level triggered irq, which needs to be
> disabled when receive it(by irq_pm_check_wakeup(my hack) or inside
> of the custom irq handler)
>
>
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
Did this problem ever get resolved? To be clear, I believe the problem
at hand is:
(a) in suspend/resume (not runtime PM; we may not even have runtime PM
support for most PCI devices)
(b) using a level-triggered signal (PCI WAKE# is active low, and it's
nice to avoid certain races by treating it as level-triggered)
And with the current wakeirq code (and the latest version of Jeffy's
patch series, IIUC), I believe the above case can still trigger an
interrupt storm of sorts (it's not usually unrecoverably-stormy, since
it's a threaded IRQ, and we make "enough" progress).
I don't see how "ordering" can really resolve this problem, unless the
ordering is configured such that the interrupt handler never runs (e.g.,
we disable the IRQ before we get out of any "noirq" phase).
Options I can think of:
(1) implement runtime PM callbacks for all PCI devices, where we clear
any PME status and ensure WAKE# stops asserting [1]
(2) synchronize a device's resume() with the dedicated wake IRQ
(3) skip using the dedicated wake IRQ infrastructure and write our own
interrupt handler for this PCI/PM function
Option (1) seems pretty strange; we don't actually want to manage these
devices with runtime PM.
Option (2) could work, but it would probably require sharing more of the
core suspend/resume internals between
drivers/base/power/{wakeirq,main}.c, which may not be desirable. Among
other problems, that seems more fragile.
Option (3) is easy enough, and we already did that once for the first
pass at poorly implementing this WAKE# logic within the mwifiex driver
:)
> >If this is for suspend/resume, You could just register the
> >wakeirq on suspend and then remove it on resume. We do have at
> >least network drivers doing device_init_wakeup(dev, true) and
> >device_init_wakeup(dev, false) as needed for WOL, see for example
> >bfin_mac_ethtool_setwol().
I don't see how that would be good enough. You still have a window of
time while the driver hasn't finished resuming, in which the interrupt
handler might trigger many times.
Brian
[1] Then we also need to fixup handle_threaded_wake_irq(). Currently it
will not even try to resume the device:
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- we exit here
}
/* We don't want RPM_ASYNC or RPM_NOWAIT here */
res = pm_runtime_resume(wirq->dev);
...
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-12-19 0:48 ` Brian Norris
@ 2017-12-20 19:19 ` Tony Lindgren
-1 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-12-20 19:19 UTC (permalink / raw)
To: Brian Norris
Cc: jeffy, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel
Hi,
* Brian Norris <briannorris@chromium.org> [171219 00:50]:
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>
> Did this problem ever get resolved? To be clear, I believe the problem
> at hand is:
>
> (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> support for most PCI devices)
It seems it should be enough to implement runtime PM in the PCI
controller. Isn't each PCI WAKE# line is wired from each PCI device
to the PCI controller?
Then the PCI controller can figure out from which PCI device the
WAKE# came from.
> Options I can think of:
> (1) implement runtime PM callbacks for all PCI devices, where we clear
> any PME status and ensure WAKE# stops asserting [1]
I don't think this is needed, it should be enough to have just
the PCI controller implement runtime PM :)
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread
* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-20 19:19 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-12-20 19:19 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
* Brian Norris <briannorris@chromium.org> [171219 00:50]:
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>
> Did this problem ever get resolved? To be clear, I believe the problem
> at hand is:
>
> (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> support for most PCI devices)
It seems it should be enough to implement runtime PM in the PCI
controller. Isn't each PCI WAKE# line is wired from each PCI device
to the PCI controller?
Then the PCI controller can figure out from which PCI device the
WAKE# came from.
> Options I can think of:
> (1) implement runtime PM callbacks for all PCI devices, where we clear
> any PME status and ensure WAKE# stops asserting [1]
I don't think this is needed, it should be enough to have just
the PCI controller implement runtime PM :)
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-12-20 19:19 ` Tony Lindgren
(?)
@ 2017-12-22 23:20 ` Brian Norris
-1 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-22 23:20 UTC (permalink / raw)
To: Tony Lindgren
Cc: Heiko Stuebner, linux-pci, shawn.lin, jeffy, Rafael J. Wysocki,
linux-kernel, dianders, linux-rockchip, bhelgaas,
linux-arm-kernel
+ Rafael to this thread
On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> >
> > Did this problem ever get resolved? To be clear, I believe the problem
> > at hand is:
> >
> > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > support for most PCI devices)
>
> It seems it should be enough to implement runtime PM in the PCI
> controller. Isn't each PCI WAKE# line is wired from each PCI device
> to the PCI controller?
No, not really. As discussed in later versions of this thread already,
the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
settled that it's reasonable to just consider this as a 1-per-device
thing, with each device "directly" attached to the PM controller. While
sharing could happen, that's something we decided to punt on...didn't
we?
> Then the PCI controller can figure out from which PCI device the
> WAKE# came from.
I'm not completely sure of the details, but I believe this *can* be
determined by PME. But I'm not sure it's guaranteed to be supported,
especially in cases where we already have 1:1 WAKE#. So we should be
*trying* to report this wakeirq info from the device, if possible.
> > Options I can think of:
> > (1) implement runtime PM callbacks for all PCI devices, where we clear
> > any PME status and ensure WAKE# stops asserting [1]
>
> I don't think this is needed, it should be enough to have just
> the PCI controller implement runtime PM :)
Brian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread
* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-22 23:20 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-22 23:20 UTC (permalink / raw)
To: linux-arm-kernel
+ Rafael to this thread
On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> >
> > Did this problem ever get resolved? To be clear, I believe the problem
> > at hand is:
> >
> > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > support for most PCI devices)
>
> It seems it should be enough to implement runtime PM in the PCI
> controller. Isn't each PCI WAKE# line is wired from each PCI device
> to the PCI controller?
No, not really. As discussed in later versions of this thread already,
the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
settled that it's reasonable to just consider this as a 1-per-device
thing, with each device "directly" attached to the PM controller. While
sharing could happen, that's something we decided to punt on...didn't
we?
> Then the PCI controller can figure out from which PCI device the
> WAKE# came from.
I'm not completely sure of the details, but I believe this *can* be
determined by PME. But I'm not sure it's guaranteed to be supported,
especially in cases where we already have 1:1 WAKE#. So we should be
*trying* to report this wakeirq info from the device, if possible.
> > Options I can think of:
> > (1) implement runtime PM callbacks for all PCI devices, where we clear
> > any PME status and ensure WAKE# stops asserting [1]
>
> I don't think this is needed, it should be enough to have just
> the PCI controller implement runtime PM :)
Brian
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-22 23:20 ` Brian Norris
0 siblings, 0 replies; 55+ messages in thread
From: Brian Norris @ 2017-12-22 23:20 UTC (permalink / raw)
To: Tony Lindgren
Cc: jeffy, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel,
Rafael J. Wysocki
+ Rafael to this thread
On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> >
> > Did this problem ever get resolved? To be clear, I believe the problem
> > at hand is:
> >
> > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > support for most PCI devices)
>
> It seems it should be enough to implement runtime PM in the PCI
> controller. Isn't each PCI WAKE# line is wired from each PCI device
> to the PCI controller?
No, not really. As discussed in later versions of this thread already,
the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
settled that it's reasonable to just consider this as a 1-per-device
thing, with each device "directly" attached to the PM controller. While
sharing could happen, that's something we decided to punt on...didn't
we?
> Then the PCI controller can figure out from which PCI device the
> WAKE# came from.
I'm not completely sure of the details, but I believe this *can* be
determined by PME. But I'm not sure it's guaranteed to be supported,
especially in cases where we already have 1:1 WAKE#. So we should be
*trying* to report this wakeirq info from the device, if possible.
> > Options I can think of:
> > (1) implement runtime PM callbacks for all PCI devices, where we clear
> > any PME status and ensure WAKE# stops asserting [1]
>
> I don't think this is needed, it should be enough to have just
> the PCI controller implement runtime PM :)
Brian
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
2017-12-22 23:20 ` Brian Norris
(?)
@ 2017-12-23 16:36 ` Tony Lindgren
-1 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-12-23 16:36 UTC (permalink / raw)
To: Brian Norris
Cc: Heiko Stuebner, linux-pci, shawn.lin, jeffy, Rafael J. Wysocki,
linux-kernel, dianders, linux-rockchip, bhelgaas,
linux-arm-kernel
* Brian Norris <briannorris@chromium.org> [171222 23:23]:
> + Rafael to this thread
>
> On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> > * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> > >
> > > Did this problem ever get resolved? To be clear, I believe the problem
> > > at hand is:
> > >
> > > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > > support for most PCI devices)
> >
> > It seems it should be enough to implement runtime PM in the PCI
> > controller. Isn't each PCI WAKE# line is wired from each PCI device
> > to the PCI controller?
>
> No, not really. As discussed in later versions of this thread already,
> the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
> settled that it's reasonable to just consider this as a 1-per-device
> thing, with each device "directly" attached to the PM controller. While
> sharing could happen, that's something we decided to punt on...didn't
> we?
Sounds like we need a confirmation from some hardware people on
this if the PCI device WAKE# can be wired to PCI controller or
to a separate PM controller directly :)
> > Then the PCI controller can figure out from which PCI device the
> > WAKE# came from.
>
> I'm not completely sure of the details, but I believe this *can* be
> determined by PME. But I'm not sure it's guaranteed to be supported,
> especially in cases where we already have 1:1 WAKE#. So we should be
> *trying* to report this wakeirq info from the device, if possible.
If a PCI device has it's WAKE# wired directly to a PM controller
instead of the PCI controller, then it seems that the PCI controller
should be woken up and then presumably the regular PCI device
interrupts will take care of the rest.
Or else I'd say if the driver for the PCI device in some custom
case needs to do something specific, then having that driver
implement PM runtime makes sense. Naturally we want to avoid a
dependency where all the possible drivers would need to implement
PM runtime, I doubt that's needed though.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 55+ messages in thread
* [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-23 16:36 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-12-23 16:36 UTC (permalink / raw)
To: linux-arm-kernel
* Brian Norris <briannorris@chromium.org> [171222 23:23]:
> + Rafael to this thread
>
> On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> > * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> > >
> > > Did this problem ever get resolved? To be clear, I believe the problem
> > > at hand is:
> > >
> > > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > > support for most PCI devices)
> >
> > It seems it should be enough to implement runtime PM in the PCI
> > controller. Isn't each PCI WAKE# line is wired from each PCI device
> > to the PCI controller?
>
> No, not really. As discussed in later versions of this thread already,
> the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
> settled that it's reasonable to just consider this as a 1-per-device
> thing, with each device "directly" attached to the PM controller. While
> sharing could happen, that's something we decided to punt on...didn't
> we?
Sounds like we need a confirmation from some hardware people on
this if the PCI device WAKE# can be wired to PCI controller or
to a separate PM controller directly :)
> > Then the PCI controller can figure out from which PCI device the
> > WAKE# came from.
>
> I'm not completely sure of the details, but I believe this *can* be
> determined by PME. But I'm not sure it's guaranteed to be supported,
> especially in cases where we already have 1:1 WAKE#. So we should be
> *trying* to report this wakeirq info from the device, if possible.
If a PCI device has it's WAKE# wired directly to a PM controller
instead of the PCI controller, then it seems that the PCI controller
should be woken up and then presumably the regular PCI device
interrupts will take care of the rest.
Or else I'd say if the driver for the PCI device in some custom
case needs to do something specific, then having that driver
implement PM runtime makes sense. Naturally we want to avoid a
dependency where all the possible drivers would need to implement
PM runtime, I doubt that's needed though.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-12-23 16:36 ` Tony Lindgren
0 siblings, 0 replies; 55+ messages in thread
From: Tony Lindgren @ 2017-12-23 16:36 UTC (permalink / raw)
To: Brian Norris
Cc: jeffy, linux-kernel, bhelgaas, shawn.lin, dianders,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel,
Rafael J. Wysocki
* Brian Norris <briannorris@chromium.org> [171222 23:23]:
> + Rafael to this thread
>
> On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> > * Brian Norris <briannorris@chromium.org> [171219 00:50]:
> > > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> > >
> > > Did this problem ever get resolved? To be clear, I believe the problem
> > > at hand is:
> > >
> > > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > > support for most PCI devices)
> >
> > It seems it should be enough to implement runtime PM in the PCI
> > controller. Isn't each PCI WAKE# line is wired from each PCI device
> > to the PCI controller?
>
> No, not really. As discussed in later versions of this thread already,
> the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
> settled that it's reasonable to just consider this as a 1-per-device
> thing, with each device "directly" attached to the PM controller. While
> sharing could happen, that's something we decided to punt on...didn't
> we?
Sounds like we need a confirmation from some hardware people on
this if the PCI device WAKE# can be wired to PCI controller or
to a separate PM controller directly :)
> > Then the PCI controller can figure out from which PCI device the
> > WAKE# came from.
>
> I'm not completely sure of the details, but I believe this *can* be
> determined by PME. But I'm not sure it's guaranteed to be supported,
> especially in cases where we already have 1:1 WAKE#. So we should be
> *trying* to report this wakeirq info from the device, if possible.
If a PCI device has it's WAKE# wired directly to a PM controller
instead of the PCI controller, then it seems that the PCI controller
should be woken up and then presumably the regular PCI device
interrupts will take care of the rest.
Or else I'd say if the driver for the PCI device in some custom
case needs to do something specific, then having that driver
implement PM runtime makes sense. Naturally we want to avoid a
dependency where all the possible drivers would need to implement
PM runtime, I doubt that's needed though.
Regards,
Tony
^ permalink raw reply [flat|nested] 55+ messages in thread