From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, Robert Richter <rrichter@amd.com>,
<alison.schofield@intel.com>, <terry.bowman@amd.com>,
<bhelgaas@google.com>, <dave.jiang@intel.com>,
<nvdimm@lists.linux.dev>
Subject: Re: [PATCH v6 04/12] cxl/pmem: Remove the cxl_pmem_wq and related infrastructure
Date: Fri, 2 Dec 2022 15:44:57 +0000 [thread overview]
Message-ID: <20221202154457.00004a5b@Huawei.com> (raw)
In-Reply-To: <166993042335.1882361.17022872468068436287.stgit@dwillia2-xfh.jf.intel.com>
On Thu, 01 Dec 2022 13:33:43 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> Now that cxl_nvdimm and cxl_pmem_region objects are torn down
> sychronously with the removal of either the bridge, or an endpoint, the
> cxl_pmem_wq infrastructure can be jettisoned.
>
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense given prior patches.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/cxl/core/pmem.c | 22 -------
> drivers/cxl/cxl.h | 17 ------
> drivers/cxl/pmem.c | 143 -----------------------------------------------
> 3 files changed, 1 insertion(+), 181 deletions(-)
>
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index 4d36805079ad..16446473d814 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -99,7 +99,6 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
>
> dev = &cxl_nvb->dev;
> cxl_nvb->port = port;
> - cxl_nvb->state = CXL_NVB_NEW;
> device_initialize(dev);
> lockdep_set_class(&dev->mutex, &cxl_nvdimm_bridge_key);
> device_set_pm_not_required(dev);
> @@ -117,28 +116,7 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
> static void unregister_nvb(void *_cxl_nvb)
> {
> struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
> - bool flush;
>
> - /*
> - * If the bridge was ever activated then there might be in-flight state
> - * work to flush. Once the state has been changed to 'dead' then no new
> - * work can be queued by user-triggered bind.
> - */
> - device_lock(&cxl_nvb->dev);
> - flush = cxl_nvb->state != CXL_NVB_NEW;
> - cxl_nvb->state = CXL_NVB_DEAD;
> - device_unlock(&cxl_nvb->dev);
> -
> - /*
> - * Even though the device core will trigger device_release_driver()
> - * before the unregister, it does not know about the fact that
> - * cxl_nvdimm_bridge_driver defers ->remove() work. So, do the driver
> - * release not and flush it before tearing down the nvdimm device
> - * hierarchy.
> - */
> - device_release_driver(&cxl_nvb->dev);
> - if (flush)
> - flush_work(&cxl_nvb->state_work);
> device_unregister(&cxl_nvb->dev);
> }
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fc6083b0e467..f0ca2d768385 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -400,34 +400,17 @@ struct cxl_region {
> struct cxl_region_params params;
> };
>
> -/**
> - * enum cxl_nvdimm_brige_state - state machine for managing bus rescans
> - * @CXL_NVB_NEW: Set at bridge create and after cxl_pmem_wq is destroyed
> - * @CXL_NVB_DEAD: Set at brige unregistration to preclude async probing
> - * @CXL_NVB_ONLINE: Target state after successful ->probe()
> - * @CXL_NVB_OFFLINE: Target state after ->remove() or failed ->probe()
> - */
> -enum cxl_nvdimm_brige_state {
> - CXL_NVB_NEW,
> - CXL_NVB_DEAD,
> - CXL_NVB_ONLINE,
> - CXL_NVB_OFFLINE,
> -};
> -
> struct cxl_nvdimm_bridge {
> int id;
> struct device dev;
> struct cxl_port *port;
> struct nvdimm_bus *nvdimm_bus;
> struct nvdimm_bus_descriptor nd_desc;
> - struct work_struct state_work;
> - enum cxl_nvdimm_brige_state state;
> };
>
> struct cxl_nvdimm {
> struct device dev;
> struct cxl_memdev *cxlmd;
> - struct cxl_nvdimm_bridge *bridge;
> };
>
> struct cxl_pmem_region_mapping {
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 76cf54eeb310..0910367a3ead 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -11,13 +11,6 @@
> #include "cxlmem.h"
> #include "cxl.h"
>
> -/*
> - * Ordered workqueue for cxl nvdimm device arrival and departure
> - * to coordinate bus rescans when a bridge arrives and trigger remove
> - * operations when the bridge is removed.
> - */
> -static struct workqueue_struct *cxl_pmem_wq;
> -
> static __read_mostly DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
>
> static void clear_exclusive(void *cxlds)
> @@ -191,105 +184,6 @@ static void unregister_nvdimm_bus(void *_cxl_nvb)
> nvdimm_bus_unregister(nvdimm_bus);
> }
>
> -static bool online_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb)
> -{
> - if (cxl_nvb->nvdimm_bus)
> - return true;
> - cxl_nvb->nvdimm_bus =
> - nvdimm_bus_register(&cxl_nvb->dev, &cxl_nvb->nd_desc);
> - return cxl_nvb->nvdimm_bus != NULL;
> -}
> -
> -static int cxl_nvdimm_release_driver(struct device *dev, void *cxl_nvb)
> -{
> - struct cxl_nvdimm *cxl_nvd;
> -
> - if (!is_cxl_nvdimm(dev))
> - return 0;
> -
> - cxl_nvd = to_cxl_nvdimm(dev);
> - if (cxl_nvd->bridge != cxl_nvb)
> - return 0;
> -
> - device_release_driver(dev);
> - return 0;
> -}
> -
> -static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
> - struct nvdimm_bus *nvdimm_bus)
> -{
> - if (!nvdimm_bus)
> - return;
> -
> - /*
> - * Set the state of cxl_nvdimm devices to unbound / idle before
> - * nvdimm_bus_unregister() rips the nvdimm objects out from
> - * underneath them.
> - */
> - bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
> - cxl_nvdimm_release_driver);
> - nvdimm_bus_unregister(nvdimm_bus);
> -}
> -
> -static void cxl_nvb_update_state(struct work_struct *work)
> -{
> - struct cxl_nvdimm_bridge *cxl_nvb =
> - container_of(work, typeof(*cxl_nvb), state_work);
> - struct nvdimm_bus *victim_bus = NULL;
> - bool release = false, rescan = false;
> -
> - device_lock(&cxl_nvb->dev);
> - switch (cxl_nvb->state) {
> - case CXL_NVB_ONLINE:
> - if (!online_nvdimm_bus(cxl_nvb)) {
> - dev_err(&cxl_nvb->dev,
> - "failed to establish nvdimm bus\n");
> - release = true;
> - } else
> - rescan = true;
> - break;
> - case CXL_NVB_OFFLINE:
> - case CXL_NVB_DEAD:
> - victim_bus = cxl_nvb->nvdimm_bus;
> - cxl_nvb->nvdimm_bus = NULL;
> - break;
> - default:
> - break;
> - }
> - device_unlock(&cxl_nvb->dev);
> -
> - if (release)
> - device_release_driver(&cxl_nvb->dev);
> - if (rescan) {
> - int rc = bus_rescan_devices(&cxl_bus_type);
> -
> - dev_dbg(&cxl_nvb->dev, "rescan: %d\n", rc);
> - }
> - offline_nvdimm_bus(cxl_nvb, victim_bus);
> -
> - put_device(&cxl_nvb->dev);
> -}
> -
> -static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
> -{
> - /*
> - * Take a reference that the workqueue will drop if new work
> - * gets queued.
> - */
> - get_device(&cxl_nvb->dev);
> - if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> - put_device(&cxl_nvb->dev);
> -}
> -
> -static void cxl_nvdimm_bridge_remove(struct device *dev)
> -{
> - struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
> -
> - if (cxl_nvb->state == CXL_NVB_ONLINE)
> - cxl_nvb->state = CXL_NVB_OFFLINE;
> - cxl_nvdimm_bridge_state_work(cxl_nvb);
> -}
> -
> static int cxl_nvdimm_bridge_probe(struct device *dev)
> {
> struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
> @@ -306,15 +200,12 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
> if (!cxl_nvb->nvdimm_bus)
> return -ENOMEM;
>
> - INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
> -
> return devm_add_action_or_reset(dev, unregister_nvdimm_bus, cxl_nvb);
> }
>
> static struct cxl_driver cxl_nvdimm_bridge_driver = {
> .name = "cxl_nvdimm_bridge",
> .probe = cxl_nvdimm_bridge_probe,
> - .remove = cxl_nvdimm_bridge_remove,
> .id = CXL_DEVICE_NVDIMM_BRIDGE,
> .drv = {
> .suppress_bind_attrs = true,
> @@ -453,31 +344,6 @@ static struct cxl_driver cxl_pmem_region_driver = {
> },
> };
>
> -/*
> - * Return all bridges to the CXL_NVB_NEW state to invalidate any
> - * ->state_work referring to the now destroyed cxl_pmem_wq.
> - */
> -static int cxl_nvdimm_bridge_reset(struct device *dev, void *data)
> -{
> - struct cxl_nvdimm_bridge *cxl_nvb;
> -
> - if (!is_cxl_nvdimm_bridge(dev))
> - return 0;
> -
> - cxl_nvb = to_cxl_nvdimm_bridge(dev);
> - device_lock(dev);
> - cxl_nvb->state = CXL_NVB_NEW;
> - device_unlock(dev);
> -
> - return 0;
> -}
> -
> -static void destroy_cxl_pmem_wq(void)
> -{
> - destroy_workqueue(cxl_pmem_wq);
> - bus_for_each_dev(&cxl_bus_type, NULL, NULL, cxl_nvdimm_bridge_reset);
> -}
> -
> static __init int cxl_pmem_init(void)
> {
> int rc;
> @@ -485,13 +351,9 @@ static __init int cxl_pmem_init(void)
> set_bit(CXL_MEM_COMMAND_ID_SET_SHUTDOWN_STATE, exclusive_cmds);
> set_bit(CXL_MEM_COMMAND_ID_SET_LSA, exclusive_cmds);
>
> - cxl_pmem_wq = alloc_ordered_workqueue("cxl_pmem", 0);
> - if (!cxl_pmem_wq)
> - return -ENXIO;
> -
> rc = cxl_driver_register(&cxl_nvdimm_bridge_driver);
> if (rc)
> - goto err_bridge;
> + return rc;
>
> rc = cxl_driver_register(&cxl_nvdimm_driver);
> if (rc)
> @@ -507,8 +369,6 @@ static __init int cxl_pmem_init(void)
> cxl_driver_unregister(&cxl_nvdimm_driver);
> err_nvdimm:
> cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
> -err_bridge:
> - destroy_cxl_pmem_wq();
> return rc;
> }
>
> @@ -517,7 +377,6 @@ static __exit void cxl_pmem_exit(void)
> cxl_driver_unregister(&cxl_pmem_region_driver);
> cxl_driver_unregister(&cxl_nvdimm_driver);
> cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
> - destroy_cxl_pmem_wq();
> }
>
> MODULE_LICENSE("GPL v2");
>
next prev parent reply other threads:[~2022-12-02 15:45 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-01 21:33 [PATCH v6 00/12] cxl: Add support for Restricted CXL hosts (RCD mode) Dan Williams
2022-12-01 21:33 ` [PATCH v6 01/12] cxl/acpi: Simplify cxl_nvdimm_bridge probing Dan Williams
2022-12-02 15:02 ` Jonathan Cameron
2022-12-01 21:33 ` [PATCH v6 02/12] cxl/region: Drop redundant pmem region release handling Dan Williams
2022-12-02 15:43 ` Jonathan Cameron
2022-12-01 21:33 ` [PATCH v6 03/12] cxl/pmem: Refactor nvdimm device registration, delete the workqueue Dan Williams
2022-12-02 15:42 ` Jonathan Cameron
2022-12-01 21:33 ` [PATCH v6 04/12] cxl/pmem: Remove the cxl_pmem_wq and related infrastructure Dan Williams
2022-12-02 15:44 ` Jonathan Cameron [this message]
2022-12-01 21:33 ` [PATCH v6 05/12] cxl/acpi: Move rescan to the workqueue Dan Williams
2022-12-02 15:50 ` Jonathan Cameron
2022-12-03 7:14 ` Dan Williams
2022-12-01 21:33 ` [PATCH v6 06/12] tools/testing/cxl: Make mock CEDT parsing more robust Dan Williams
2022-12-01 21:57 ` Dave Jiang
2022-12-02 15:58 ` Jonathan Cameron
2022-12-03 7:22 ` Dan Williams
2022-12-01 21:33 ` [PATCH v6 07/12] cxl/ACPI: Register CXL host ports by bridge device Dan Williams
2022-12-01 22:00 ` Dave Jiang
2022-12-02 16:11 ` Jonathan Cameron
2022-12-03 7:28 ` Dan Williams
2022-12-01 21:34 ` [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB Dan Williams
2022-12-01 23:55 ` Dave Jiang
2022-12-02 8:16 ` Robert Richter
2022-12-03 7:04 ` Dan Williams
2022-12-03 8:41 ` Dan Williams
2022-12-03 16:03 ` Robert Richter
2022-12-03 17:06 ` Dan Williams
2022-12-02 16:38 ` Jonathan Cameron
2022-12-03 7:39 ` Dan Williams
2022-12-01 21:34 ` [PATCH v6 09/12] cxl/mem: Move devm_cxl_add_endpoint() from cxl_core to cxl_mem Dan Williams
2022-12-02 16:40 ` Jonathan Cameron
2022-12-01 21:34 ` [PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration Dan Williams
2022-12-02 8:21 ` Robert Richter
2022-12-03 7:05 ` Dan Williams
2022-12-02 16:45 ` Jonathan Cameron
2022-12-01 21:34 ` [PATCH v6 11/12] tools/testing/cxl: Add an RCH topology Dan Williams
2022-12-02 8:05 ` Robert Richter
2022-12-02 17:04 ` Jonathan Cameron
2022-12-03 7:50 ` Dan Williams
2022-12-01 21:34 ` [PATCH v6 12/12] cxl/acpi: Set ACPI's CXL _OSC to indicate RCD mode support Dan Williams
2022-12-02 17:05 ` Jonathan Cameron
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=20221202154457.00004a5b@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--cc=rrichter@amd.com \
--cc=terry.bowman@amd.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