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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
