On Tue, Dec 04, 2018 at 10:42:19PM +0100, Rafael J. Wysocki wrote:

On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:

quoted

On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:

quoted

On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:

quoted

Hi Lorenzo,

Lorenzo Pieralisi [off-list ref] wrote on Mon, 3 Dec 2018
10:27:08 +0000:

quoted

[+Rafael, Sudeep]

On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:

quoted

Add suspend and resume callbacks. The priority of these are
"_noirq()", to workaround early access to the registers done by the
PCI core through the ->read()/->write() callbacks at resume time.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 108b3f15c410..7ecf1ac4036b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c

@@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pci
 	return ret;
 }
 
+static int __maybe_unused advk_pcie_suspend(struct device *dev)
+{
+	struct advk_pcie *pcie = dev_get_drvdata(dev);
+
+	advk_pcie_disable_phy(pcie);
+
+	clk_disable_unprepare(pcie->clk);  

I have noticed it is common practice, still, I would like to check whether
it is allowed to call functions that may sleep in a NOIRQ suspend/resume
callback ?

You are right this is weird. I double checked and for instance,
pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
probably other cases where drivers call functions that may sleep from a NOIRQ
context. I am interested to know if this is valid and if not, what is the
alternative?

Yes, it is valid.  _noirq means that the high-level action handlers
will not be invoked for interrupts occurring during that period, but
that doesn't apply to timer interrupts.

IOW, don't expect *your* IRQ handler to be invoked then (if this is
not a timer IRQ), but you can sleep.

Hi Rafael, all,

I did not ask my question (that may be silly) properly apologies. I know
that the S2R context allows sleeping the question is, in case
clk_disable_unprepare() (and resume counterparts) sleeps,

If it just sleeps, then this is not a problem, but if it actually *waits*
for something meaningful to happen (which I guess is what you really mean),
then things may go awry.

That's what I meant and I assume that's reason why the
*_prepare/unprepare() API are allowed to sleep, waiting for
an event to trigger.

quoted

what is going to wake it up, given that we are in the S2R NOIRQ phase and as
you said the action handlers (that are possibly required to wake up the eg
clk_disable_unprepare() caller) are disabled (unless, AFAIK,
IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).

So if it waits for an action handler to do something and wake it up, it may
very well deadlock.  I have no idea if that really happens, though.

I have no idea either, that's why I asked the clock maintainers too,
the point is, the clk API allows this behaviour, I do not think it is safe
to rely on a non-blocking clk back-end, that's why I raised the question
in the first place.

quoted

The clk API implementations back-ends are beyond my depth, I just wanted
to make sure I understand how the S2R flow is expected to work in this
specific case.

Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
(well, there are a few more complications I don't recall exactly, but
that's the basic rule).  If anything depends on them to run, it will block.

It looks like this patch (and more code in the kernel) expects either
the clk calls not to block or the action handler implemented in the
clock back-ends to run in S2R-NOIRQ (possibly by using the
IRQF_NO_SUSPEND flag).

I would appreciate if the clock maintainers can shed some light on
this.

Thanks,
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel