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