Re: [igt-dev] [PATCH i-g-t 2/2] tests/device_reset: Add warm_reset test

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 10 Jan 2023 06:14:37 -0800, Gupta, Anshuman wrote:
> > On 1/9/2023 5:32 PM, Anshuman Gupta wrote:
> > > +bool igt_pci_bridge_warm_reset(struct pci_device *dev) {
> > > +	uint8_t header_type;
> > > +	uint16_t bridge_ctl;
> > > +
> > > +	if (pci_device_cfg_read_u8(dev, &header_type, PCI_HEADER_TYPE))
> > > +		return false;
> > > +
> > > +	igt_assert_f((header_type & 0x7f) == 1, "PCI device is not a
> > > +Bridge\n");
> > > +
> > > +	if (pci_device_cfg_read_u16(dev, &bridge_ctl,
> > PCI_TYPE_1_HEADER_BRIDGE_CTL))
> > > +		return false;
> > > +
> > > +	bridge_ctl |=  PCI_TYPE_1_SEC_BUS_RESET;
> > > +
> > > +	if (pci_device_cfg_write_u16(dev, bridge_ctl,
> > PCI_TYPE_1_HEADER_BRIDGE_CTL))
> > > +		return false;
> > > +
> >
> >
> > The pci code adds a minimum reset duration (Trst) delay after setting the bit.
> > Do we need to add the same here?
> > https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci.c#L5045
> Thanks for review.
> Will add delay of 2ms to keep parity with the kernel.
> >
> > Is restoring sequence not necessary?
> I think driver has the responsibility of restoring.
> I am not sure about if IGT needs to restore it.
> The test itself has sequence of unbind and bind, so driver should restore it.

Hmm, not too sure about this stuff but looks like we should follow the
entire sequence in the kernel function pci_reset_secondary_bus()? Thanks
Riana for pointing out.

> > > @@ -417,6 +421,7 @@ igt_main
> > >			set_device_filter(dev_path);
> > >
> > >			igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
> > > +		dev.root = igt_device_get_pci_root_port(dev.fds.dev);

Also is SBR done on the root port or the upstream bridge, in case there is
a bridge in the middle (between the root port and the device/bus)? In which
case this should be pci_device_get_parent_bridge()?

Thanks.
--
Ashutosh