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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 11-01-2023 02:44, Dixit, Ashutosh wrote:
> 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.

Reset de-assert is missing here. Reset need to be de-asserted after 
asserting it.

Regards,
Badal
> 
>>>> @@ -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