| Message ID | 1442850433-5903-5-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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(-)