Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Bjorn Helgaas <helgaas@kernel.org>]

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@linux.intel.com>
> > Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  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);
> > +}