| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <t3la3jte5tia7rh5ftuv5cchrwdxe4cxa2v3g6lxgoh5u6rmcy@hzw7lbke2vac>
Date: Thu, 30 Oct 2025 13:16:12 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, x86@...nel.org, linux-arm-kernel@...ts.infradead.org,
linux-riscv@...ts.infradead.org, linux-mips@...r.kernel.org, loongarch@...ts.linux.dev,
linuxppc-dev@...ts.ozlabs.org, linux-sh@...r.kernel.org, linux-pci@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/7] MIPS: PCI: Use contextual data instead of global
variable
On Wed, Oct 29, 2025 at 12:46:54PM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 29, 2025 at 05:33:31PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@...dia.com>
> >
> > Pass the driver-specific data via the syscore struct and use it in the
> > syscore ops.
>
> Would be nice to include the "instead of global variable" part here so
> the commit log includes the benefit and can stand alone even without
> the subject.
Good point.
> Awesome to get rid of another global variable! More comments below.
\o/
> > +++ b/arch/mips/pci/pci-alchemy.c
> > @@ -33,6 +33,7 @@
> >
> > struct alchemy_pci_context {
> > struct pci_controller alchemy_pci_ctrl; /* leave as first member! */
> > + struct syscore syscore;
> > void __iomem *regs; /* ctrl base */
> > /* tools for wired entry for config space access */
> > unsigned long last_elo0;
> > @@ -46,12 +47,6 @@ struct alchemy_pci_context {
> > int (*board_pci_idsel)(unsigned int devsel, int assert);
> > };
> >
> > -/* for syscore_ops. There's only one PCI controller on Alchemy chips, so this
> > - * should suffice for now.
> > - */
> > -static struct alchemy_pci_context *__alchemy_pci_ctx;
> > -
> > -
> > /* IO/MEM resources for PCI. Keep the memres in sync with fixup_bigphys_addr
> > * in arch/mips/alchemy/common/setup.c
> > */
> > @@ -306,9 +301,7 @@ static int alchemy_pci_def_idsel(unsigned int devsel, int assert)
> > /* save PCI controller register contents. */
> > static int alchemy_pci_suspend(void *data)
> > {
> > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > - if (!ctx)
> > - return 0;
> > + struct alchemy_pci_context *ctx = data;
> >
> > ctx->pm[0] = __raw_readl(ctx->regs + PCI_REG_CMEM);
> > ctx->pm[1] = __raw_readl(ctx->regs + PCI_REG_CONFIG) & 0x0009ffff;
> > @@ -328,9 +321,7 @@ static int alchemy_pci_suspend(void *data)
> >
> > static void alchemy_pci_resume(void *data)
> > {
> > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > - if (!ctx)
> > - return;
> > + struct alchemy_pci_context *ctx = data;
> >
> > __raw_writel(ctx->pm[0], ctx->regs + PCI_REG_CMEM);
> > __raw_writel(ctx->pm[2], ctx->regs + PCI_REG_B2BMASK_CCH);
> > @@ -359,10 +350,6 @@ static const struct syscore_ops alchemy_pci_syscore_ops = {
> > .resume = alchemy_pci_resume,
> > };
> >
> > -static struct syscore alchemy_pci_syscore = {
> > - .ops = &alchemy_pci_syscore_ops,
> > -};
> > -
> > static int alchemy_pci_probe(struct platform_device *pdev)
> > {
> > struct alchemy_pci_platdata *pd = pdev->dev.platform_data;
> > @@ -480,9 +467,10 @@ static int alchemy_pci_probe(struct platform_device *pdev)
> > __raw_writel(val, ctx->regs + PCI_REG_CONFIG);
> > wmb();
> >
> > - __alchemy_pci_ctx = ctx;
> > platform_set_drvdata(pdev, ctx);
> > - register_syscore(&alchemy_pci_syscore);
> > + ctx->syscore.ops = &alchemy_pci_syscore_ops;
> > + ctx->syscore.data = ctx;
> > + register_syscore(&ctx->syscore);
>
> As far as I can tell, the only use of syscore in this driver is for
> suspend/resume.
>
> This is a regular platform_device driver, so instead of syscore, I
> think it should use generic power management like other PCI host
> controller drivers do, something like this:
>
> static int alchemy_pci_suspend_noirq(struct device *dev)
> ...
>
> static int alchemy_pci_resume_noirq(struct device *dev)
> ...
>
> static DEFINE_NOIRQ_DEV_PM_OPS(alchemy_pci_pm_ops,
> alchemy_pci_suspend_noirq,
> alchemy_pci_resume_noirq);
>
> static struct platform_driver alchemy_pcictl_driver = {
> .probe = alchemy_pci_probe,
> .driver = {
> .name = "alchemy-pci",
> .pm = pm_sleep_ptr(&alchemy_pci_pm_ops),
> },
> };
>
> Here's a sample in another driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/cadence/pci-j721e.c?id=v6.17#n663
I thought so too, but then I looked at the history and saw that it was
initially regular PM ops and then fixed by using syscore in this commit:
commit 864c6c22e9a5742b0f43c983b6c405d52817bacd
Author: Manuel Lauss <manuel.lauss@...glemail.com>
Date: Wed Nov 16 15:42:28 2011 +0100
MIPS: Alchemy: Fix PCI PM
Move PCI Controller PM to syscore_ops since the platform_driver PM methods
are called way too late on resume and far too early on suspend (after and
before PCI device resume/suspend).
This also allows to simplify wired entry management a bit.
Signed-off-by: Manuel Lauss <manuel.lauss@...glemail.com>
Cc: linux-mips@...ux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/3007/
Signed-off-by: Ralf Baechle <ralf@...ux-mips.org>
So unfortunately I don't think it'll work for this driver.
Thierry
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists