On Wed, Jul 21, 2021 at 10:50:26AM +0200, Pali Rohár wrote:
> On Tuesday 20 July 2021 13:50:08 Nirmal Patel wrote:
> > During VT-d passthrough repetitive reboot tests, it was determined that the VMD
> > domain needed to be reset in order to allow downstream devices to reinitialize
> > properly. This is done using a secondary bus reset at each of the VMD root
> > ports and any bridges in the domain.
> >
> > Signed-off-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
> > Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> > ---
> > drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
> > +static void vmd_domain_sbr(struct vmd_dev *vmd)
> > +{
> > + char __iomem *base;
> > + u16 ctl;
> > + int dev_seq;
> > + int max_devs = resource_size(&vmd->resources[0]) * 32;
> > +
> > + /*
> > + * Subdevice config space may or many not be mapped linearly using 4k config
> > + * space.
> > + */
> > + for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> > + base = VMD_DEVICE_BASE(vmd, dev_seq);
> > + if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
> > + continue;
> > +
> > + if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
> > + PCI_HEADER_TYPE_BRIDGE)
> > + continue;
> > +
> > + if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
> > + continue;
> > +
> > + /* pci_reset_secondary_bus() */
> > + ctl = readw(base + PCI_BRIDGE_CONTROL);
> > + ctl |= PCI_BRIDGE_CTL_BUS_RESET;
> > + writew(ctl, base + PCI_BRIDGE_CONTROL);
> > + readw(base + PCI_BRIDGE_CONTROL);
> > + msleep(2);
> > +
> > + ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> > + writew(ctl, base + PCI_BRIDGE_CONTROL);
> > + readw(base + PCI_BRIDGE_CONTROL);
>
> You cannot unconditionally call secondary bus reset for arbitrary PCIe
> Bridge. Calling it breaks more PCIe devices behind bridge and
> pci_reset_secondary_bus() already handles it and skip reset if reset is
> causing issues.
>
> I would suggest to use pci_reset_secondary_bus() and extend it
> so you can call it also from your driver.
Are you referring to PCI_DEV_FLAGS_NO_BUS_RESET? That's handled in
pci_parent_bus_reset(), not pci_reset_secondary_bus().
I would probably agree that PCI_DEV_FLAGS_NO_BUS_RESET *should* be
checked in pci_reset_secondary_bus(), since there are several paths to
get there without going through pci_parent_bus_reset().
> > + }
> > + ssleep(1);
> > +}
