Re: [PATCH v3 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_state hook

[Bryan O'Donoghue]

On Tue, 2016-10-25 at 08:19 +0200, Lukas Wunner wrote:
> On Mon, Oct 24, 2016 at 02:05:45PM +0300, Andy Shevchenko wrote:
> > 
> > On Mon, 2016-10-24 at 12:09 +0200, Lukas Wunner wrote:
> > > 
> > > I note that you're exporting intel_mid_pci_set_power_state() even
> > > though there's currently no module user, so perhaps you're
> > > intending
> > > to call the function from somewhere else.
> > The export there is purely dictated by leaving abstract stuff under
> > drivers/pci when platform code is kept under arch/x86/platform.
> > Other
> > than that there is no plans to call this outside of pci-mid.c.
> The PCI core, including pci-mid.o, is linked into vmlinux, not a
> module,
> and exporting is only needed for module users.ÂÂA commit to unexport
> the
> symbol has been sitting on my development branch for 2 weeks, I
> delayed
> it because I wanted to wait for the regression to be fixed first.
> Sending out now since the topic has come up.
> 
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > The usage of a mutex in mid_pwr_set_power_state() actually
> > > > > seems
> > > > > questionable since this is called with interrupts disabled:
> > > > > 
> > > > > pci_pm_resume_noirq
> > > > > Â pci_pm_default_resume_early
> > > > > ÂÂÂÂpci_power_up
> > > > > ÂÂÂÂÂÂplatform_pci_set_power_state
> > > > > ÂÂÂÂÂÂÂÂmid_pci_set_power_state
> > > > > ÂÂÂÂÂÂÂÂÂÂintel_mid_pci_set_power_state
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂmid_pwr_set_power_state
> > > > Hmm... I have to look at this closer. I don't remember why I
> > > > put
> > > > mutex
> > > > in the first place there. Anyway it's another story.
> > There are two code paths
> > pci_power_up()
> > pci_platform_power_transition()
> > 
> > Second one can be called in non-atomic context for sure (consider
> > standard ->resume() callback).
> > 
> > First one runs when IRQ disabled on CPU side.
> > 
> > In any case we probably need to serialize access in our code to
> > protect
> > against several PCI devices being powered up simultaneously.
> Right, good point, the PM core will indeed parallelize suspend/resume
> of the devices, so if __update_power_state() cannot be safely called
> concurrently for different devices (I don't know if that's the case)
> then indeed you need locking (with spinlocks).ÂÂIt might be worth
> pondering if the locking should happen further down in the call stack
> because I assume the critical section would really just be in
> __update_power_state().
> 
> Best regards,
> 
> Lukas

So the conclusion is to apply this patch now and go and look further @
locking in a separate series right ? There's not much point in leaving
Edison not booting as is the case with tip-of-tree right now.

---
bod