Message ID | 20211107191057.145467-1-marek.vasut@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | [v3] PCI: rcar: Check if device is runtime suspended instead of __clk_is_enabled() | expand |
[+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/20211107013703.19995-1-rdunlap@infradead.org/ Krzysztof
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/20211107013703.19995-1-rdunlap@infradead.org/ 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.
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/20211107013703.19995-1-rdunlap@infradead.org/ > > 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
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/20211107013703.19995-1-rdunlap@infradead.org/ >> >> 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 <rdunlap@infradead.org> thanks.
Hi Marek, Thanks for your patch! On Sun, Nov 7, 2021 at 8:11 PM <marek.vasut@gmail.com> wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > 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 <marek.vasut+renesas@gmail.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
[+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
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; }