linux-kernel - RE: Subject : [ PATCH ] pci-reset-error_state-to-pci_channel_io_normal-at-report_slot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 21 May 2013 15:41:44 +0000
From:	"Liu, Joseph" <Joseph.Liu@...lex.Com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Zhang, LongX" <longx.zhang@...el.com>
CC:	"linasvepstas@...il.com" <linasvepstas@...il.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>
Subject: RE: Subject : [ PATCH ]
 pci-reset-error_state-to-pci_channel_io_normal-at-report_slot_reset

Bjorn,

>> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
>> index ed4d094..7abefd9 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -332,13 +332,11 @@ static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>>  	pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
>>  	int retval;
>>  
>> -	/* If fatal, restore cfg space for possible link reset at upstream */
>> -	if (dev->error_state == pci_channel_io_frozen) {
>> -		dev->state_saved = true;
>> -		pci_restore_state(dev);
>> -		pcie_portdrv_restore_config(dev);
>> -		pci_enable_pcie_error_reporting(dev);
>> -	}
>> +	/* restore cfg space for possible link reset at upstream */
>> +	dev->state_saved = true;
>> +	pci_restore_state(dev);
>> +	pcie_portdrv_restore_config(dev);
>> +	pci_enable_pcie_error_reporting(dev);
>>  
>>  	/* get true return value from &status */
>>  	retval = device_for_each_child(&dev->dev, &status, slot_reset_iter);
>
>I think this patch changes the behavior in the case of a non-fatal error
>where one of the .error_detected() methods returned
>PCI_ERS_RESULT_NEED_RESET.  In that case, pcie_portdrv_slot_reset()
>previously did not restore config space, but after your patch, it *will*
>restore it.  We need an explanation of why this is safe. 

Here is my understanding of this part of the patch:

I think your concern here should not be an issue. Whether it is a non-fatal error or a fatal error, as long as one of the .error_detected() method from the downstream drivers involved returns a PCI_ERS_RESULT_NEED_RESET, it should trump all others and a slot reset should be performed, even though it was originally due to a non-fatal error reported or only one of the downstream drivers requests it. In the case of AER driver, this should be implemented in the broadcast_error_message() with pci_walk_bus() by passing in the report_error_detected() function, and merging the results into the result_data->result...

In any case, the fact that this pcie_portdrv_slot_reset() being invoked should be considered as a aftermath of a slot reset has been performed, thus the restore of config space should be performed after the reset. I suppose the restore should be to the same state as fresh power-on states, right?

Thanks,
Joe Liu, Ph.D.
Senior Principal Engineer
Emulex Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/