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

[Gupta, Anshuman]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: Wednesday, January 11, 2023 2:45 AM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: Tauro, Riana <riana.tauro at intel.com>; igt-dev at lists.freedesktop.org;
> Iddamsetty, Aravind <aravind.iddamsetty at intel.com>; Nilawar, Badal
> <badal.nilawar at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH i-g-t 2/2] tests/device_reset: Add warm_reset test
> 
> 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#L50
> > > 45
> > 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.
I was looking to the kernel source code , it seems by using /sys/bus/pvi/devices/$bdf/reset and /sys/bus/pvi/devices/$bdf/reset_method
sysfs we can trigger bus reset i.e warm reset. So we don't need to reinvent the wheel in IGT. So dropping the patch series.
Br,
Anshuman Gupta. 
> 
> > > > @@ -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