[PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

The device is set as wakeup capable using proper wakeup API but the
driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
which is incorrect.

This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
enable_irq_wake instead.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 4b69d061d90f..803ed4c93503 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1970,8 +1970,7 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 		/* Install our interrupt handlers for Error,
 		 * Transmit, and Receive
 		 */
-		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error,
-				  IRQF_NO_SUSPEND,
+		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
 				  gfar_irq(grp, ER)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
@@ -1979,6 +1978,8 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 
 			goto err_irq_fail;
 		}
+		enable_irq_wake(gfar_irq(grp, ER)->irq);
+
 		err = request_irq(gfar_irq(grp, TX)->irq, gfar_transmit, 0,
 				  gfar_irq(grp, TX)->name, grp);
 		if (err < 0) {
@@ -1994,14 +1995,14 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 			goto rx_irq_fail;
 		}
 	} else {
-		err = request_irq(gfar_irq(grp, TX)->irq, gfar_interrupt,
-				  IRQF_NO_SUSPEND,
+		err = request_irq(gfar_irq(grp, TX)->irq, gfar_interrupt, 0,
 				  gfar_irq(grp, TX)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
 				  gfar_irq(grp, TX)->irq);
 			goto err_irq_fail;
 		}
+		enable_irq_wake(gfar_irq(grp, TX)->irq);
 	}
 
 	return 0;
-- 
1.9.1

RE: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Manoil Claudiu]

>-----Original Message-----
>From: Sudeep Holla [mailto:sudeep.holla@arm.com]
>Sent: Monday, September 21, 2015 6:47 PM
>To: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>Cc: Sudeep Holla <sudeep.holla@arm.com>; Thomas Gleixner
><tglx@linutronix.de>; Rafael J. Wysocki <rjw@rjwysocki.net>; David S. Miller
><davem@davemloft.net>; Manoil Claudiu-B08782
><claudiu.manoil@freescale.com>; Kevin Hao <haokexin@gmail.com>;
>netdev@vger.kernel.org
>Subject: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND
>flag
>
>The device is set as wakeup capable using proper wakeup API but the
>driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
>which is incorrect.
>
>This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>enable_irq_wake instead.
>

What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
friendlier API compared to calling enable_irq_wake().  For older kernels,
on PPC architectures, the flag did the job.  When did this change? Since
when using IRQF_NO_SUSPEND is a "misuse"?

Thanks,
Claudiu

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 21/09/15 17:24, Manoil Claudiu wrote:
>> -----Original Message-----
>> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
>> Sent: Monday, September 21, 2015 6:47 PM
>> To: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Sudeep Holla <sudeep.holla@arm.com>; Thomas Gleixner
>> <tglx@linutronix.de>; Rafael J. Wysocki <rjw@rjwysocki.net>; David S. Miller
>> <davem@davemloft.net>; Manoil Claudiu-B08782
>> <claudiu.manoil@freescale.com>; Kevin Hao <haokexin@gmail.com>;
>> netdev@vger.kernel.org
>> Subject: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND
>> flag
>>
>> The device is set as wakeup capable using proper wakeup API but the
>> driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
>> which is incorrect.
>>
>> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>> enable_irq_wake instead.
>>
>
> What would be the purpose of IRQF_NO_SUSPEND flag then?

If you look at the section "IRQF_NO_SUSPEND Flag" in
Documentation/power/suspend-and-interrupts.txt, it states it doesn't
guarantee that the interrupt will wake the system from a suspended state
-- for such cases it is necessary to use enable_irq_wake().

> The flag is a friendlier API compared to calling enable_irq_wake().

In what sense ?

> For older kernels, on PPC architectures, the flag did the job.

Agreed, it does the job even now, just that it's not designed for that.
One can change that anytime internally as the APIs for configuring
wakeups as in place now. I am just making use of right APIs.

> When did this change? Since when using IRQF_NO_SUSPEND is a "misuse"?
>

Sorry if the term "misuse" is not appropriate here, since the intention
of the flag usage doesn't meet the requirements, I mentioned it as misuse.

Regards,
Sudeep

RE: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Thomas Gleixner]

On Mon, 21 Sep 2015, Manoil Claudiu wrote:
> >The device is set as wakeup capable using proper wakeup API but the
> >driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
> >which is incorrect.
> >
> >This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
> >enable_irq_wake instead.
> >
> 
> What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
> friendlier API compared to calling enable_irq_wake().  For older kernels,

It's not an API, it's just a bandaid for lazy programmers.

> on PPC architectures, the flag did the job.  When did this change? Since
> when using IRQF_NO_SUSPEND is a "misuse"?

It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
to do with wakeup interrupt sources. It's a flag which excludes the
interrupt from the suspend mechanism, but it does not flag it a wakeup
source.

