diff mbox series

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

Message ID 20240409160256.94184-3-dave.jiang@intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Add Secondary Bus Reset (SBR) support for CXL | expand

Commit Message

Dave Jiang April 9, 2024, 4:01 p.m. UTC 
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>
---
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(+)

Comments

Kuppuswamy Sathyanarayanan April 9, 2024, 9:39 p.m. UTC | #1 
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>
> ---
> 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.

> +
> +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?

> +
> +		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 */
Dave Jiang April 9, 2024, 9:56 p.m. UTC | #2 
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>
>> ---
>> 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.

> 
>> +
>> +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.

> 
>> +
>> +		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 */
>
Dan Williams April 9, 2024, 10:56 p.m. UTC | #3 
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

Feels like a missing "Otherwise," at the beginning of this sentence to
indicate transition from what the patch does to what happens without the
patch.

> 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.

Otherwise, heh heh heh, you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...I would say, do not spin the patch just for that small fixup.
Kuppuswamy Sathyanarayanan April 11, 2024, 2:33 a.m. UTC | #4 
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 */
diff mbox series

Patch

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);
+}
+
+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;
+
+		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 */