From: Dave Jiang <dave.jiang@intel.com>
To: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com>,
linux-cxl@vger.kernel.org, linux-pci@vger.kernel.org
Cc: dan.j.williams@intel.com, ira.weiny@intel.com,
vishal.l.verma@intel.com, alison.schofield@intel.com,
Jonathan.Cameron@huawei.com, dave@stgolabs.net,
bhelgaas@google.com, lukas@wunner.de
Subject: Re: [PATCH v4 2/4] PCI: Add check for CXL Secondary Bus Reset
Date: Tue, 9 Apr 2024 14:56:08 -0700 [thread overview]
Message-ID: <f3394e7d-8094-4821-9fec-1d7b296805bc@intel.com> (raw)
In-Reply-To: <da8ca6a2-860f-44a5-bb47-b5967f40be90@linux.intel.com>
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, ®);
>> + 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 */
>
next prev parent reply other threads:[~2024-04-09 21:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 16:01 [PATCH 0/4 v4] PCI: Add Secondary Bus Reset (SBR) support for CXL Dave Jiang
2024-04-09 16:01 ` [PATCH v4 1/4] PCI/cxl: Move PCI CXL vendor Id to a common location from CXL subsystem Dave Jiang
2024-04-09 21:28 ` Kuppuswamy Sathyanarayanan
2024-04-09 22:51 ` Dan Williams
2024-04-09 16:01 ` [PATCH v4 2/4] PCI: Add check for CXL Secondary Bus Reset Dave Jiang
2024-04-09 21:39 ` Kuppuswamy Sathyanarayanan
2024-04-09 21:56 ` Dave Jiang [this message]
2024-04-11 2:33 ` Kuppuswamy Sathyanarayanan
2024-04-09 22:56 ` Dan Williams
2024-04-09 16:01 ` [PATCH v4 3/4] PCI: Create new reset method to force SBR for CXL Dave Jiang
2024-04-26 19:46 ` Dan Williams
2024-04-27 6:19 ` Lukas Wunner
2024-04-27 17:07 ` Dan Williams
2024-04-09 16:01 ` [PATCH v4 4/4] cxl: Add post reset warning if reset results in loss of previously committed HDM decoders Dave Jiang
2024-04-26 20:03 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f3394e7d-8094-4821-9fec-1d7b296805bc@intel.com \
--to=dave.jiang@intel.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=dave@stgolabs.net \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).