Thanks,

	tglx

RE: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Manoil Claudiu]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de]
>Sent: Monday, September 21, 2015 7:52 PM
>To: Manoil Claudiu-B08782 <claudiu.manoil@freescale.com>
>Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-pm@vger.kernel.org; linux-
>kernel@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>; David S.
>Miller <davem@davemloft.net>; Kevin Hao <haokexin@gmail.com>;
>netdev@vger.kernel.org
>Subject: RE: [PATCH 13/17] net: gianfar: remove misuse of
>IRQF_NO_SUSPEND flag
>
>On Mon, 21 Sep 2015, Manoil Claudiu wrote:
>> >The device is set as wakeup capable using proper wakeup API but the
>> >driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
>> >which is incorrect.
>> >
>> >This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>> >enable_irq_wake instead.
>> >
>>
>> What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
>> friendlier API compared to calling enable_irq_wake().  For older kernels,
>
>It's not an API, it's just a bandaid for lazy programmers.
>
>> on PPC architectures, the flag did the job.  When did this change? Since
>> when using IRQF_NO_SUSPEND is a "misuse"?
>
>It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
>to do with wakeup interrupt sources. It's a flag which excludes the
>interrupt from the suspend mechanism, but it does not flag it a wakeup
>source.
>

I'm seeing also a "powerpc: mpic" patch in the series, unfortunately I can't
afford to test it right now. However I ran a quick test with this gianfar patch
in isolation on a powerpc system, and seen some difference in the behavior
(with and w/o the patch).  In both cases the system wakes up from standby
by magic packet.  However, without the IRQF_NO_SUSPEND flag 2 wake-up
interrupts are reported in /proc/interrupts for one magic packet; with the
flag on there's just 1 interrupt.  Maybe this is not relevant, maybe the
"powerpc: mpic" patch from this series changes this behavior.
But if this is the API, what can I say? We'll see in time.  Btw, enable_irq_wake()
returns an error code, normally it should be handled by printing a warning
message at least, right?  But since most drivers don't handle that, I'm assuming
it should be left unhandled to avoid overcomplicating things.
FWIW
Acked-by: Claudiu Manoil <claudiu.manoil@freescale.com>

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 22/09/15 15:04, Manoil Claudiu wrote:
>> -----Original Message-----
>> From: Thomas Gleixner [mailto:tglx@linutronix.de]

[...]

>>> on PPC architectures, the flag did the job.  When did this change? Since
>>> when using IRQF_NO_SUSPEND is a "misuse"?
>>
>> It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
>> to do with wakeup interrupt sources. It's a flag which excludes the
>> interrupt from the suspend mechanism, but it does not flag it a wakeup
>> source.
>>
>
> I'm seeing also a "powerpc: mpic" patch in the series, unfortunately I can't

Yes I think that was a redundant code, so I removed it. IIRC it was
setting IRQF_NO_SUSPEND in irq_set_wake callback which again is incorrect.

> afford to test it right now. However I ran a quick test with this gianfar patch
> in isolation on a powerpc system, and seen some difference in the behavior
> (with and w/o the patch).  In both cases the system wakes up from standby
> by magic packet.  However, without the IRQF_NO_SUSPEND flag 2 wake-up
> interrupts are reported in /proc/interrupts for one magic packet; with the

OK that's interesting, will have check if I have similar behavior on my
setup too.

> flag on there's just 1 interrupt.  Maybe this is not relevant, maybe the
> "powerpc: mpic" patch from this series changes this behavior.

Hmm not sure, but better to test it together if possible. If required we
can reorder for bisect-ability reasons.

> But if this is the API, what can I say? We'll see in time.  Btw, enable_irq_wake()
> returns an error code, normally it should be handled by printing a warning
> message at least, right?  But since most drivers don't handle that, I'm assuming
> it should be left unhandled to avoid overcomplicating things.

Yes I left it so that I can add if the maintainer insist and not churn
too much code adding warning.

> FWIW
> Acked-by: Claudiu Manoil <claudiu.manoil@freescale.com>
>

Thanks.

Regards,
Sudeep

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Li Yang]

On Mon, Sep 21, 2015 at 11:51 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 21 Sep 2015, Manoil Claudiu wrote:
>> >The device is set as wakeup capable using proper wakeup API but the
>> >driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
>> >which is incorrect.
>> >
>> >This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>> >enable_irq_wake instead.
>> >
>>
>> What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
>> friendlier API compared to calling enable_irq_wake().  For older kernels,
>
> It's not an API, it's just a bandaid for lazy programmers.
>
>> on PPC architectures, the flag did the job.  When did this change? Since
>> when using IRQF_NO_SUSPEND is a "misuse"?
>
> It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
> to do with wakeup interrupt sources. It's a flag which excludes the
> interrupt from the suspend mechanism, but it does not flag it a wakeup
> source.

