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

[Gupta, Anshuman]

> -----Original Message-----
> From: Tauro, Riana <riana.tauro at intel.com>
> Sent: Tuesday, January 10, 2023 6:36 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; igt-
> dev at lists.freedesktop.org; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Dixit, Ashutosh
> <ashutosh.dixit at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>
> Subject: Re: [PATCH i-g-t 2/2] tests/device_reset: Add warm_reset test
> 
> 
> Hi Anshuman,
> 
> On 1/9/2023 5:32 PM, Anshuman Gupta wrote:
> > Adding gfx card root port warm reset support by triggering secondary
> > bus reset on root port.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> >   lib/igt_pci.c        | 28 ++++++++++++++++++++++++++++
> >   lib/igt_pci.h        |  5 +++++
> >   tests/device_reset.c | 18 +++++++++++++++---
> >   3 files changed, 48 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/igt_pci.c b/lib/igt_pci.c index
> > 61aaf939d1..3940a0ee1e 100644
> > --- a/lib/igt_pci.c
> > +++ b/lib/igt_pci.c
> > @@ -51,3 +51,31 @@ int find_pci_cap_offset(struct pci_device *dev,
> enum pci_cap_id cap_id)
> >   {
> >   	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
> >   }
> > +
> > +/**
> > + * igt_pci_bridge_warm_reset:
> > + * @dev: pci bridge device
> > + *
> > + * return:
> > + * true on success otherwise false.
> > + */
> > +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.
Br,
Anshuman Gupta.
> 
> Thanks
> Riana
> 
> > +	return true;
> > +}
> > diff --git a/lib/igt_pci.h b/lib/igt_pci.h index
> > 92b9cc3929..f755e4baab 100644
> > --- a/lib/igt_pci.h
> > +++ b/lib/igt_pci.h
> > @@ -12,6 +12,10 @@
> >   /* forward declaration */
> >   struct pci_device;
> >
> > +#define PCI_HEADER_TYPE 0xe
> > +#define PCI_TYPE_1_HEADER_BRIDGE_CTL 0x3e > +#define
> > +PCI_TYPE_1_SEC_BUS_RESET (1 << 6)
> > +
> >   #define PCI_TYPE0_1_HEADER_SIZE 0x40
> >   #define PCI_CAPS_START 0x34
> >   #define PCI_CFG_SPACE_SIZE 0x100
> > @@ -24,5 +28,6 @@ enum pci_cap_id {
> >   #define  PCI_SLOT_PWR_CTRL_PRESENT (1 << 1)
> >
> >   int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id
> > cap_id);
> > +bool igt_pci_bridge_warm_reset(struct pci_device *dev);
> >
> >   #endif
> > diff --git a/tests/device_reset.c b/tests/device_reset.c index
> > fe26030783..cbb5bef443 100644
> > --- a/tests/device_reset.c
> > +++ b/tests/device_reset.c
> > @@ -22,8 +22,9 @@ IGT_TEST_DESCRIPTION("Examine behavior of a driver
> on device sysfs reset");
> >   #define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
> >
> >   enum reset {
> > -	COLD_RESET,
> > -	FLR_RESET
> > +	FLR_RESET,
> > +	WARM_RESET,
> > +	COLD_RESET
> >   };
> >
> >   /**
> > @@ -39,6 +40,7 @@ struct device_data {
> >   	} fds;
> >   	char dev_bus_addr[DEV_BUS_ADDR_LEN];
> >   	bool snd_unload;
> > +	struct pci_device *root;
> >   };
> >
> >   static int __open_sysfs_dir(int fd, const char* path) @@ -336,6
> > +338,8 @@ static void initiate_device_reset(struct device_data *dev,
> > enum reset type)
> >
> >   	if (type == FLR_RESET) {
> >   		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> > +	} else if (type == WARM_RESET) {
> > +		igt_assert(igt_pci_bridge_warm_reset(dev->root));
> >   	} else if (type == COLD_RESET) {
> >   		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> >   		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir,
> "power")); @@
> > -404,7 +408,7 @@ static void unbind_reset_rebind(struct device_data
> > *dev, enum reset type)
> >
> >   igt_main
> >   {
> > -	struct device_data dev = { .fds = {-1, -1, -1}, .dev_bus_addr = {0}, };
> > +	struct device_data dev = { .fds = {-1, -1, -1}, .dev_bus_addr = {0},
> > +.root = NULL};
> >
> >   	igt_fixture {
> >   		char dev_path[PATH_MAX];
> > @@ -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);
> >   	}
> >
> >   	igt_describe("Unbinds driver from device, initiates reset"
> > @@ -432,6 +437,13 @@ igt_main
> >   		healthcheck(&dev);
> >   	}
> >
> > +	igt_describe("Unbinds driver from device, initiates warm reset"
> > +		     " then rebinds driver to device");
> > +	igt_subtest("unbind-warm-reset-rebind") {
> > +		unbind_reset_rebind(&dev, WARM_RESET);
> > +		healthcheck(&dev);
> > +	}
> > +
> >   	igt_subtest_group {
> >   		igt_fixture {
> >   			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does
> not support any "