[v3] PCI: rcar: Check if device is runtime suspended instead of __clk_is_enabled()

Commit Message

Marek Vasut Nov. 7, 2021, 7:10 p.m. UTC

From: Marek Vasut <[e-mail]>

Replace __clk_is_enabled() with pm_runtime_suspended(),
otherwise the following build error occurs:
  arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
  pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'

Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
Signed-off-by: Marek Vasut <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Wolfram Sang <[e-mail]>
Cc: Yoshihiro Shimoda <[e-mail]>
Cc: [e-mail]
---
V2: Drop the __clk_is_enabled(), like it was done already in V1 of
    a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
V3: Use pm_runtime_suspended() instead of __clk_is_enabled()
---
 drivers/pci/controller/pcie-rcar-host.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Krzysztof Wilczyński Nov. 7, 2021, 8:11 p.m. UTC | #1

[+CC Randy as he sent a patch to fix the same thing]

Hi Marek,

> Replace __clk_is_enabled() with pm_runtime_suspended(),
> otherwise the following build error occurs:
>   arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
>   pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'
> 
> Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")

Randy sent a patch which aims to fix the same issue, albeit in a slightly
different way.  I wonder if it would make sense for the two of you to look
at which approach should we pick, or even whether we should combine the
two into one patch?

  https://lore.kernel.org/linux-pci/[e-mail]/

	Krzysztof

Randy Dunlap Nov. 7, 2021, 9:01 p.m. UTC | #2

On 11/7/21 12:11 PM, Krzysztof Wilczyński wrote:
> [+CC Randy as he sent a patch to fix the same thing]
> 
> Hi Marek,
> 
>> Replace __clk_is_enabled() with pm_runtime_suspended(),
>> otherwise the following build error occurs:
>>    arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
>>    pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'
>>
>> Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
> 
> Randy sent a patch which aims to fix the same issue, albeit in a slightly
> different way.  I wonder if it would make sense for the two of you to look
> at which approach should we pick, or even whether we should combine the
> two into one patch?
> 
>    https://lore.kernel.org/linux-pci/[e-mail]/

Hi,
I saw Marek's patch a couple of hours ago.

As long as pm_runtime_suspended() is always available,
either live or as a stub (and it looks like it is),
I don't see any reason for my patch at all.

thanks.

Krzysztof Wilczyński Nov. 7, 2021, 10:33 p.m. UTC | #3

Hi Randy,

[...]
> > Randy sent a patch which aims to fix the same issue, albeit in a slightly
> > different way.  I wonder if it would make sense for the two of you to look
> > at which approach should we pick, or even whether we should combine the
> > two into one patch?
> > 
> >    https://lore.kernel.org/linux-pci/[e-mail]/
> 
> Hi,
> I saw Marek's patch a couple of hours ago.
> 
> As long as pm_runtime_suspended() is always available,
> either live or as a stub (and it looks like it is),
> I don't see any reason for my patch at all.

Understood!  I marked it accordingly in Patchwok.  Thank you both!

By the way, can I count on your Acked-by or Reviewed-by here for Marek?  If
you agree with the premise of the changes here, etc., of course.

	Krzysztof

Randy Dunlap Nov. 7, 2021, 10:34 p.m. UTC | #4

On 11/7/21 2:33 PM, Krzysztof Wilczyński wrote:
> Hi Randy,
> 
> [...]
>>> Randy sent a patch which aims to fix the same issue, albeit in a slightly
>>> different way.  I wonder if it would make sense for the two of you to look
>>> at which approach should we pick, or even whether we should combine the
>>> two into one patch?
>>>
>>>     https://lore.kernel.org/linux-pci/[e-mail]/
>>
>> Hi,
>> I saw Marek's patch a couple of hours ago.
>>
>> As long as pm_runtime_suspended() is always available,
>> either live or as a stub (and it looks like it is),
>> I don't see any reason for my patch at all.
> 
> Understood!  I marked it accordingly in Patchwok.  Thank you both!
> 
> By the way, can I count on your Acked-by or Reviewed-by here for Marek?  If
> you agree with the premise of the changes here, etc., of course.

Sure.
Acked-by: Randy Dunlap <[e-mail]>

thanks.

Geert Uytterhoeven Nov. 8, 2021, 9:53 a.m. UTC | #5

Hi Marek,

Thanks for your patch!

On Sun, Nov 7, 2021 at 8:11 PM <[e-mail]> wrote:
> From: Marek Vasut <[e-mail]>
>
> Replace __clk_is_enabled() with pm_runtime_suspended(),
> otherwise the following build error occurs:
>   arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
>   pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'

While this describes what is done and why, it misses one important
semantic change: the old __clk_is_enabled() actually checked the wrong
clock (bus clock instead of module clock), while pm_runtime_suspended()
reflects (a.o.) the state of the module clock.

>
> Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
> Signed-off-by: Marek Vasut <[e-mail]>

Reviewed-by: Geert Uytterhoeven <[e-mail]>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Krzysztof Wilczyński Nov. 8, 2021, 11:14 a.m. UTC | #6

[+CC Adding Sasha for visibiity]

Hi Geert,

[...]
> > Replace __clk_is_enabled() with pm_runtime_suspended(),
> > otherwise the following build error occurs:
> >   arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
> >   pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'
> 
> While this describes what is done and why, it misses one important
> semantic change: the old __clk_is_enabled() actually checked the wrong
> clock (bus clock instead of module clock), while pm_runtime_suspended()
> reflects (a.o.) the state of the module clock.

This looks like it was a latent bug then, something that we might need to
port back to the stable and long-term kernels, I believe.

Thank you, Geert!

	Krzysztof

Patch

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index e12c2d8be05a..138592e22600 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -50,10 +50,10 @@  struct rcar_msi {
  */
 static void __iomem *pcie_base;
 /*
- * Static copy of bus clock pointer, so we can check whether the clock
- * is enabled or not.
+ * Static copy of pcie device pointer, so we can check whether the
+ * device is runtime suspended or not.
  */
-static struct clk *pcie_bus_clk;
+static struct device *pcie_dev;
 #endif
 
 /* Structure representing the PCIe interface */
@@ -792,7 +792,7 @@  static int rcar_pcie_get_resources(struct rcar_pcie_host *host)
 #ifdef CONFIG_ARM
 	/* Cache static copy for L1 link state fixup hook on aarch32 */
 	pcie_base = pcie->base;
-	pcie_bus_clk = host->bus_clk;
+	pcie_dev = pcie->dev;
 #endif
 
 	return 0;
@@ -1062,7 +1062,7 @@  static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 
 	spin_lock_irqsave(&pmsr_lock, flags);
 
-	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
+	if (!pcie_base || pm_runtime_suspended(pcie_dev)) {
 		ret = 1;
 		goto unlock_exit;
 	}