It was not very clear on the intended use of IRQF_NO_SUSPEND at the
beginning as it was not documented anywhere.  It's good that we have
the Documentation/power/suspend-and-interrupts.txt to clarify that
now.

Regards,
Leo

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Thomas Gleixner]

On Wed, 23 Sep 2015, Li Yang wrote:
> On Mon, Sep 21, 2015 at 11:51 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 21 Sep 2015, Manoil Claudiu wrote:
> >> >The device is set as wakeup capable using proper wakeup API but the
> >> >driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
> >> >which is incorrect.
> >> >
> >> >This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
> >> >enable_irq_wake instead.
> >> >
> >>
> >> What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
> >> friendlier API compared to calling enable_irq_wake().  For older kernels,
> >
> > It's not an API, it's just a bandaid for lazy programmers.
> >
> >> on PPC architectures, the flag did the job.  When did this change? Since
> >> when using IRQF_NO_SUSPEND is a "misuse"?
> >
> > It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
> > to do with wakeup interrupt sources. It's a flag which excludes the
> > interrupt from the suspend mechanism, but it does not flag it a wakeup
> > source.
> 
> It was not very clear on the intended use of IRQF_NO_SUSPEND at the
> beginning as it was not documented anywhere.  It's good that we have
> the Documentation/power/suspend-and-interrupts.txt to clarify that
> now.

This is complete and utter bullshit to put it blunt.

The documentation of IRQF_NO_SUSPEND as non suitable for marking a
particular device interrupt as wakeup source was added on Feb 20 2015
with commit 737eb0301f2. Further clarification was added with commit
7438b633a6b on Mar 4 2015.

The IRQF_NO_SUSPEND crap was introduced to gianfar with commit
614b42426cc3 on Jul 31 2015.

Did you really expect that nobody will notice that the documentation
which you praise now, has been available FIVE month before you decided
to ignore it?

You can play your $corp fingerpointing games inside of your $corp
without someone noticing, but in that community this is an absolute
nono.

No thanks,

   tglx

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[Li Yang]

On Wed, Sep 23, 2015 at 5:03 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 23 Sep 2015, Li Yang wrote:
>> On Mon, Sep 21, 2015 at 11:51 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Mon, 21 Sep 2015, Manoil Claudiu wrote:
>> >> >The device is set as wakeup capable using proper wakeup API but the
>> >> >driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
>> >> >which is incorrect.
>> >> >
>> >> >This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>> >> >enable_irq_wake instead.
>> >> >
>> >>
>> >> What would be the purpose of IRQF_NO_SUSPEND flag then?  The flag is a
>> >> friendlier API compared to calling enable_irq_wake().  For older kernels,
>> >
>> > It's not an API, it's just a bandaid for lazy programmers.
>> >
>> >> on PPC architectures, the flag did the job.  When did this change? Since
>> >> when using IRQF_NO_SUSPEND is a "misuse"?
>> >
>> > It always was. Simply because IRQF_NO_SUSPEND has absolutely nothing
>> > to do with wakeup interrupt sources. It's a flag which excludes the
>> > interrupt from the suspend mechanism, but it does not flag it a wakeup
>> > source.
>>
>> It was not very clear on the intended use of IRQF_NO_SUSPEND at the
>> beginning as it was not documented anywhere.  It's good that we have
>> the Documentation/power/suspend-and-interrupts.txt to clarify that
>> now.
>
> This is complete and utter bullshit to put it blunt.
>
> The documentation of IRQF_NO_SUSPEND as non suitable for marking a
> particular device interrupt as wakeup source was added on Feb 20 2015
> with commit 737eb0301f2. Further clarification was added with commit
> 7438b633a6b on Mar 4 2015.
>
> The IRQF_NO_SUSPEND crap was introduced to gianfar with commit
> 614b42426cc3 on Jul 31 2015.

It is definitely an oversight of us to have missed the new development
on this topic.  It's a problem for us and I'm not trying to defend
that.  But the decision to use IRQF_NO_SUSPEND for wakeup interrupt
was made several years ago. (https://lkml.org/lkml/2012/7/20/174)  I
was just trying to say that it's hard to make the right decision at
the beginning.

Regards,
Leo

Re: [PATCH 13/17] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag

[David Miller]

From: Sudeep Holla <sudeep.holla@arm.com>
Date: Mon, 21 Sep 2015 16:47:09 +0100

> The device is set as wakeup capable using proper wakeup API but the
> driver misuses IRQF_NO_SUSPEND to set the interrupt as wakeup source
> which is incorrect.
> 
> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
> enable_irq_wake instead.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
> Cc: Kevin Hao <haokexin@gmail.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Applied, thanks.