Hello Sam, On 9/10/18 8:03 AM, Sam Bobroff wrote: > Hi Sergey, > > On Thu, Sep 06, 2018 at 02:57:52PM +0300, Sergey Miroshnichenko wrote: >> Reading an empty slot returns all ones, which triggers a false >> EEH error event on PowerNV. >> >> New callbacks pcibios_rescan_prepare/done are introduced to >> pause/resume the EEH during rescan. > > If I understand it correctly, this temporarily disables EEH for config space > accesses on the whole PHB while the rescan runs. Is it possible that a > real EEH event could be missed if it occurred during the rescan? > > Even if it's not possible, I think it would be good to mention that in a > comment.
Yes, missing a real EEH event is possible, unfortunately, and it is indeed worth mentioning. To reduce this probability the next patchset I'll post in a few days among other things puts all the affected device drivers to pause during rescan, mainly because of moving BARs and bridge windows, but it will also help here a bit. > >> Signed-off-by: Sergey Miroshnichenko <s.miroshniche...@yadro.com> >> --- >> arch/powerpc/include/asm/eeh.h | 2 ++ >> arch/powerpc/kernel/eeh.c | 12 +++++++++++ >> arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++++++ >> drivers/pci/probe.c | 14 +++++++++++++ >> include/linux/pci.h | 2 ++ >> 5 files changed, 52 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h >> index 219637ea69a1..926c3e31df99 100644 >> --- a/arch/powerpc/include/asm/eeh.h >> +++ b/arch/powerpc/include/asm/eeh.h >> @@ -219,6 +219,8 @@ struct eeh_ops { >> int (*next_error)(struct eeh_pe **pe); >> int (*restore_config)(struct pci_dn *pdn); >> int (*notify_resume)(struct pci_dn *pdn); >> + int (*pause)(struct pci_bus *bus); >> + int (*resume)(struct pci_bus *bus); > > I think these names are a bit too generic, what about naming them > pause_bus()/resume_bus() or even prepare_rescan()/rescan_done()? > Thanks! I will rename them to rescan_prepare/rescan_done to make friends with reset_prepare/reset_done from struct pci_error_handlers. >> }; >> >> extern int eeh_subsystem_flags; >> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c >> index 6ebba3e48b01..9fb5012f389d 100644 >> --- a/arch/powerpc/kernel/eeh.c >> +++ b/arch/powerpc/kernel/eeh.c >> @@ -1831,3 +1831,15 @@ static int __init eeh_init_proc(void) >> return 0; >> } >> __initcall(eeh_init_proc); >> + >> +void pcibios_rescan_prepare(struct pci_bus *bus) >> +{ >> + if (eeh_ops && eeh_ops->pause) >> + eeh_ops->pause(bus); >> +} >> + >> +void pcibios_rescan_done(struct pci_bus *bus) >> +{ >> + if (eeh_ops && eeh_ops->resume) >> + eeh_ops->resume(bus); >> +} >> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c >> b/arch/powerpc/platforms/powernv/eeh-powernv.c >> index 3c1beae29f2d..9724a58afcd2 100644 >> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c >> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c >> @@ -59,6 +59,26 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev) >> eeh_sysfs_add_device(pdev); >> } >> >> +static int pnv_eeh_pause(struct pci_bus *bus) >> +{ >> + struct pci_controller *hose = pci_bus_to_host(bus); >> + struct pnv_phb *phb = hose->private_data; >> + >> + phb->flags &= ~PNV_PHB_FLAG_EEH; >> + disable_irq(eeh_event_irq); >> + return 0; >> +} >> + >> +static int pnv_eeh_resume(struct pci_bus *bus) >> +{ >> + struct pci_controller *hose = pci_bus_to_host(bus); >> + struct pnv_phb *phb = hose->private_data; >> + >> + enable_irq(eeh_event_irq); >> + phb->flags |= PNV_PHB_FLAG_EEH; >> + return 0; >> +} >> + >> static int pnv_eeh_init(void) >> { >> struct pci_controller *hose; >> @@ -1710,6 +1730,8 @@ static struct eeh_ops pnv_eeh_ops = { >> .write_config = pnv_eeh_write_config, >> .next_error = pnv_eeh_next_error, >> .restore_config = pnv_eeh_restore_config, >> + .pause = pnv_eeh_pause, >> + .resume = pnv_eeh_resume, >> .notify_resume = NULL >> }; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index ac876e32de4b..4a9045364809 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -2801,6 +2801,14 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) >> { >> } >> >> +void __weak pcibios_rescan_prepare(struct pci_bus *bus) >> +{ >> +} >> + >> +void __weak pcibios_rescan_done(struct pci_bus *bus) >> +{ >> +} >> + >> struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> @@ -3055,9 +3063,15 @@ unsigned int pci_rescan_bus_bridge_resize(struct >> pci_dev *bridge) >> unsigned int pci_rescan_bus(struct pci_bus *bus) >> { >> unsigned int max; >> + struct pci_bus *root = bus; >> + >> + while (!pci_is_root_bus(root)) >> + root = root->parent; >> >> + pcibios_rescan_prepare(root); >> max = pci_scan_child_bus(bus); >> pci_assign_unassigned_bus_resources(bus); >> + pcibios_rescan_done(root); >> pci_bus_add_devices(bus); >> >> return max; >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 340029b2fb38..42930731c5a7 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1929,6 +1929,8 @@ void pcibios_penalize_isa_irq(int irq, int active); >> int pcibios_alloc_irq(struct pci_dev *dev); >> void pcibios_free_irq(struct pci_dev *dev); >> resource_size_t pcibios_default_alignment(void); >> +void pcibios_rescan_prepare(struct pci_bus *bus); >> +void pcibios_rescan_done(struct pci_bus *bus); >> >> #ifdef CONFIG_HIBERNATE_CALLBACKS >> extern struct dev_pm_ops pcibios_pm_ops; >> -- >> 2.17.1 >> Best regards, Serge
signature.asc
Description: OpenPGP digital signature