From: Dan Williams <dan.j.williams@intel.com>
To: Dave Jiang <dave.jiang@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 3/4] PCI: Create new reset method to force SBR for CXL
Date: Fri, 26 Apr 2024 12:46:45 -0700 [thread overview]
Message-ID: <662c04a5d8f5f_b6e02945f@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <20240409160256.94184-4-dave.jiang@intel.com>
Dave Jiang wrote:
> CXL spec r3.1 8.1.5.2
> By default Secondary Bus Reset (SBR) is masked for CXL ports. Introduce a
> new PCI reset method "cxl_bus" to force SBR on CXL ports by setting
> the unmask SBR bit in the CXL DVSEC port control register before performing
> the bus reset and restore the original value of the bit post reset. The
> new reset method allows the user to intentionally perform SBR on a CXL
> device without needing to set the "Unmask SBR" bit via a user tool.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v4:
> - Use pci_dev_lock guard() on bridge. (Dan)
> ---
> drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/linux/pci.h | 2 +-
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 570b00fe10f7..3b4f7b369287 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4982,6 +4982,44 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
> return pci_parent_bus_reset(dev, probe);
> }
>
> +static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
> +{
> + struct pci_dev *bridge;
> + u16 dvsec, reg, val;
> + int rc;
> +
> + bridge = pci_upstream_bridge(dev);
> + if (!bridge)
> + return -ENOTTY;
> +
> + dvsec = cxl_port_dvsec(bridge);
> + if (!dvsec)
> + return -ENOTTY;
> +
> + if (probe)
> + return 0;
> +
> + guard(pci_dev)(bridge);
My concern here is this sets up a pci_reset_function() locking order of:
pci_dev_lock(@dev)
pci_dev_lock(pci_upstream_bridge(@dev))
Compare that to pci_reset_bus() that does:
pci_dev_lock(pci_upstream_bridge(@dev))
pci_dev_lock(@dev)
This also highlights that the pci_dev_lock() performed by
pci_reset_function() has long been insufficient for the
pci_reset_bus_function() method case that could race userspace when
pci_reset_secondary_bus() is manipulating the bridge control register.
So, if the goal of the lock is to prevent userspace from clobbering the
kernel's read-modify-write cycles of @dev's parent bridge, then the lock
needs to be held over both the cxl_reset_bus_function() and the
pci_reset_bus_function() cases, and it needs to be taken in
upstream-bridge => endpoint order.
next prev parent reply other threads:[~2024-04-26 19:46 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
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 [this message]
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=662c04a5d8f5f_b6e02945f@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=bhelgaas@google.com \
--cc=dave.jiang@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=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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.