linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Linux PM list <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Dmitry Torokhov <dtor@google.com>,
	Aubrey Li <aubrey.li@intel.com>
Subject: Re: [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts
Date: Fri, 29 Aug 2014 00:44:11 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.10.1408282355350.23342@nanos> (raw)
In-Reply-To: <5320472.coYotHR1d0@vostro.rjw.lan>

On Wed, 27 Aug 2014, Rafael J. Wysocki wrote:
> To me, all of this is relatively straightforward and the handling of
> IRQF_NO_SUSPEND for shared interrupts, which is a separate problem, can be
> addressed on top of it later (make no mistake, I still think that it should be
> addressed).

Why? Just because there is enough idiotic code using it?

Let's look at the usage sites:

sound/soc/codecs/twl6040.c

  Introduced in commit 8d61f4901f83461e1f04df7743777e9db5f541aa

    ASoC: twl6040: Convert PLUGINT to no-suspend irq
    
    Convert headset PLUGINT interrupt to NO_SUSPEND type in order to
    allow handling of insertion/removal events while device is
    suspended.

  So why does this need to be a NO_SUSPEND type interrupt? Just because
  the flag is sexy? What's wrong with using the wake mechanism?

drivers/xen/events/events_base.c

  A genuine usecase which makes sense and is not shared

drivers/watchdog/intel-mid_wdt.c

  MUHAHAHAHAHAHA

  static irqreturn_t mid_wdt_irq(int irq, void *dev_id)
  {
      panic("Kernel Watchdog");
      /* This code should not be reached */
      return IRQ_HANDLED;
  }

  So why does this need to be NO_SUSPEND? Because we forgot to trigger
  the watchdog during suspend? Brilliant!

drivers/usb/phy/phy-ab8500-usb.c

  Of course no explanation WHY this uses NO_SUSPEND plus SHARED and
  there is no fcking reason to do so.

So really, I'm too lazy to walk through that mess further. I bet NONE
of the usage sites except those for which this has been introduced in
the first place has a real good reason to do so.

Unless someone comes up with a use case where a shared NO_SUSPEND
handler is absolutely required, there is nothing which needs to be
addressed. You've proven yourself, that the wake mechanism is
sufficient to solve the problem which led to this discussion.

So we rather go and fix the ABUSE instead of making it legitimate by
fugly workarounds in the core code.

The same applies to the IRQF_RESUME_EARLY flag.

Just look at:

  commit 8b41669ceba0c2d4c09d69ccb9a3458953dae784

    mfd: twl4030: Fix chained irq handling on resume from suspend
    
    The irqs are enabled one-by-one in pm core resume_noirq phase.
    This leads to situation where the twl4030 primary interrupt
    handler (PIH) is enabled before the chained secondary handlers
    (SIH). As the PIH cannot clear the pending interrupt, and
    SIHs have not been enabled yet, a flood of interrupts hangs
    the device.
    
    Fixed the issue by setting the SIH irqs with IRQF_EARLY_RESUME
    flags, so they get enabled before the PIH.

So we solve an ordering problem which has a completely different root
cause by slapping random flags on it which paper over the issue?

The solution to the problem is completely wrong and of course the
"fix" is only applied to the one instance which might be affected by
that issue, i.e. the one which caused the patch submitter trouble.

Now I don't blame the author, I blame the maintainer who happily
applied that "fix".

That's unfortunately a very common pattern in the kernel which will
cause serious maintainability issues in the long run, but of course
that's just the opinion of a grumpy old greybeard.

The sad thing is that neither the author nor the maintainer who
applied it is going to be around and responsive when the shit hits the
fan. That commit is a perfect example for this.

Thanks,

	tglx

  parent reply	other threads:[~2014-08-28 22:44 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-11 13:56 [PATCH 0/6 v2] irq / PM: Shared IRQs vs IRQF_NO_SUSPEND and suspend-to-idle " Rafael J. Wysocki
2014-08-11 13:58 ` [PATCH 1/6 v2] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-08-11 13:59 ` [PATCH 2/6 v2] irq / PM: Make IRQF_NO_SUSPEND work with shared interrupts Rafael J. Wysocki
2014-08-11 14:00 ` [PATCH 3/6 v2] irq / PM: Make wakeup interrupts work with suspend-to-idle Rafael J. Wysocki
2014-08-11 14:01 ` [PATCH 4/6 v2] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-08-11 14:02 ` [PATCH 5/6 v2] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-08-11 14:03 ` [PATCH 6/6 v2] irq / PM: Document rules related to system suspend and interrupts Rafael J. Wysocki
2014-08-26 23:46 ` [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts Rafael J. Wysocki
2014-08-26 23:47   ` [PATCH 1/5 v3] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-08-26 23:49   ` [PATCH 2/5 v3] irq / PM: Make wakeup interrupts work with suspend-to-idle Rafael J. Wysocki
2014-08-27 20:32     ` Thomas Gleixner
2014-08-27 22:51       ` Rafael J. Wysocki
2014-08-28  9:23         ` Thomas Gleixner
2014-08-29  1:51           ` Rafael J. Wysocki
2014-08-26 23:50   ` [PATCH 3/5 v3] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-08-26 23:51   ` [PATCH 4/5 v3] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-08-26 23:52   ` [PATCH 5/5 v3] irq / PM: Document rules related to system suspend and interrupts Rafael J. Wysocki
2014-08-28 22:44   ` Thomas Gleixner [this message]
2014-08-29  0:54     ` [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts Rafael J. Wysocki
2014-08-29  1:09       ` Thomas Gleixner
2014-09-01 14:18   ` [PATCH 00/13] genirq / PM: Wakeup interrupts handling rework (related to suspend-to-idle) Rafael J. Wysocki
2014-09-01 14:19     ` [PATCH 01/13] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-09-01 14:20     ` [PATCH 02/13] genirq: Move suspend/resume logic into irq/pm code Rafael J. Wysocki
2014-09-01 14:21     ` [PATCH 03/13] genirq: Add sanity checks for PM options on shared interrupt lines Rafael J. Wysocki
2014-09-01 14:22     ` [PATCH 04/13] genirq: Make use of pm misfeature accounting Rafael J. Wysocki
2014-09-01 14:22     ` [PATCH 05/13] genirq: Move MASK_ON_SUSPEND handling into suspend_device_irqs() Rafael J. Wysocki
2014-09-01 14:23     ` [PATCH 06/13] genirq: Avoid double loop on suspend Rafael J. Wysocki
2014-09-01 14:24     ` [PATCH 07/13] genirq: Distangle edge handler entry Rafael J. Wysocki
2014-09-01 14:24     ` [PATCH 08/13] genirq: Create helper for flow handler entry check Rafael J. Wysocki
2014-09-01 14:26     ` [PATCH 09/13] genirq: Mark wakeup sources as armed on suspend Rafael J. Wysocki
2014-09-01 14:27     ` [PATCH 10/13] genirq: Simplify wakeup mechanism Rafael J. Wysocki
2014-09-01 14:28     ` [PATCH 11/13] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-09-01 14:28     ` [PATCH 12/13] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-09-01 14:29     ` [PATCH 13/13] PM / genirq: Document rules related to system suspend and interrupts Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.10.1408282355350.23342@nanos \
    --to=tglx@linutronix.de \
    --cc=aubrey.li@intel.com \
    --cc=dtor@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --subject='Re: [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).