Re: [PATCH v4 2/4] PCI: Add check for CXL Secondary Bus Reset

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 4/9/24 2:56 PM, Dave Jiang wrote:
>
> On 4/9/24 2:39 PM, Kuppuswamy Sathyanarayanan wrote:
>> On 4/9/24 9:01 AM, Dave Jiang wrote:
>>> Per CXL spec r3.1 8.1.5.2, Secondary Bus Reset (SBR) is masked unless the
>>> "Unmask SBR" bit is set. Add a check to the PCI secondary bus reset
>>> path to fail the CXL SBR request if the "Unmask SBR" bit is clear in
>>> the CXL Port Control Extensions register by returning -ENOTTY.
>>>
>>> When the "Unmask SBR" bit is set to 0 (default), the bus_reset would
>>> appear to have executed successfully. However the operation is actually
>>> masked. The intention is to inform the user that SBR for the CXL device
>>> is masked and will not go through.
>>>
>>> If the "Unmask SBR" bit is set to 1, then the bus reset will execute
>>> successfully.
>>>
>>> Link: https://lore.kernel.org/linux-cxl/20240220203956.GA1502351@bhelgaas/
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>>> v4:
>>> - cxl_port_dvsec() should return u16. (Lukas)
>>> ---
>>>  drivers/pci/pci.c             | 45 +++++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/pci_regs.h |  5 ++++
>>>  2 files changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index e5f243dd4288..570b00fe10f7 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -4927,10 +4927,55 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>>>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>>>  }
>>>  
>>> +static u16 cxl_port_dvsec(struct pci_dev *dev)
>>> +{
>>> +	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
>>> +					 PCI_DVSEC_CXL_PORT);
>>> +}
>> Since cxl_sbr_masked() is the only user of this function, why not directly
>> check for this capability there.
> It's used by another function in the 3rd patch. I previously had it open coded. But Dan said to reduce churn, just create the function to begin with instead of moving that code to a function later on.

May be that patch would be the right place to introduce a helper function. But I think it fine either way.

>>> +
>>> +static bool cxl_sbr_masked(struct pci_dev *dev)
>>> +{
>>> +	u16 dvsec, reg;
>>> +	int rc;
>>> +
>>> +	/*
>>> +	 * No DVSEC found, either is not a CXL port, or not connected in which
>>> +	 * case mask state is a nop (CXL r3.1 sec 9.12.3 "Enumerating CXL RPs
>>> +	 * and DSPs"
>>> +	 */
>>> +	dvsec = cxl_port_dvsec(dev);
>>> +	if (!dvsec)
>>> +		return false;
>>> +
>>> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_PORT_CTL, &reg);
>>> +	if (rc || PCI_POSSIBLE_ERROR(reg))
>>> +		return false;
>>> +
>>> +	/*
>>> +	 * CXL spec r3.1 8.1.5.2
>>> +	 * When 0, SBR bit in Bridge Control register of this Port has no effect.
>>> +	 * When 1, the Port shall generate hot reset when SBR bit in Bridge
>>> +	 * Control gets set to 1.
>>> +	 */
>>> +	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>>> +
>>>  static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>>>  {
>>> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
>>>  	int rc;
>>>  
>>> +	/* If it's a CXL port and the SBR control is masked, fail the SBR */
>>> +	if (bridge && cxl_sbr_masked(bridge)) {
>>> +		if (probe)
>>> +			return 0;
>> Why return success during the probe?
> Otherwise the reset_method will disappear as available after initial probe. We want to leave the reset method available. If the register bit gets unmasked we can perform a bus reset. We don't want to take it away the option entirely if it's masked.

Ok.

>>> +
>>> +		return -ENOTTY;
>>> +	}
>>> +
>>>  	rc = pci_dev_reset_slot_function(dev, probe);
>>>  	if (rc != -ENOTTY)
>>>  		return rc;
>>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>>> index a39193213ff2..d61fa43662e3 100644
>>> --- a/include/uapi/linux/pci_regs.h
>>> +++ b/include/uapi/linux/pci_regs.h
>>> @@ -1148,4 +1148,9 @@
>>>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL		0x00ff0000
>>>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
>>>  
>>> +/* Compute Express Link (CXL) */
>>> +#define PCI_DVSEC_CXL_PORT				3
>>> +#define PCI_DVSEC_CXL_PORT_CTL				0x0c
>>> +#define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
>>> +
>>>  #endif /* LINUX_PCI_REGS_H */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer