Re: [PATCH v3 2/7] MIPS: PCI: Use contextual data instead of global variable

On Thu, Oct 30, 2025 at 01:16:12PM +0100, Thierry Reding wrote:
> 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 <[email protected]>
> > > 
> > > Pass the driver-specific data via the syscore struct and use it in the
> > > syscore ops.

> > > +++ 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 <[email protected]>
>     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 <[email protected]>
>         Cc: [email protected]
>         Patchwork: https://patchwork.linux-mips.org/patch/3007/
>         Signed-off-by: Ralf Baechle <[email protected]>

The alchemy PCI controller is a platform_device, and it must be
initialized before enumerating the PCI devices below it.  The same
order should apply for suspend/resume (suspend PCI devices, then PCI
controller; resume PCI controller, then PCI devices).

So if this didn't work before, I think it means something is messed up
with the device hierarchy.

But I understand the difficulty of testing changes here, so syscore is
simplest from that point of view.

It does complicate maintenance though.  I think all of mips ultimately
uses register_pci_controller() and pcibios_scanbus().  Neither really
contains anything mips-specific, so they duplicate a lot of the code
in pci_host_probe().  Oh well, I guess that's part of the burden of
supporting old platforms forever.

Bjorn