Re: [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service

[Yicong Yang <yangyicong@hisilicon.com>]

On 2021/7/27 6:05, Bjorn Helgaas wrote:
> On Wed, Feb 24, 2021 at 05:47:58PM +0800, Yicong Yang wrote:
>> On 2021/2/3 20:53, Yicong Yang wrote:
>>> Per Downstream Port Containment Related Enhancements ECN[1]
>>> Table 4-6, Interpretation of _OSC Control Field Returned Value,
>>> for bit 7 of _OSC control return value:
>>>
>>>   "Firmware sets this bit to 1 to grant the OS control over PCI Express
>>>   Downstream Port Containment configuration."
>>>   "If control of this feature was requested and denied,
>>>   or was not requested, the firmware returns this bit set to 0."
>>>
>>> We store bit 7 of _OSC control return value in host->native_dpc,
>>> check it before enable the dpc service as the firmware may not
>>> grant the control.
>>>
>>> [1] Downstream Port Containment Related Enhancements ECN,
>>>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>>>     https://members.pcisig.com/wg/PCI-SIG/document/12888
>>>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>> ---
>>> Change since v1:
>>> - use correct reference for _OSC control return value
>>>
>>>  drivers/pci/pcie/portdrv_core.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>>> index e1fed664..7445d03 100644
>>> --- a/drivers/pci/pcie/portdrv_core.c
>>> +++ b/drivers/pci/pcie/portdrv_core.c
>>> @@ -253,7 +253,8 @@ static int get_port_device_capability(struct pci_dev *dev)
>>>  	 */
>>>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>>>  	    pci_aer_available() &&
>>> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>>> +	    (pcie_ports_dpc_native ||
>>> +	    ((services & PCIE_PORT_SERVICE_AER) && host->native_dpc)))
>>>  		services |= PCIE_PORT_SERVICE_DPC;
>>>  
>>
>> the check here maybe problematic. the bit 7 of _OSC return value is reserved
>> previously and the change here may break the backward compatibility.
>> currently we make dpc enabled along with aer, which can ensure the native
>> dpc won't be enabled if the edr is enabled.
> 
> Since you think this is problematic, I'll drop it for now.  If you
> decide it's the right thing to do, please post it again.
> 

yes, I still think this has problem. sorry for the noise.

>> i feel a bit confused as the bit 7 is not used.
>> does it provide a way to enable native dpc regardless of aer ownership?
>> just as pcie_ports=dpc-native does when i checked the discussion in [1].
>>
>> [1] https://lore.kernel.org/linux-pci/20191023192205.97024-1-olof@lixom.net/
>>
>> Thanks,
>> Yicong
>>
>>>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>>>
>>
> 
> .
>