| Message ID | 1442850433-5903-5-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Scott Wood |
| Headers | show |
On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > as it doesn't guarantee wakeup for that interrupt. > > This patch removes the redundant mpic_irq_set_wake and sets the > IRQCHIP_SKIP_SET_WAKE for only FSL MPIC. > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Hongtao Jia <hongtao.jia@freescale.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/powerpc/sysdev/mpic.c | 23 ++++------------------- > 1 file changed, 4 insertions(+), 19 deletions(-) > > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c > index 537e5db85a06..123e43612f0a 100644 > --- a/arch/powerpc/sysdev/mpic.c > +++ b/arch/powerpc/sysdev/mpic.c > @@ -924,22 +924,6 @@ int mpic_set_irq_type(struct irq_data *d, unsigned int > flow_type) > return IRQ_SET_MASK_OK_NOCOPY; > } > > -static int mpic_irq_set_wake(struct irq_data *d, unsigned int on) > -{ > - struct irq_desc *desc = container_of(d, struct irq_desc, irq_data); > - struct mpic *mpic = mpic_from_irq_data(d); > - > - if (!(mpic->flags & MPIC_FSL)) > - return -ENXIO; > - > - if (on) > - desc->action->flags |= IRQF_NO_SUSPEND; > - else > - desc->action->flags &= ~IRQF_NO_SUSPEND; > - > - return 0; > -} > - > void mpic_set_vector(unsigned int virq, unsigned int vector) > { > struct mpic *mpic = mpic_from_irq(virq); > @@ -977,7 +961,6 @@ static struct irq_chip mpic_irq_chip = { > .irq_unmask = mpic_unmask_irq, > .irq_eoi = mpic_end_irq, > .irq_set_type = mpic_set_irq_type, > - .irq_set_wake = mpic_irq_set_wake, > }; > > #ifdef CONFIG_SMP > @@ -992,7 +975,6 @@ static struct irq_chip mpic_tm_chip = { > .irq_mask = mpic_mask_tm, > .irq_unmask = mpic_unmask_tm, > .irq_eoi = mpic_end_irq, > - .irq_set_wake = mpic_irq_set_wake, > }; > > #ifdef CONFIG_MPIC_U3_HT_IRQS > @@ -1283,8 +1265,11 @@ struct mpic * __init mpic_alloc(struct device_node > *node, > flags |= MPIC_NO_RESET; > if (of_get_property(node, "single-cpu-affinity", NULL)) > flags |= MPIC_SINGLE_DEST_CPU; > - if (of_device_is_compatible(node, "fsl,mpic")) > + if (of_device_is_compatible(node, "fsl,mpic")) { > flags |= MPIC_FSL | MPIC_LARGE_VECTORS; > + mpic_irq_chip.flags |= IRQCHIP_SKIP_SET_WAKE; > + mpic_tm_chip.flags |= IRQCHIP_SKIP_SET_WAKE; > + } What difference does IRQCHIP_SKIP_SET_WAKE make in the absence of an .irq_set_wake() callback? This is basically repealing commit 5ff04b7287d87c1db7 ("powerpc/mpic: add irq_set_wake support"). Wang Dongsheng, can you explain why that patch was needed? -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, September 23, 2015 7:50 AM > To: Sudeep Holla > Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Thomas Gleixner; > Rafael J. Wysocki; Benjamin Herrenschmidt; Paul Mackerras; Michael Ellerman; Jia > Hongtao-B38951; Marc Zyngier; linuxppc-dev@lists.ozlabs.org; Wang Dongsheng- > B40534 > Subject: Re: [PATCH 04/17] powerpc: mpic: use IRQCHIP_SKIP_SET_WAKE instead of > redundant mpic_irq_set_wake > > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > > as it doesn't guarantee wakeup for that interrupt. > > Non-freescale return -ENXIO, is there any issue? If non-freescale platform does not support it, but IPs still use enable/disable_irq_wake, we should return a error number. IRQCHIP_SKIP_SET_WAKE just skip this feature, this is not our expected. If non-freescale platform need this flag to skip this feature, it should be add in self platform. @Scott: If set this flag we cannot keep a irq as a wakeup source when system going to SUSPEND or MEM. irq_set_wake() means we can set this irq as a wake source. IRQCHIP_SKIP_SET_WAKE is ignore irq_set_wake() feature. Regards, -Dongsheng
On Wed, 23 Sep 2015, Wang Dongsheng wrote: > > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > > > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > > > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > > > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > > > as it doesn't guarantee wakeup for that interrupt. > > > > > Non-freescale return -ENXIO, is there any issue? If non-freescale > platform does not support it, but IPs still use > enable/disable_irq_wake, we should return a error number. You can just set IRQCHIP_SKIP_SET_WAKE for FSL chips and not for the others. > @Scott: > If set this flag we cannot keep a irq as a wakeup source when system going to > SUSPEND or MEM. > > irq_set_wake() means we can set this irq as a wake source. > IRQCHIP_SKIP_SET_WAKE is ignore irq_set_wake() feature. Nonsense. IRQCHIP_SKIP_SET_WAKE merily tells the core not to bail on !chip->irq_set_wake(), but its still marking the interrupt as wakeup source and therefor not masking it on suspend. IRQF_NO_SUSPEND is the wrong tool. End of story. Thanks, tglx
On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > as it doesn't guarantee wakeup for that interrupt. > > This patch removes the redundant mpic_irq_set_wake and sets the > IRQCHIP_SKIP_SET_WAKE for only FSL MPIC. > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Hongtao Jia <hongtao.jia@freescale.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/powerpc/sysdev/mpic.c | 23 ++++------------------- > 1 file changed, 4 insertions(+), 19 deletions(-) Acked-by: Scott Wood <scottwood@freescale.com> -Scott
> -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Thomas Gleixner > Sent: Wednesday, September 23, 2015 11:49 AM > To: Wang Dongsheng-B40534 > Cc: Wood Scott-B07421; Sudeep Holla; linux-pm@vger.kernel.org; linux- > kernel@vger.kernel.org; Rafael J. Wysocki; Benjamin Herrenschmidt; Paul > Mackerras; Michael Ellerman; Jia Hongtao-B38951; Marc Zyngier; linuxppc- > dev@lists.ozlabs.org > Subject: RE: [PATCH 04/17] powerpc: mpic: use IRQCHIP_SKIP_SET_WAKE instead of > redundant mpic_irq_set_wake > > On Wed, 23 Sep 2015, Wang Dongsheng wrote: > > > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > > > > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > > > > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > > > > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > > > > as it doesn't guarantee wakeup for that interrupt. > > > > > > > > Non-freescale return -ENXIO, is there any issue? If non-freescale > > platform does not support it, but IPs still use > > enable/disable_irq_wake, we should return a error number. > > You can just set IRQCHIP_SKIP_SET_WAKE for FSL chips and not for the > others. > > > @Scott: > > If set this flag we cannot keep a irq as a wakeup source when system going to > > SUSPEND or MEM. > > > > irq_set_wake() means we can set this irq as a wake source. > > IRQCHIP_SKIP_SET_WAKE is ignore irq_set_wake() feature. > > Nonsense. IRQCHIP_SKIP_SET_WAKE merily tells the core not to bail on > !chip->irq_set_wake(), but its still marking the interrupt as wakeup > source and therefor not masking it on suspend. > Sorry, I just check irq_set_irq_wake() code, right, IRQCHIP_SKIP_SET_WAKE also can going to irqd_set to mask IRQD_WAKEUP_STATE. Yes, this flag just skip the irq_set_wake() not this feature. Regards, -Dongsheng
On Wed, 23 Sep 2015, Wang Dongsheng wrote: > > -----Original Message----- > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > > owner@vger.kernel.org] On Behalf Of Thomas Gleixner > > Sent: Wednesday, September 23, 2015 11:49 AM > > To: Wang Dongsheng-B40534 > > Cc: Wood Scott-B07421; Sudeep Holla; linux-pm@vger.kernel.org; linux- > > kernel@vger.kernel.org; Rafael J. Wysocki; Benjamin Herrenschmidt; Paul > > Mackerras; Michael Ellerman; Jia Hongtao-B38951; Marc Zyngier; linuxppc- > > dev@lists.ozlabs.org > > Subject: RE: [PATCH 04/17] powerpc: mpic: use IRQCHIP_SKIP_SET_WAKE instead of > > redundant mpic_irq_set_wake Can you please fix you mail client to get rid of that silly copy of the mail header? > > On Wed, 23 Sep 2015, Wang Dongsheng wrote: > > > > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > > > > > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND > > > > > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > > > > > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > > > > > as it doesn't guarantee wakeup for that interrupt. > > > > > > > > > > > Non-freescale return -ENXIO, is there any issue? If non-freescale > > > platform does not support it, but IPs still use > > > enable/disable_irq_wake, we should return a error number. > > > > You can just set IRQCHIP_SKIP_SET_WAKE for FSL chips and not for the > > others. > > > > > @Scott: > > > If set this flag we cannot keep a irq as a wakeup source when system going to > > > SUSPEND or MEM. > > > > > > irq_set_wake() means we can set this irq as a wake source. > > > IRQCHIP_SKIP_SET_WAKE is ignore irq_set_wake() feature. > > > > Nonsense. IRQCHIP_SKIP_SET_WAKE merily tells the core not to bail on > > !chip->irq_set_wake(), but its still marking the interrupt as wakeup > > source and therefor not masking it on suspend. > > > > Sorry, I just check irq_set_irq_wake() code, right, IRQCHIP_SKIP_SET_WAKE also can > going to irqd_set to mask IRQD_WAKEUP_STATE. > > Yes, this flag just skip the irq_set_wake() not this feature. And just for completeness. That commit 5ff04b7287d87c 'powerpc/mpic: add irq_set_wake support' is another example of trainwreck engineering. desc->action->flags |= IRQF_NO_SUSPEND; This is not only horribly avoiding any of the existing APIs, it's also broken as hell. desc->action can be NULL when that is called. It seems fleascale is hell bent to fiddle with the guts of the core code mindlessly. See commit c866cda47f2c Yours grumpy tglx
Hi Ben, On 23/09/15 05:06, Scott Wood wrote: > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: >> mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND >> flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak >> is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag >> as it doesn't guarantee wakeup for that interrupt. >> >> This patch removes the redundant mpic_irq_set_wake and sets the >> IRQCHIP_SKIP_SET_WAKE for only FSL MPIC. >> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Paul Mackerras <paulus@samba.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Cc: Scott Wood <scottwood@freescale.com> >> Cc: Hongtao Jia <hongtao.jia@freescale.com> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: linuxppc-dev@lists.ozlabs.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> arch/powerpc/sysdev/mpic.c | 23 ++++------------------- >> 1 file changed, 4 insertions(+), 19 deletions(-) > > Acked-by: Scott Wood <scottwood@freescale.com> > Can you pick this up via your tree ?
On Mon, 2015-10-19 at 18:35 +0100, Sudeep Holla wrote: > Hi Ben, > > On 23/09/15 05:06, Scott Wood wrote: > > On Mon, 2015-09-21 at 16:47 +0100, Sudeep Holla wrote: > > > mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets > > > IRQF_NO_SUSPEND > > > flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak > > > is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag > > > as it doesn't guarantee wakeup for that interrupt. > > > > > > This patch removes the redundant mpic_irq_set_wake and sets the > > > IRQCHIP_SKIP_SET_WAKE for only FSL MPIC. > > > > > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > Cc: Paul Mackerras <paulus@samba.org> > > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > > Cc: Scott Wood <scottwood@freescale.com> > > > Cc: Hongtao Jia <hongtao.jia@freescale.com> > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > > Cc: linuxppc-dev@lists.ozlabs.org > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > arch/powerpc/sysdev/mpic.c | 23 ++++------------------- > > > 1 file changed, 4 insertions(+), 19 deletions(-) > > > > Acked-by: Scott Wood <scottwood@freescale.com> > > > > Can you pick this up via your tree ? OK. -Scott
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c index 537e5db85a06..123e43612f0a 100644 --- a/arch/powerpc/sysdev/mpic.c +++ b/arch/powerpc/sysdev/mpic.c @@ -924,22 +924,6 @@ int mpic_set_irq_type(struct irq_data *d, unsigned int flow_type) return IRQ_SET_MASK_OK_NOCOPY; } -static int mpic_irq_set_wake(struct irq_data *d, unsigned int on) -{ - struct irq_desc *desc = container_of(d, struct irq_desc, irq_data); - struct mpic *mpic = mpic_from_irq_data(d); - - if (!(mpic->flags & MPIC_FSL)) - return -ENXIO; - - if (on) - desc->action->flags |= IRQF_NO_SUSPEND; - else - desc->action->flags &= ~IRQF_NO_SUSPEND; - - return 0; -} - void mpic_set_vector(unsigned int virq, unsigned int vector) { struct mpic *mpic = mpic_from_irq(virq); @@ -977,7 +961,6 @@ static struct irq_chip mpic_irq_chip = { .irq_unmask = mpic_unmask_irq, .irq_eoi = mpic_end_irq, .irq_set_type = mpic_set_irq_type, - .irq_set_wake = mpic_irq_set_wake, }; #ifdef CONFIG_SMP @@ -992,7 +975,6 @@ static struct irq_chip mpic_tm_chip = { .irq_mask = mpic_mask_tm, .irq_unmask = mpic_unmask_tm, .irq_eoi = mpic_end_irq, - .irq_set_wake = mpic_irq_set_wake, }; #ifdef CONFIG_MPIC_U3_HT_IRQS @@ -1283,8 +1265,11 @@ struct mpic * __init mpic_alloc(struct device_node *node, flags |= MPIC_NO_RESET; if (of_get_property(node, "single-cpu-affinity", NULL)) flags |= MPIC_SINGLE_DEST_CPU; - if (of_device_is_compatible(node, "fsl,mpic")) + if (of_device_is_compatible(node, "fsl,mpic")) { flags |= MPIC_FSL | MPIC_LARGE_VECTORS; + mpic_irq_chip.flags |= IRQCHIP_SKIP_SET_WAKE; + mpic_tm_chip.flags |= IRQCHIP_SKIP_SET_WAKE; + } mpic = kzalloc(sizeof(struct mpic), GFP_KERNEL); if (mpic == NULL)
mpic_irq_set_wake return -ENXIO for non FSL MPIC and sets IRQF_NO_SUSPEND flag for FSL ones. enable_irq_wake already returns -ENXIO if irq_set_wak is not implemented. Also there's no need to set the IRQF_NO_SUSPEND flag as it doesn't guarantee wakeup for that interrupt. This patch removes the redundant mpic_irq_set_wake and sets the IRQCHIP_SKIP_SET_WAKE for only FSL MPIC. Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Scott Wood <scottwood@freescale.com> Cc: Hongtao Jia <hongtao.jia@freescale.com> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: linuxppc-dev@lists.ozlabs.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/powerpc/sysdev/mpic.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)