[PATCH v3 00/10] cxl/mem: Fix shutdown order

[PATCH v3 00/10] cxl/mem: Fix shutdown order

[Dan Williams]

Changes since v2 [1]:
- Fix @dev vs @host confusion in cxl_sanitize_setup_notifier()
  (Davidlohr)
- Fix hardirq vs threadirq context for taking mbox lock in the handler
  (Davidlohr)
- Switch a test to boolean notation (Jonathan)
- Clarify why cxl_sanitize_setup_notifier() takes @cxlmd, instead of
  @mds (Jonathan)
- Drop export of cxl_mem_sanitize()
- Make it more obvious where some setup functions are taking an @host
  parameter vs an device object / context to operate on parameter.
- Fix synchronization between sanitize and decoder commit
- Add cxl_test infrastructure to regression test these ABI paths

[1]: http://lore.kernel.org/r/169602896768.904193.11292185494339980455.stgit@dwillia2-xfh.jf.intel.com

---

While fixing a crash where the cxlmd->cxlds validity needed to be
maintained over the span of the memdev being unregistered, I made a note
to come back and cleanup sanitize notifier setup/shutdown
implementation. Jonathan went further and noticed that the fix needs
that rework first [2].

[2]: http://lore.kernel.org/r/20230929100316.00004546@Huawei.com

The special wrinkle of the sanitize notifier is that it interacts with
interrupts, which are enabled early in the flow, and it interacts with
memdev sysfs which is not initialized until late in the flow.

After some other cleanups, a self contained
cxl_sanitize_setup_notifier() is introduced to centralize that
incremental setup work, and leave cxl_memdev_shutdown() alone to
coordinate closing down the ioctl path relative to the unregister event
of the memdev.

As I went to checkout the notifier in cxl_test I realized that it
insta-crashes since it tries to read registers from a sysfs attribute.
It turns out that could be fixed as a side effect of fixing the race
between issuing the sanitize command and committing decoders.

Given the new locking and the fallout from not having regression
coverage here I went ahead and extended cxl_test to exercise these ABI
paths. Watch for the related cxl-cli set next.

---

Dan Williams (10):
      cxl/pci: Remove unnecessary device reference management in sanitize work
      cxl/pci: Cleanup 'sanitize' to always poll
      cxl/pci: Remove hardirq handler for cxl_request_irq()
      cxl/pci: Remove inconsistent usage of dev_err_probe()
      cxl/pci: Clarify devm host for memdev relative setup
      cxl/pci: Fix sanitize notifier setup
      cxl/memdev: Fix sanitize vs decoder setup locking
      cxl/mem: Fix shutdown order
      tools/testing/cxl: Make cxl_memdev_state available to other command emulation
      tools/testing/cxl: Add 'sanitize notifier' support


 drivers/cxl/core/core.h      |    1 
 drivers/cxl/core/hdm.c       |   19 +++++
 drivers/cxl/core/mbox.c      |   55 +++++++++++----
 drivers/cxl/core/memdev.c    |  157 ++++++++++++++++++------------------------
 drivers/cxl/core/port.c      |    6 ++
 drivers/cxl/core/region.c    |    6 --
 drivers/cxl/cxlmem.h         |   13 ++-
 drivers/cxl/pci.c            |   88 +++++++++++-------------
 tools/testing/cxl/test/mem.c |   78 +++++++++++++++++++--
 9 files changed, 256 insertions(+), 167 deletions(-)

base-commit: 6465e260f48790807eef06b583b38ca9789b6072

[PATCH v3 01/10] cxl/pci: Remove unnecessary device reference management in sanitize work

[Dan Williams]

Given that any particular put_device() could be the final put of the
device, the fact that there are usages of cxlds->dev after
put_device(cxlds->dev) is a red flag. Drop the reference counting since
the device is pinned by being registered and will not be unregistered
without triggering the driver + workqueue to shutdown.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 44a21ab7add5..aa1b3dd9e64c 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -152,8 +152,6 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
 	mutex_lock(&mds->mbox_mutex);
 	if (cxl_mbox_background_complete(cxlds)) {
 		mds->security.poll_tmo_secs = 0;
-		put_device(cxlds->dev);
-
 		if (mds->security.sanitize_node)
 			sysfs_notify_dirent(mds->security.sanitize_node);
 
@@ -296,9 +294,6 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
 		 */
 		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
 			if (mds->security.poll) {
-				/* hold the device throughout */
-				get_device(cxlds->dev);
-
 				/* give first timeout a second */
 				timeout = 1;
 				mds->security.poll_tmo_secs = timeout;

[PATCH v3 02/10] cxl/pci: Cleanup 'sanitize' to always poll

[Dan Williams]

In preparation for fixing the init/teardown of the 'sanitize' workqueue
and sysfs notification mechanism, arrange for cxl_mbox_sanitize_work()
to be the single location where the sysfs attribute is notified. With
that change there is no distinction between polled mode and interrupt
mode. All the interrupt does is accelerate the polling interval.

The change to check for "mds->security.sanitize_node" under the lock is
there to ensure that the interrupt, the work routine and the
setup/teardown code can all have a consistent view of the registered
notifier and the workqueue state. I.e. the expectation is that the
interrupt is live past the point that the sanitize sysfs attribute is
published, and it may race teardown, so it must be consulted under a
lock. Given that new locking requirement, cxl_pci_mbox_irq() is moved
from hard to thread irq context.

Lastly, some opportunistic replacements of
"queue_delayed_work(system_wq, ...)", which is just open coded
schedule_delayed_work(), are included.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |    3 +-
 drivers/cxl/cxlmem.h      |    2 --
 drivers/cxl/pci.c         |   60 +++++++++++++++++++--------------------------
 3 files changed, 26 insertions(+), 39 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 14b547c07f54..2a7a07f6d165 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -561,8 +561,7 @@ static void cxl_memdev_security_shutdown(struct device *dev)
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 
-	if (mds->security.poll)
-		cancel_delayed_work_sync(&mds->security.poll_dwork);
+	cancel_delayed_work_sync(&mds->security.poll_dwork);
 }
 
 static void cxl_memdev_shutdown(struct device *dev)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 706f8a6d1ef4..55f00ad17a77 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -360,7 +360,6 @@ struct cxl_fw_state {
  *
  * @state: state of last security operation
  * @enabled_cmds: All security commands enabled in the CEL
- * @poll: polling for sanitization is enabled, device has no mbox irq support
  * @poll_tmo_secs: polling timeout
  * @poll_dwork: polling work item
  * @sanitize_node: sanitation sysfs file to notify
@@ -368,7 +367,6 @@ struct cxl_fw_state {
 struct cxl_security_state {
 	unsigned long state;
 	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
-	bool poll;
 	int poll_tmo_secs;
 	struct delayed_work poll_dwork;
 	struct kernfs_node *sanitize_node;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index aa1b3dd9e64c..49d9b2ef5c5c 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -128,10 +128,10 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
 	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
 	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
 	if (opcode == CXL_MBOX_OP_SANITIZE) {
+		mutex_lock(&mds->mbox_mutex);
 		if (mds->security.sanitize_node)
-			sysfs_notify_dirent(mds->security.sanitize_node);
-
-		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
+			mod_delayed_work(system_wq, &mds->security.poll_dwork, 0);
+		mutex_unlock(&mds->mbox_mutex);
 	} else {
 		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
 		rcuwait_wake_up(&mds->mbox_wait);
@@ -160,8 +160,7 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
 		int timeout = mds->security.poll_tmo_secs + 10;
 
 		mds->security.poll_tmo_secs = min(15 * 60, timeout);
-		queue_delayed_work(system_wq, &mds->security.poll_dwork,
-				   timeout * HZ);
+		schedule_delayed_work(&mds->security.poll_dwork, timeout * HZ);
 	}
 	mutex_unlock(&mds->mbox_mutex);
 }
@@ -293,15 +292,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
 		 * and allow userspace to poll(2) for completion.
 		 */
 		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
-			if (mds->security.poll) {
-				/* give first timeout a second */
-				timeout = 1;
-				mds->security.poll_tmo_secs = timeout;
-				queue_delayed_work(system_wq,
-						   &mds->security.poll_dwork,
-						   timeout * HZ);
-			}
-
+			/* give first timeout a second */
+			timeout = 1;
+			mds->security.poll_tmo_secs = timeout;
+			schedule_delayed_work(&mds->security.poll_dwork,
+					      timeout * HZ);
 			dev_dbg(dev, "Sanitization operation started\n");
 			goto success;
 		}
@@ -384,7 +379,9 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
 	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
 	struct device *dev = cxlds->dev;
 	unsigned long timeout;
+	int irq, msgnum;
 	u64 md_status;
+	u32 ctrl;
 
 	timeout = jiffies + mbox_ready_timeout * HZ;
 	do {
@@ -432,33 +429,26 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
 	dev_dbg(dev, "Mailbox payload sized %zu", mds->payload_size);
 
 	rcuwait_init(&mds->mbox_wait);
+	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mbox_sanitize_work);
 
-	if (cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) {
-		u32 ctrl;
-		int irq, msgnum;
-		struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-
-		msgnum = FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);
-		irq = pci_irq_vector(pdev, msgnum);
-		if (irq < 0)
-			goto mbox_poll;
-
-		if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq, NULL))
-			goto mbox_poll;
+	/* background command interrupts are optional */
+	if (!(cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ))
+		return 0;
 
-		/* enable background command mbox irq support */
-		ctrl = readl(cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
-		ctrl |= CXLDEV_MBOX_CTRL_BG_CMD_IRQ;
-		writel(ctrl, cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
+	msgnum = FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);
+	irq = pci_irq_vector(to_pci_dev(cxlds->dev), msgnum);
+	if (irq < 0)
+		return 0;
 
+	if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
 		return 0;
-	}
 
-mbox_poll:
-	mds->security.poll = true;
-	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mbox_sanitize_work);
+	dev_dbg(cxlds->dev, "Mailbox interrupts enabled\n");
+	/* enable background command mbox irq support */
+	ctrl = readl(cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
+	ctrl |= CXLDEV_MBOX_CTRL_BG_CMD_IRQ;
+	writel(ctrl, cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
 
-	dev_dbg(cxlds->dev, "Mailbox interrupts are unsupported");
 	return 0;
 }
 

Re: [PATCH v3 02/10] cxl/pci: Cleanup 'sanitize' to always poll

[Davidlohr Bueso]

On Fri, 06 Oct 2023, Dan Williams wrote:

>In preparation for fixing the init/teardown of the 'sanitize' workqueue
>and sysfs notification mechanism, arrange for cxl_mbox_sanitize_work()
>to be the single location where the sysfs attribute is notified. With
>that change there is no distinction between polled mode and interrupt
>mode. All the interrupt does is accelerate the polling interval.
>
>The change to check for "mds->security.sanitize_node" under the lock is
>there to ensure that the interrupt, the work routine and the
>setup/teardown code can all have a consistent view of the registered
>notifier and the workqueue state. I.e. the expectation is that the
>interrupt is live past the point that the sanitize sysfs attribute is
>published, and it may race teardown, so it must be consulted under a
>lock. Given that new locking requirement, cxl_pci_mbox_irq() is moved
>from hard to thread irq context.
>
>Lastly, some opportunistic replacements of
>"queue_delayed_work(system_wq, ...)", which is just open coded
>schedule_delayed_work(), are included.

So the current order of this patch will cause bisection issues -
the next patch which moves the irq handler to a threaded context
should come before this one. Otherwise:

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 02/10] cxl/pci: Cleanup 'sanitize' to always poll

[Dan Williams]

Davidlohr Bueso wrote:
> On Fri, 06 Oct 2023, Dan Williams wrote:
> 
> >In preparation for fixing the init/teardown of the 'sanitize' workqueue
> >and sysfs notification mechanism, arrange for cxl_mbox_sanitize_work()
> >to be the single location where the sysfs attribute is notified. With
> >that change there is no distinction between polled mode and interrupt
> >mode. All the interrupt does is accelerate the polling interval.
> >
> >The change to check for "mds->security.sanitize_node" under the lock is
> >there to ensure that the interrupt, the work routine and the
> >setup/teardown code can all have a consistent view of the registered
> >notifier and the workqueue state. I.e. the expectation is that the
> >interrupt is live past the point that the sanitize sysfs attribute is
> >published, and it may race teardown, so it must be consulted under a
> >lock. Given that new locking requirement, cxl_pci_mbox_irq() is moved
> >from hard to thread irq context.
> >
> >Lastly, some opportunistic replacements of
> >"queue_delayed_work(system_wq, ...)", which is just open coded
> >schedule_delayed_work(), are included.
> 
> So the current order of this patch will cause bisection issues -
> the next patch which moves the irq handler to a threaded context
> should come before this one. Otherwise:


This patch does switch the order:

@@ -432,33 +429,26 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
        dev_dbg(dev, "Mailbox payload sized %zu", mds->payload_size);
 
        rcuwait_init(&mds->mbox_wait);
[..]
-               if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq, NULL))
-                       goto mbox_poll;
[..]
+       if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
                return 0;

...the next patch only deletes the option to pass non-null in the third
argument.

@@ -440,7 +440,7 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
        if (irq < 0)
                return 0;
 
-       if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
+       if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq))
                return 0;
 
        dev_dbg(cxlds->dev, "Mailbox interrupts enabled\n");
@@ -638,7 +638,7 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
        if (irq < 0)
                return irq;
 
-       return cxl_request_irq(cxlds, irq, NULL, cxl_event_thread);
+       return cxl_request_irq(cxlds, irq, cxl_event_thread);
 }
 
 static int cxl_event_get_int_policy(struct cxl_memdev_state *mds,


...did I miss anything?

Re: [PATCH v3 02/10] cxl/pci: Cleanup 'sanitize' to always poll

[Davidlohr Bueso]

On Mon, 09 Oct 2023, Dan Williams wrote:

>Davidlohr Bueso wrote:
>> On Fri, 06 Oct 2023, Dan Williams wrote:
>>
>> >In preparation for fixing the init/teardown of the 'sanitize' workqueue
>> >and sysfs notification mechanism, arrange for cxl_mbox_sanitize_work()
>> >to be the single location where the sysfs attribute is notified. With
>> >that change there is no distinction between polled mode and interrupt
>> >mode. All the interrupt does is accelerate the polling interval.
>> >
>> >The change to check for "mds->security.sanitize_node" under the lock is
>> >there to ensure that the interrupt, the work routine and the
>> >setup/teardown code can all have a consistent view of the registered
>> >notifier and the workqueue state. I.e. the expectation is that the
>> >interrupt is live past the point that the sanitize sysfs attribute is
>> >published, and it may race teardown, so it must be consulted under a
>> >lock. Given that new locking requirement, cxl_pci_mbox_irq() is moved
>> >from hard to thread irq context.
>> >
>> >Lastly, some opportunistic replacements of
>> >"queue_delayed_work(system_wq, ...)", which is just open coded
>> >schedule_delayed_work(), are included.
>>
>> So the current order of this patch will cause bisection issues -
>> the next patch which moves the irq handler to a threaded context
>> should come before this one. Otherwise:
>
>
>This patch does switch the order:

Yes, you are right.

>...did I miss anything?

Nope.

Thanks,
Davidlohr

[PATCH v3 03/10] cxl/pci: Remove hardirq handler for cxl_request_irq()

[Dan Williams]

Now that all callers of cxl_request_irq() are using threaded irqs, drop
the hardirq handler option.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 49d9b2ef5c5c..dc665b12be8f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -90,7 +90,7 @@ struct cxl_dev_id {
 };
 
 static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
-			   irq_handler_t handler, irq_handler_t thread_fn)
+			   irq_handler_t thread_fn)
 {
 	struct device *dev = cxlds->dev;
 	struct cxl_dev_id *dev_id;
@@ -101,9 +101,9 @@ static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
 		return -ENOMEM;
 	dev_id->cxlds = cxlds;
 
-	return devm_request_threaded_irq(dev, irq, handler, thread_fn,
-					 IRQF_SHARED | IRQF_ONESHOT,
-					 NULL, dev_id);
+	return devm_request_threaded_irq(dev, irq, NULL, thread_fn,
+					 IRQF_SHARED | IRQF_ONESHOT, NULL,
+					 dev_id);
 }
 
 static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
@@ -440,7 +440,7 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
 	if (irq < 0)
 		return 0;
 
-	if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
+	if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq))
 		return 0;
 
 	dev_dbg(cxlds->dev, "Mailbox interrupts enabled\n");
@@ -638,7 +638,7 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
 	if (irq < 0)
 		return irq;
 
-	return cxl_request_irq(cxlds, irq, NULL, cxl_event_thread);
+	return cxl_request_irq(cxlds, irq, cxl_event_thread);
 }
 
 static int cxl_event_get_int_policy(struct cxl_memdev_state *mds,

Re: [PATCH v3 03/10] cxl/pci: Remove hardirq handler for cxl_request_irq()

[Davidlohr Bueso]

On Fri, 06 Oct 2023, Dan Williams wrote:

>Now that all callers of cxl_request_irq() are using threaded irqs, drop
>the hardirq handler option.
>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 03/10] cxl/pci: Remove hardirq handler for cxl_request_irq()

[Ira Weiny]

Dan Williams wrote:
> Now that all callers of cxl_request_irq() are using threaded irqs, drop
> the hardirq handler option.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH v3 03/10] cxl/pci: Remove hardirq handler for cxl_request_irq()

[Jonathan Cameron]

On Fri, 06 Oct 2023 00:26:16 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that all callers of cxl_request_irq() are using threaded irqs, drop
> the hardirq handler option.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 49d9b2ef5c5c..dc665b12be8f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -90,7 +90,7 @@ struct cxl_dev_id {
>  };
>  
>  static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
> -			   irq_handler_t handler, irq_handler_t thread_fn)
> +			   irq_handler_t thread_fn)
>  {
>  	struct device *dev = cxlds->dev;
>  	struct cxl_dev_id *dev_id;
> @@ -101,9 +101,9 @@ static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
>  		return -ENOMEM;
>  	dev_id->cxlds = cxlds;
>  
> -	return devm_request_threaded_irq(dev, irq, handler, thread_fn,
> -					 IRQF_SHARED | IRQF_ONESHOT,
> -					 NULL, dev_id);
> +	return devm_request_threaded_irq(dev, irq, NULL, thread_fn,
> +					 IRQF_SHARED | IRQF_ONESHOT, NULL,
> +					 dev_id);
>  }
>  
>  static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
> @@ -440,7 +440,7 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
>  	if (irq < 0)
>  		return 0;
>  
> -	if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
> +	if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq))
>  		return 0;
>  
>  	dev_dbg(cxlds->dev, "Mailbox interrupts enabled\n");
> @@ -638,7 +638,7 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
>  	if (irq < 0)
>  		return irq;
>  
> -	return cxl_request_irq(cxlds, irq, NULL, cxl_event_thread);
> +	return cxl_request_irq(cxlds, irq, cxl_event_thread);
>  }
>  
>  static int cxl_event_get_int_policy(struct cxl_memdev_state *mds,
> 
> 

Re: [PATCH v3 03/10] cxl/pci: Remove hardirq handler for cxl_request_irq()

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> Now that all callers of cxl_request_irq() are using threaded irqs, drop
> the hardirq handler option.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/pci.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 49d9b2ef5c5c..dc665b12be8f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -90,7 +90,7 @@ struct cxl_dev_id {
>  };
>  
>  static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
> -			   irq_handler_t handler, irq_handler_t thread_fn)
> +			   irq_handler_t thread_fn)
>  {
>  	struct device *dev = cxlds->dev;
>  	struct cxl_dev_id *dev_id;
> @@ -101,9 +101,9 @@ static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
>  		return -ENOMEM;
>  	dev_id->cxlds = cxlds;
>  
> -	return devm_request_threaded_irq(dev, irq, handler, thread_fn,
> -					 IRQF_SHARED | IRQF_ONESHOT,
> -					 NULL, dev_id);
> +	return devm_request_threaded_irq(dev, irq, NULL, thread_fn,
> +					 IRQF_SHARED | IRQF_ONESHOT, NULL,
> +					 dev_id);
>  }
>  
>  static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
> @@ -440,7 +440,7 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
>  	if (irq < 0)
>  		return 0;
>  
> -	if (cxl_request_irq(cxlds, irq, NULL, cxl_pci_mbox_irq))
> +	if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq))
>  		return 0;
>  
>  	dev_dbg(cxlds->dev, "Mailbox interrupts enabled\n");
> @@ -638,7 +638,7 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
>  	if (irq < 0)
>  		return irq;
>  
> -	return cxl_request_irq(cxlds, irq, NULL, cxl_event_thread);
> +	return cxl_request_irq(cxlds, irq, cxl_event_thread);
>  }
>  
>  static int cxl_event_get_int_policy(struct cxl_memdev_state *mds,
> 
> 

[PATCH v3 04/10] cxl/pci: Remove inconsistent usage of dev_err_probe()

[Dan Williams]

If dev_err_probe() is to be used it should at least be used consistently
within the same function. It is also worth questioning whether
every potential -ENOMEM needs an explicit error message.

Remove the cxl_setup_fw_upload() error prints for what are rare /
hardware-independent failures.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 2a7a07f6d165..6efe4e2a2cf5 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
 	struct cxl_dev_state *cxlds = &mds->cxlds;
 	struct device *dev = &cxlds->cxlmd->dev;
 	struct fw_upload *fwl;
-	int rc;
 
 	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds))
 		return 0;
@@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
 	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
 				       &cxl_memdev_fw_ops, mds);
 	if (IS_ERR(fwl))
-		return dev_err_probe(dev, PTR_ERR(fwl),
-				     "Failed to register firmware loader\n");
-
-	rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload,
-				      fwl);
-	if (rc)
-		dev_err(dev,
-			"Failed to add firmware loader remove action: %d\n",
-			rc);
+		return PTR_ERR(fwl);
 
-	return rc;
+	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
 

Re: [PATCH v3 04/10] cxl/pci: Remove inconsistent usage of dev_err_probe()

[Davidlohr Bueso]

On Fri, 06 Oct 2023, Dan Williams wrote:

>If dev_err_probe() is to be used it should at least be used consistently
>within the same function. It is also worth questioning whether
>every potential -ENOMEM needs an explicit error message.
>
>Remove the cxl_setup_fw_upload() error prints for what are rare /
>hardware-independent failures.
>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 04/10] cxl/pci: Remove inconsistent usage of dev_err_probe()

[Ira Weiny]

Dan Williams wrote:
> If dev_err_probe() is to be used it should at least be used consistently
> within the same function. It is also worth questioning whether
> every potential -ENOMEM needs an explicit error message.
> 
> Remove the cxl_setup_fw_upload() error prints for what are rare /
> hardware-independent failures.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems ok.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH v3 04/10] cxl/pci: Remove inconsistent usage of dev_err_probe()

[Jonathan Cameron]

On Fri, 06 Oct 2023 00:26:22 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> If dev_err_probe() is to be used it should at least be used consistently
> within the same function. It is also worth questioning whether
> every potential -ENOMEM needs an explicit error message.
> 
> Remove the cxl_setup_fw_upload() error prints for what are rare /
> hardware-independent failures.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Vanishingly unlikely this would ever fail under anything approach
a normal situation, so fair enough to drop the error message.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/memdev.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 2a7a07f6d165..6efe4e2a2cf5 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct device *dev = &cxlds->cxlmd->dev;
>  	struct fw_upload *fwl;
> -	int rc;
>  
>  	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds))
>  		return 0;
> @@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
>  				       &cxl_memdev_fw_ops, mds);
>  	if (IS_ERR(fwl))
> -		return dev_err_probe(dev, PTR_ERR(fwl),
> -				     "Failed to register firmware loader\n");
> -
> -	rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload,
> -				      fwl);
> -	if (rc)
> -		dev_err(dev,
> -			"Failed to add firmware loader remove action: %d\n",
> -			rc);
> +		return PTR_ERR(fwl);
>  
> -	return rc;
> +	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
>  
> 
> 

Re: [PATCH v3 04/10] cxl/pci: Remove inconsistent usage of dev_err_probe()

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> If dev_err_probe() is to be used it should at least be used consistently
> within the same function. It is also worth questioning whether
> every potential -ENOMEM needs an explicit error message.
> 
> Remove the cxl_setup_fw_upload() error prints for what are rare /
> hardware-independent failures.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/memdev.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 2a7a07f6d165..6efe4e2a2cf5 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct device *dev = &cxlds->cxlmd->dev;
>  	struct fw_upload *fwl;
> -	int rc;
>  
>  	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds))
>  		return 0;
> @@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
>  				       &cxl_memdev_fw_ops, mds);
>  	if (IS_ERR(fwl))
> -		return dev_err_probe(dev, PTR_ERR(fwl),
> -				     "Failed to register firmware loader\n");
> -
> -	rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload,
> -				      fwl);
> -	if (rc)
> -		dev_err(dev,
> -			"Failed to add firmware loader remove action: %d\n",
> -			rc);
> +		return PTR_ERR(fwl);
>  
> -	return rc;
> +	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
>  
> 
> 

[PATCH v3 05/10] cxl/pci: Clarify devm host for memdev relative setup

[Dan Williams]

It is all too easy to get confused about @dev usage in the CXL driver
stack. Before adding a new cxl_pci_probe() setup operation that has a
devm lifetime dependent on @cxlds->dev binding, but also references
@cxlmd->dev, and prints messages, rework the devm_cxl_add_memdev() and
cxl_memdev_setup_fw_upload() function signatures to make this
distinction explicit. I.e. pass in the devm context as an @host argument
rather than infer it from other objects.

This is in preparation for adding a devm_cxl_sanitize_setup_notifier().

Note the whitespace fixup near the change of the devm_cxl_add_memdev()
signature. That uncaught typo originated in the patch that added
cxl_memdev_security_init().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c    |   16 ++++++++--------
 drivers/cxl/cxlmem.h         |    5 +++--
 drivers/cxl/pci.c            |    4 ++--
 tools/testing/cxl/test/mem.c |    4 ++--
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 6efe4e2a2cf5..63353d990374 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -960,12 +960,12 @@ static const struct fw_upload_ops cxl_memdev_fw_ops = {
         .cleanup = cxl_fw_cleanup,
 };
 
-static void devm_cxl_remove_fw_upload(void *fwl)
+static void cxl_remove_fw_upload(void *fwl)
 {
 	firmware_upload_unregister(fwl);
 }
 
-int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
+int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds)
 {
 	struct cxl_dev_state *cxlds = &mds->cxlds;
 	struct device *dev = &cxlds->cxlmd->dev;
@@ -978,10 +978,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
 				       &cxl_memdev_fw_ops, mds);
 	if (IS_ERR(fwl))
 		return PTR_ERR(fwl);
-
-	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
+	return devm_add_action_or_reset(host, cxl_remove_fw_upload, fwl);
 }
-EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
+EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fw_upload, CXL);
 
 static const struct file_operations cxl_memdev_fops = {
 	.owner = THIS_MODULE,
@@ -1019,9 +1018,10 @@ static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
 	}
 
 	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
- }
+}
 
-struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
+struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
+				       struct cxl_dev_state *cxlds)
 {
 	struct cxl_memdev *cxlmd;
 	struct device *dev;
@@ -1053,7 +1053,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
 	if (rc)
 		goto err;
 
-	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
+	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
 	if (rc)
 		return ERR_PTR(rc);
 	return cxlmd;
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 55f00ad17a77..fdb2c8dd98d0 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -84,9 +84,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 	return is_cxl_memdev(port->uport_dev);
 }
 
-struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
+struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
+				       struct cxl_dev_state *cxlds);
 struct cxl_memdev_state;
-int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds);
+int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
 int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 			 resource_size_t base, resource_size_t len,
 			 resource_size_t skipped);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index dc665b12be8f..7c117eb62750 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -867,11 +867,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	cxlmd = devm_cxl_add_memdev(cxlds);
+	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
-	rc = cxl_memdev_setup_fw_upload(mds);
+	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
 	if (rc)
 		return rc;
 
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 464fc39ed277..68118c37f0b5 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1450,11 +1450,11 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	mdata->mes.mds = mds;
 	cxl_mock_add_event_logs(&mdata->mes);
 
-	cxlmd = devm_cxl_add_memdev(cxlds);
+	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
-	rc = cxl_memdev_setup_fw_upload(mds);
+	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
 	if (rc)
 		return rc;
 

Re: [PATCH v3 05/10] cxl/pci: Clarify devm host for memdev relative setup

[Ira Weiny]

Dan Williams wrote:
> It is all too easy to get confused about @dev usage in the CXL driver
> stack. Before adding a new cxl_pci_probe() setup operation that has a
> devm lifetime dependent on @cxlds->dev binding, but also references
> @cxlmd->dev, and prints messages, rework the devm_cxl_add_memdev() and
> cxl_memdev_setup_fw_upload() function signatures to make this
> distinction explicit. I.e. pass in the devm context as an @host argument
> rather than infer it from other objects.
> 
> This is in preparation for adding a devm_cxl_sanitize_setup_notifier().
> 
> Note the whitespace fixup near the change of the devm_cxl_add_memdev()
> signature. That uncaught typo originated in the patch that added
> cxl_memdev_security_init().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/memdev.c    |   16 ++++++++--------
>  drivers/cxl/cxlmem.h         |    5 +++--
>  drivers/cxl/pci.c            |    4 ++--
>  tools/testing/cxl/test/mem.c |    4 ++--
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 6efe4e2a2cf5..63353d990374 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -960,12 +960,12 @@ static const struct fw_upload_ops cxl_memdev_fw_ops = {
>          .cleanup = cxl_fw_cleanup,
>  };
>  
> -static void devm_cxl_remove_fw_upload(void *fwl)
> +static void cxl_remove_fw_upload(void *fwl)

Technically this drop of devm (and add to the setup call below) is not
covered in the cover letter.  However, I like it.


Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  {
>  	firmware_upload_unregister(fwl);
>  }
>  
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds)
>  {
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct device *dev = &cxlds->cxlmd->dev;
> @@ -978,10 +978,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  				       &cxl_memdev_fw_ops, mds);
>  	if (IS_ERR(fwl))
>  		return PTR_ERR(fwl);
> -
> -	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
> +	return devm_add_action_or_reset(host, cxl_remove_fw_upload, fwl);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fw_upload, CXL);
>  
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
> @@ -1019,9 +1018,10 @@ static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
>  	}
>  
>  	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
> - }
> +}
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_memdev *cxlmd;
>  	struct device *dev;
> @@ -1053,7 +1053,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		goto err;
>  
> -	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> +	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 55f00ad17a77..fdb2c8dd98d0 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -84,9 +84,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  	return is_cxl_memdev(port->uport_dev);
>  }
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds);
>  struct cxl_memdev_state;
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds);
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
>  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			 resource_size_t base, resource_size_t len,
>  			 resource_size_t skipped);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index dc665b12be8f..7c117eb62750 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -867,11 +867,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 464fc39ed277..68118c37f0b5 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -1450,11 +1450,11 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	mdata->mes.mds = mds;
>  	cxl_mock_add_event_logs(&mdata->mes);
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> 
> 

Re: [PATCH v3 05/10] cxl/pci: Clarify devm host for memdev relative setup

[Jonathan Cameron]

On Fri, 06 Oct 2023 00:26:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> It is all too easy to get confused about @dev usage in the CXL driver
> stack. Before adding a new cxl_pci_probe() setup operation that has a
> devm lifetime dependent on @cxlds->dev binding, but also references
> @cxlmd->dev, and prints messages, rework the devm_cxl_add_memdev() and
> cxl_memdev_setup_fw_upload() function signatures to make this
> distinction explicit. I.e. pass in the devm context as an @host argument
> rather than infer it from other objects.
> 
> This is in preparation for adding a devm_cxl_sanitize_setup_notifier().
> 
> Note the whitespace fixup near the change of the devm_cxl_add_memdev()
> signature. That uncaught typo originated in the patch that added
> cxl_memdev_security_init().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Definitely an improvement in readability. Nice.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/memdev.c    |   16 ++++++++--------
>  drivers/cxl/cxlmem.h         |    5 +++--
>  drivers/cxl/pci.c            |    4 ++--
>  tools/testing/cxl/test/mem.c |    4 ++--
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 6efe4e2a2cf5..63353d990374 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -960,12 +960,12 @@ static const struct fw_upload_ops cxl_memdev_fw_ops = {
>          .cleanup = cxl_fw_cleanup,
>  };
>  
> -static void devm_cxl_remove_fw_upload(void *fwl)
> +static void cxl_remove_fw_upload(void *fwl)
>  {
>  	firmware_upload_unregister(fwl);
>  }
>  
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds)
>  {
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct device *dev = &cxlds->cxlmd->dev;
> @@ -978,10 +978,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  				       &cxl_memdev_fw_ops, mds);
>  	if (IS_ERR(fwl))
>  		return PTR_ERR(fwl);
> -
> -	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
> +	return devm_add_action_or_reset(host, cxl_remove_fw_upload, fwl);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fw_upload, CXL);
>  
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
> @@ -1019,9 +1018,10 @@ static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
>  	}
>  
>  	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
> - }
> +}
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_memdev *cxlmd;
>  	struct device *dev;
> @@ -1053,7 +1053,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		goto err;
>  
> -	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> +	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 55f00ad17a77..fdb2c8dd98d0 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -84,9 +84,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  	return is_cxl_memdev(port->uport_dev);
>  }
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds);
>  struct cxl_memdev_state;
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds);
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
>  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			 resource_size_t base, resource_size_t len,
>  			 resource_size_t skipped);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index dc665b12be8f..7c117eb62750 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -867,11 +867,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 464fc39ed277..68118c37f0b5 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -1450,11 +1450,11 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	mdata->mes.mds = mds;
>  	cxl_mock_add_event_logs(&mdata->mes);
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> 
> 

Re: [PATCH v3 05/10] cxl/pci: Clarify devm host for memdev relative setup

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> It is all too easy to get confused about @dev usage in the CXL driver
> stack. Before adding a new cxl_pci_probe() setup operation that has a
> devm lifetime dependent on @cxlds->dev binding, but also references
> @cxlmd->dev, and prints messages, rework the devm_cxl_add_memdev() and
> cxl_memdev_setup_fw_upload() function signatures to make this
> distinction explicit. I.e. pass in the devm context as an @host argument
> rather than infer it from other objects.
> 
> This is in preparation for adding a devm_cxl_sanitize_setup_notifier().
> 
> Note the whitespace fixup near the change of the devm_cxl_add_memdev()
> signature. That uncaught typo originated in the patch that added
> cxl_memdev_security_init().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/memdev.c    |   16 ++++++++--------
>  drivers/cxl/cxlmem.h         |    5 +++--
>  drivers/cxl/pci.c            |    4 ++--
>  tools/testing/cxl/test/mem.c |    4 ++--
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 6efe4e2a2cf5..63353d990374 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -960,12 +960,12 @@ static const struct fw_upload_ops cxl_memdev_fw_ops = {
>          .cleanup = cxl_fw_cleanup,
>  };
>  
> -static void devm_cxl_remove_fw_upload(void *fwl)
> +static void cxl_remove_fw_upload(void *fwl)
>  {
>  	firmware_upload_unregister(fwl);
>  }
>  
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds)
>  {
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct device *dev = &cxlds->cxlmd->dev;
> @@ -978,10 +978,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds)
>  				       &cxl_memdev_fw_ops, mds);
>  	if (IS_ERR(fwl))
>  		return PTR_ERR(fwl);
> -
> -	return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl);
> +	return devm_add_action_or_reset(host, cxl_remove_fw_upload, fwl);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fw_upload, CXL);
>  
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
> @@ -1019,9 +1018,10 @@ static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
>  	}
>  
>  	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
> - }
> +}
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_memdev *cxlmd;
>  	struct device *dev;
> @@ -1053,7 +1053,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		goto err;
>  
> -	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> +	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 55f00ad17a77..fdb2c8dd98d0 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -84,9 +84,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  	return is_cxl_memdev(port->uport_dev);
>  }
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds);
>  struct cxl_memdev_state;
> -int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds);
> +int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
>  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			 resource_size_t base, resource_size_t len,
>  			 resource_size_t skipped);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index dc665b12be8f..7c117eb62750 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -867,11 +867,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 464fc39ed277..68118c37f0b5 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -1450,11 +1450,11 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	mdata->mes.mds = mds;
>  	cxl_mock_add_event_logs(&mdata->mes);
>  
> -	cxlmd = devm_cxl_add_memdev(cxlds);
> +	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> -	rc = cxl_memdev_setup_fw_upload(mds);
> +	rc = devm_cxl_setup_fw_upload(&pdev->dev, mds);
>  	if (rc)
>  		return rc;
>  
> 
> 

[PATCH v3 06/10] cxl/pci: Fix sanitize notifier setup

[Dan Williams]

Fix a race condition between the mailbox-background command interrupt
firing and the security-state sysfs attribute being removed.

The race is difficult to see due to the awkward placement of the
sanitize-notifier setup code and the multiple places the teardown calls
are made, cxl_memdev_security_init() and cxl_memdev_security_shutdown().

Unify setup in one place, cxl_sanitize_setup_notifier(). Arrange for
the paired cxl_sanitize_teardown_notifier() to safely quiet the notifier
and let the cxl_memdev + irq be unregistered later in the flow.

Note: The special wrinkle of the sanitize notifier is that it interacts
with interrupts, which are enabled early in the flow, and it interacts
with memdev sysfs which is not initialized until late in the flow. Hence
why this setup routine takes an @cxlmd argument, and not just @mds.

This fix is also needed as a preparation fix for a memdev unregistration
crash.

Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Closes: http://lore.kernel.org/r/20230929100316.00004546@Huawei.com
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |   86 +++++++++++++++++++++++----------------------
 drivers/cxl/cxlmem.h      |    2 +
 drivers/cxl/pci.c         |    4 ++
 3 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 63353d990374..4c2e24a1a89c 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -556,20 +556,11 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 }
 EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, CXL);
 
-static void cxl_memdev_security_shutdown(struct device *dev)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
-
-	cancel_delayed_work_sync(&mds->security.poll_dwork);
-}
-
 static void cxl_memdev_shutdown(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 
 	down_write(&cxl_memdev_rwsem);
-	cxl_memdev_security_shutdown(dev);
 	cxlmd->cxlds = NULL;
 	up_write(&cxl_memdev_rwsem);
 }
@@ -991,35 +982,6 @@ static const struct file_operations cxl_memdev_fops = {
 	.llseek = noop_llseek,
 };
 
-static void put_sanitize(void *data)
-{
-	struct cxl_memdev_state *mds = data;
-
-	sysfs_put(mds->security.sanitize_node);
-}
-
-static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
-{
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	struct device *dev = &cxlmd->dev;
-	struct kernfs_node *sec;
-
-	sec = sysfs_get_dirent(dev->kobj.sd, "security");
-	if (!sec) {
-		dev_err(dev, "sysfs_get_dirent 'security' failed\n");
-		return -ENODEV;
-	}
-	mds->security.sanitize_node = sysfs_get_dirent(sec, "state");
-	sysfs_put(sec);
-	if (!mds->security.sanitize_node) {
-		dev_err(dev, "sysfs_get_dirent 'state' failed\n");
-		return -ENODEV;
-	}
-
-	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
-}
-
 struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 				       struct cxl_dev_state *cxlds)
 {
@@ -1049,10 +1011,6 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 	if (rc)
 		goto err;
 
-	rc = cxl_memdev_security_init(cxlmd);
-	if (rc)
-		goto err;
-
 	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
 	if (rc)
 		return ERR_PTR(rc);
@@ -1069,6 +1027,50 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
 
+static void sanitize_teardown_notifier(void *data)
+{
+	struct cxl_memdev_state *mds = data;
+	struct kernfs_node *state;
+
+	/*
+	 * Prevent new irq triggered invocations of the workqueue and
+	 * flush inflight invocations.
+	 */
+	mutex_lock(&mds->mbox_mutex);
+	state = mds->security.sanitize_node;
+	mds->security.sanitize_node = NULL;
+	mutex_unlock(&mds->mbox_mutex);
+
+	cancel_delayed_work_sync(&mds->security.poll_dwork);
+	sysfs_put(state);
+}
+
+int devm_cxl_sanitize_setup_notifier(struct device *host,
+				     struct cxl_memdev *cxlmd)
+{
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct kernfs_node *sec;
+
+	if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
+		return 0;
+
+	/*
+	 * Note, the expectation is that @cxlmd would have failed to be
+	 * created if these sysfs_get_dirent calls fail.
+	 */
+	sec = sysfs_get_dirent(cxlmd->dev.kobj.sd, "security");
+	if (!sec)
+		return -ENOENT;
+	mds->security.sanitize_node = sysfs_get_dirent(sec, "state");
+	sysfs_put(sec);
+	if (!mds->security.sanitize_node)
+		return -ENOENT;
+
+	return devm_add_action_or_reset(host, sanitize_teardown_notifier, mds);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_sanitize_setup_notifier, CXL);
+
 __init int cxl_memdev_init(void)
 {
 	dev_t devt;
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index fdb2c8dd98d0..fbdee1d63717 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -86,6 +86,8 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 
 struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 				       struct cxl_dev_state *cxlds);
+int devm_cxl_sanitize_setup_notifier(struct device *host,
+				     struct cxl_memdev *cxlmd);
 struct cxl_memdev_state;
 int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
 int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 7c117eb62750..9955871e9ec1 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -875,6 +875,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
+	if (rc)
+		return rc;
+
 	pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU);
 	for (i = 0; i < pmu_count; i++) {
 		struct cxl_pmu_regs pmu_regs;

Re: [PATCH v3 06/10] cxl/pci: Fix sanitize notifier setup

[Jonathan Cameron]

On Fri, 06 Oct 2023 00:26:33 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Fix a race condition between the mailbox-background command interrupt
> firing and the security-state sysfs attribute being removed.
> 
> The race is difficult to see due to the awkward placement of the
> sanitize-notifier setup code and the multiple places the teardown calls
> are made, cxl_memdev_security_init() and cxl_memdev_security_shutdown().
> 
> Unify setup in one place, cxl_sanitize_setup_notifier(). Arrange for
> the paired cxl_sanitize_teardown_notifier() to safely quiet the notifier
> and let the cxl_memdev + irq be unregistered later in the flow.
> 
> Note: The special wrinkle of the sanitize notifier is that it interacts
> with interrupts, which are enabled early in the flow, and it interacts
> with memdev sysfs which is not initialized until late in the flow. Hence
> why this setup routine takes an @cxlmd argument, and not just @mds.

Fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> This fix is also needed as a preparation fix for a memdev unregistration
> crash.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20230929100316.00004546@Huawei.com
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/memdev.c |   86 +++++++++++++++++++++++----------------------
>  drivers/cxl/cxlmem.h      |    2 +
>  drivers/cxl/pci.c         |    4 ++
>  3 files changed, 50 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 63353d990374..4c2e24a1a89c 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -556,20 +556,11 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  }
>  EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, CXL);
>  
> -static void cxl_memdev_security_shutdown(struct device *dev)
> -{
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> -
> -	cancel_delayed_work_sync(&mds->security.poll_dwork);
> -}
> -
>  static void cxl_memdev_shutdown(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  
>  	down_write(&cxl_memdev_rwsem);
> -	cxl_memdev_security_shutdown(dev);
>  	cxlmd->cxlds = NULL;
>  	up_write(&cxl_memdev_rwsem);
>  }
> @@ -991,35 +982,6 @@ static const struct file_operations cxl_memdev_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> -static void put_sanitize(void *data)
> -{
> -	struct cxl_memdev_state *mds = data;
> -
> -	sysfs_put(mds->security.sanitize_node);
> -}
> -
> -static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
> -{
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct device *dev = &cxlmd->dev;
> -	struct kernfs_node *sec;
> -
> -	sec = sysfs_get_dirent(dev->kobj.sd, "security");
> -	if (!sec) {
> -		dev_err(dev, "sysfs_get_dirent 'security' failed\n");
> -		return -ENODEV;
> -	}
> -	mds->security.sanitize_node = sysfs_get_dirent(sec, "state");
> -	sysfs_put(sec);
> -	if (!mds->security.sanitize_node) {
> -		dev_err(dev, "sysfs_get_dirent 'state' failed\n");
> -		return -ENODEV;
> -	}
> -
> -	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
> -}
> -
>  struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  				       struct cxl_dev_state *cxlds)
>  {
> @@ -1049,10 +1011,6 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  	if (rc)
>  		goto err;
>  
> -	rc = cxl_memdev_security_init(cxlmd);
> -	if (rc)
> -		goto err;
> -
>  	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
> @@ -1069,6 +1027,50 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
>  
> +static void sanitize_teardown_notifier(void *data)
> +{
> +	struct cxl_memdev_state *mds = data;
> +	struct kernfs_node *state;
> +
> +	/*
> +	 * Prevent new irq triggered invocations of the workqueue and
> +	 * flush inflight invocations.
> +	 */
> +	mutex_lock(&mds->mbox_mutex);
> +	state = mds->security.sanitize_node;
> +	mds->security.sanitize_node = NULL;
> +	mutex_unlock(&mds->mbox_mutex);
> +
> +	cancel_delayed_work_sync(&mds->security.poll_dwork);
> +	sysfs_put(state);
> +}
> +
> +int devm_cxl_sanitize_setup_notifier(struct device *host,
> +				     struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +	struct kernfs_node *sec;
> +
> +	if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
> +		return 0;
> +
> +	/*
> +	 * Note, the expectation is that @cxlmd would have failed to be
> +	 * created if these sysfs_get_dirent calls fail.
> +	 */
> +	sec = sysfs_get_dirent(cxlmd->dev.kobj.sd, "security");
> +	if (!sec)
> +		return -ENOENT;
> +	mds->security.sanitize_node = sysfs_get_dirent(sec, "state");
> +	sysfs_put(sec);
> +	if (!mds->security.sanitize_node)
> +		return -ENOENT;
> +
> +	return devm_add_action_or_reset(host, sanitize_teardown_notifier, mds);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_sanitize_setup_notifier, CXL);
> +
>  __init int cxl_memdev_init(void)
>  {
>  	dev_t devt;
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index fdb2c8dd98d0..fbdee1d63717 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -86,6 +86,8 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  
>  struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  				       struct cxl_dev_state *cxlds);
> +int devm_cxl_sanitize_setup_notifier(struct device *host,
> +				     struct cxl_memdev *cxlmd);
>  struct cxl_memdev_state;
>  int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds);
>  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 7c117eb62750..9955871e9ec1 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -875,6 +875,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
> +	if (rc)
> +		return rc;
> +
>  	pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU);
>  	for (i = 0; i < pmu_count; i++) {
>  		struct cxl_pmu_regs pmu_regs;
> 
> 

Re: [PATCH v3 06/10] cxl/pci: Fix sanitize notifier setup

[Davidlohr Bueso]

On Fri, 06 Oct 2023, Dan Williams wrote:

>Fix a race condition between the mailbox-background command interrupt
>firing and the security-state sysfs attribute being removed.
>
>The race is difficult to see due to the awkward placement of the
>sanitize-notifier setup code and the multiple places the teardown calls
>are made, cxl_memdev_security_init() and cxl_memdev_security_shutdown().
>
>Unify setup in one place, cxl_sanitize_setup_notifier(). Arrange for
>the paired cxl_sanitize_teardown_notifier() to safely quiet the notifier
>and let the cxl_memdev + irq be unregistered later in the flow.
>
>Note: The special wrinkle of the sanitize notifier is that it interacts
>with interrupts, which are enabled early in the flow, and it interacts
>with memdev sysfs which is not initialized until late in the flow. Hence
>why this setup routine takes an @cxlmd argument, and not just @mds.
>
>This fix is also needed as a preparation fix for a memdev unregistration
>crash.
>
>Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>Closes: http://lore.kernel.org/r/20230929100316.00004546@Huawei.com
>Cc: Dave Jiang <dave.jiang@intel.com>
>Cc: Davidlohr Bueso <dave@stgolabs.net>
>Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
>Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

[PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Dan Williams]

The sanitize operation is destructive and the expectation is that the
device is unmapped while in progress. The current implementation does a
lockless check for decoders being active, but then does nothing to
prevent decoders from racing to be committed. Introduce state tracking
to resolve this race.

This incidentally cleans up unpriveleged userspace from triggering mmio
read cycles by spinning on reading the 'securiry/state' attribute. Which
at a minimum is a waste since the kernel state machine can cache the
completion result.

Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
original implementation, but an export was never required.

Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
Cc: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/core.h   |    1 +
 drivers/cxl/core/hdm.c    |   19 ++++++++++++++++
 drivers/cxl/core/mbox.c   |   55 +++++++++++++++++++++++++++++++++------------
 drivers/cxl/core/memdev.c |   43 +++++++++++++----------------------
 drivers/cxl/core/port.c   |    6 +++++
 drivers/cxl/core/region.c |    6 -----
 drivers/cxl/cxlmem.h      |    4 ++-
 drivers/cxl/pci.c         |    5 ++++
 8 files changed, 90 insertions(+), 49 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 45e7e044cf4a..8e5f3d84311e 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -75,6 +75,7 @@ resource_size_t __rcrb_to_component(struct device *dev,
 				    enum cxl_rcrb which);
 
 extern struct rw_semaphore cxl_dpa_rwsem;
+extern struct rw_semaphore cxl_region_rwsem;
 
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 4449b34a80cc..506c9e14cdf9 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 		return -EBUSY;
 	}
 
+	/*
+	 * For endpoint decoders hosted on CXL memory devices that
+	 * support the sanitize operation, make sure sanitize is not in-flight.
+	 */
+	if (is_endpoint_decoder(&cxld->dev)) {
+		struct cxl_endpoint_decoder *cxled =
+			to_cxl_endpoint_decoder(&cxld->dev);
+		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+		struct cxl_memdev_state *mds =
+			to_cxl_memdev_state(cxlmd->cxlds);
+
+		if (mds && mds->security.sanitize_active) {
+			dev_dbg(&cxlmd->dev,
+				"attempted to commit %s during sanitize\n",
+				dev_name(&cxld->dev));
+			return -EBUSY;
+		}
+	}
+
 	down_read(&cxl_dpa_rwsem);
 	/* common decoder settings */
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 4df4f614f490..67aec57cc12f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1125,20 +1125,7 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_dev_state_identify, CXL);
 
-/**
- * cxl_mem_sanitize() - Send a sanitization command to the device.
- * @mds: The device data for the operation
- * @cmd: The specific sanitization command opcode
- *
- * Return: 0 if the command was executed successfully, regardless of
- * whether or not the actual security operation is done in the background,
- * such as for the Sanitize case.
- * Error return values can be the result of the mailbox command, -EINVAL
- * when security requirements are not met or invalid contexts.
- *
- * See CXL 3.0 @8.2.9.8.5.1 Sanitize and @8.2.9.8.5.2 Secure Erase.
- */
-int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
+static int __cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
 {
 	int rc;
 	u32 sec_out = 0;
@@ -1183,7 +1170,45 @@ int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
 
 	return 0;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
+
+
+/**
+ * cxl_mem_sanitize() - Send a sanitization command to the device.
+ * @mds: The device for the operation
+ * @cmd: The specific sanitization command opcode
+ *
+ * Return: 0 if the command was executed successfully, regardless of
+ * whether or not the actual security operation is done in the background,
+ * such as for the Sanitize case.
+ * Error return values can be the result of the mailbox command, -EINVAL
+ * when security requirements are not met or invalid contexts, or -EBUSY
+ * if the sanitize operation is already in flight.
+ *
+ * See CXL 3.0 @8.2.9.8.5.1 Sanitize and @8.2.9.8.5.2 Secure Erase.
+ */
+int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
+{
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
+	struct cxl_port  *endpoint;
+	int rc;
+
+	/* synchronize with cxl_mem_probe() and decoder write operations */
+	device_lock(&cxlmd->dev);
+	endpoint = cxlmd->endpoint;
+	down_read(&cxl_region_rwsem);
+	/*
+	 * Require an endpoint to be safe otherwise the driver can not
+	 * be sure that the device is unmapped.
+	 */
+	if (endpoint && endpoint->commit_end == -1)
+		rc = __cxl_mem_sanitize(mds, cmd);
+	else
+		rc = -EBUSY;
+	up_read(&cxl_region_rwsem);
+	device_unlock(&cxlmd->dev);
+
+	return rc;
+}
 
 static int add_dpa_res(struct device *dev, struct resource *parent,
 		       struct resource *res, resource_size_t start,
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 4c2e24a1a89c..a02061028b71 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -125,13 +125,16 @@ static ssize_t security_state_show(struct device *dev,
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	u64 reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
-	u32 pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, reg);
-	u16 cmd = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
 	unsigned long state = mds->security.state;
+	int rc = 0;
 
-	if (cmd == CXL_MBOX_OP_SANITIZE && pct != 100)
-		return sysfs_emit(buf, "sanitize\n");
+	/* sync with latest submission state */
+	mutex_lock(&mds->mbox_mutex);
+	if (mds->security.sanitize_active)
+		rc = sysfs_emit(buf, "sanitize\n");
+	mutex_unlock(&mds->mbox_mutex);
+	if (rc)
+		return rc;
 
 	if (!(state & CXL_PMEM_SEC_STATE_USER_PASS_SET))
 		return sysfs_emit(buf, "disabled\n");
@@ -152,24 +155,17 @@ static ssize_t security_sanitize_store(struct device *dev,
 				       const char *buf, size_t len)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
-	struct cxl_port *port = cxlmd->endpoint;
 	bool sanitize;
 	ssize_t rc;
 
 	if (kstrtobool(buf, &sanitize) || !sanitize)
 		return -EINVAL;
 
-	if (!port || !is_cxl_endpoint(port))
-		return -EINVAL;
-
-	/* ensure no regions are mapped to this memdev */
-	if (port->commit_end != -1)
-		return -EBUSY;
-
-	rc = cxl_mem_sanitize(mds, CXL_MBOX_OP_SANITIZE);
+	rc = cxl_mem_sanitize(cxlmd, CXL_MBOX_OP_SANITIZE);
+	if (rc)
+		return rc;
 
-	return rc ? rc : len;
+	return len;
 }
 static struct device_attribute dev_attr_security_sanitize =
 	__ATTR(sanitize, 0200, NULL, security_sanitize_store);
@@ -179,24 +175,17 @@ static ssize_t security_erase_store(struct device *dev,
 				    const char *buf, size_t len)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
-	struct cxl_port *port = cxlmd->endpoint;
 	ssize_t rc;
 	bool erase;
 
 	if (kstrtobool(buf, &erase) || !erase)
 		return -EINVAL;
 
-	if (!port || !is_cxl_endpoint(port))
-		return -EINVAL;
-
-	/* ensure no regions are mapped to this memdev */
-	if (port->commit_end != -1)
-		return -EBUSY;
-
-	rc = cxl_mem_sanitize(mds, CXL_MBOX_OP_SECURE_ERASE);
+	rc = cxl_mem_sanitize(cxlmd, CXL_MBOX_OP_SECURE_ERASE);
+	if (rc)
+		return rc;
 
-	return rc ? rc : len;
+	return len;
 }
 static struct device_attribute dev_attr_security_erase =
 	__ATTR(erase, 0200, NULL, security_erase_store);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 7ca01a834e18..5ba606c6e03f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -28,6 +28,12 @@
  * instantiated by the core.
  */
 
+/*
+ * All changes to the interleave configuration occur with this lock held
+ * for write.
+ */
+DECLARE_RWSEM(cxl_region_rwsem);
+
 static DEFINE_IDA(cxl_port_ida);
 static DEFINE_XARRAY(cxl_root_buses);
 
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..d74bf1b664b6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -28,12 +28,6 @@
  * 3. Decoder targets
  */
 
-/*
- * All changes to the interleave configuration occur with this lock held
- * for write.
- */
-static DECLARE_RWSEM(cxl_region_rwsem);
-
 static struct cxl_region *to_cxl_region(struct device *dev);
 
 static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index fbdee1d63717..6933bc20e76b 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -364,6 +364,7 @@ struct cxl_fw_state {
  * @state: state of last security operation
  * @enabled_cmds: All security commands enabled in the CEL
  * @poll_tmo_secs: polling timeout
+ * @sanitize_active: sanitize completion pending
  * @poll_dwork: polling work item
  * @sanitize_node: sanitation sysfs file to notify
  */
@@ -371,6 +372,7 @@ struct cxl_security_state {
 	unsigned long state;
 	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
 	int poll_tmo_secs;
+	bool sanitize_active;
 	struct delayed_work poll_dwork;
 	struct kernfs_node *sanitize_node;
 };
@@ -884,7 +886,7 @@ static inline void cxl_mem_active_dec(void)
 }
 #endif
 
-int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd);
+int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd);
 
 struct cxl_hdm {
 	struct cxl_component_regs regs;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 9955871e9ec1..06fafe59c054 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -154,6 +154,7 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
 		mds->security.poll_tmo_secs = 0;
 		if (mds->security.sanitize_node)
 			sysfs_notify_dirent(mds->security.sanitize_node);
+		mds->security.sanitize_active = false;
 
 		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
 	} else {
@@ -292,9 +293,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
 		 * and allow userspace to poll(2) for completion.
 		 */
 		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
+			if (mds->security.sanitize_active)
+				return -EBUSY;
+
 			/* give first timeout a second */
 			timeout = 1;
 			mds->security.poll_tmo_secs = timeout;
+			mds->security.sanitize_active = true;
 			schedule_delayed_work(&mds->security.poll_dwork,
 					      timeout * HZ);
 			dev_dbg(dev, "Sanitization operation started\n");

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[kernel test robot]

Hi Dan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 6465e260f48790807eef06b583b38ca9789b6072]

url:    https://github.com/intel-lab-lkp/linux/commits/Dan-Williams/cxl-pci-Remove-unnecessary-device-reference-management-in-sanitize-work/20231006-152823
base:   6465e260f48790807eef06b583b38ca9789b6072
patch link:    https://lore.kernel.org/r/169657719974.1491153.15276451196916291864.stgit%40dwillia2-xfh.jf.intel.com
patch subject: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20231006/202310061706.JTytkmZT-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231006/202310061706.JTytkmZT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310061706.JTytkmZT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/cxl/core/mbox.c:1190: warning: Function parameter or member 'cxlmd' not described in 'cxl_mem_sanitize'
>> drivers/cxl/core/mbox.c:1190: warning: Excess function parameter 'mds' description in 'cxl_mem_sanitize'


vim +1190 drivers/cxl/core/mbox.c

  1173	
  1174	
  1175	/**
  1176	 * cxl_mem_sanitize() - Send a sanitization command to the device.
  1177	 * @mds: The device for the operation
  1178	 * @cmd: The specific sanitization command opcode
  1179	 *
  1180	 * Return: 0 if the command was executed successfully, regardless of
  1181	 * whether or not the actual security operation is done in the background,
  1182	 * such as for the Sanitize case.
  1183	 * Error return values can be the result of the mailbox command, -EINVAL
  1184	 * when security requirements are not met or invalid contexts, or -EBUSY
  1185	 * if the sanitize operation is already in flight.
  1186	 *
  1187	 * See CXL 3.0 @8.2.9.8.5.1 Sanitize and @8.2.9.8.5.2 Secure Erase.
  1188	 */
  1189	int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> 1190	{
  1191		struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
  1192		struct cxl_port  *endpoint;
  1193		int rc;
  1194	
  1195		/* synchronize with cxl_mem_probe() and decoder write operations */
  1196		device_lock(&cxlmd->dev);
  1197		endpoint = cxlmd->endpoint;
  1198		down_read(&cxl_region_rwsem);
  1199		/*
  1200		 * Require an endpoint to be safe otherwise the driver can not
  1201		 * be sure that the device is unmapped.
  1202		 */
  1203		if (endpoint && endpoint->commit_end == -1)
  1204			rc = __cxl_mem_sanitize(mds, cmd);
  1205		else
  1206			rc = -EBUSY;
  1207		up_read(&cxl_region_rwsem);
  1208		device_unlock(&cxlmd->dev);
  1209	
  1210		return rc;
  1211	}
  1212	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Ira Weiny]

Dan Williams wrote:
> The sanitize operation is destructive and the expectation is that the
> device is unmapped while in progress. The current implementation does a
> lockless check for decoders being active, but then does nothing to
> prevent decoders from racing to be committed. Introduce state tracking
> to resolve this race.
> 
> This incidentally cleans up unpriveleged userspace from triggering mmio
> read cycles by spinning on reading the 'securiry/state' attribute. Which
> at a minimum is a waste since the kernel state machine can cache the
> completion result.
> 
> Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> original implementation, but an export was never required.
> 
> Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---

[snip]

> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  		return -EBUSY;
>  	}
>  
> +	/*
> +	 * For endpoint decoders hosted on CXL memory devices that
> +	 * support the sanitize operation, make sure sanitize is not in-flight.
> +	 */
> +	if (is_endpoint_decoder(&cxld->dev)) {
> +		struct cxl_endpoint_decoder *cxled =
> +			to_cxl_endpoint_decoder(&cxld->dev);
> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +		struct cxl_memdev_state *mds =
> +			to_cxl_memdev_state(cxlmd->cxlds);
> +
> +		if (mds && mds->security.sanitize_active) {

I'm curious why this check does not need to hold the mds->mbox_mutex?  Or
how this may be protected by the cxl_dpa_rwsem?

Ira

[snip]

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Dan Williams]

Ira Weiny wrote:
> Dan Williams wrote:
> > The sanitize operation is destructive and the expectation is that the
> > device is unmapped while in progress. The current implementation does a
> > lockless check for decoders being active, but then does nothing to
> > prevent decoders from racing to be committed. Introduce state tracking
> > to resolve this race.
> > 
> > This incidentally cleans up unpriveleged userspace from triggering mmio
> > read cycles by spinning on reading the 'securiry/state' attribute. Which
> > at a minimum is a waste since the kernel state machine can cache the
> > completion result.
> > 
> > Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> > original implementation, but an export was never required.
> > 
> > Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> 
> [snip]
> 
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> >  		return -EBUSY;
> >  	}
> >  
> > +	/*
> > +	 * For endpoint decoders hosted on CXL memory devices that
> > +	 * support the sanitize operation, make sure sanitize is not in-flight.
> > +	 */
> > +	if (is_endpoint_decoder(&cxld->dev)) {
> > +		struct cxl_endpoint_decoder *cxled =
> > +			to_cxl_endpoint_decoder(&cxld->dev);
> > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +		struct cxl_memdev_state *mds =
> > +			to_cxl_memdev_state(cxlmd->cxlds);
> > +
> > +		if (mds && mds->security.sanitize_active) {
> 
> I'm curious why this check does not need to hold the mds->mbox_mutex?  Or
> how this may be protected by the cxl_dpa_rwsem?

...because cxl_decoder_commit() knows it is holding the cxl_dpa_rwsem for
write which means that sanitize_active can not transition from false to
true in cxl_mem_sanitize().

It does mean that in-flight completions may race, but that's a benign race
where whatever triggered cxl_decoder_commit() is already in the position of
needing to wait for cxl_mbox_sanitize_work() to fire.

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Dan Williams]

Dan Williams wrote:
> Ira Weiny wrote:
> > Dan Williams wrote:
> > > The sanitize operation is destructive and the expectation is that the
> > > device is unmapped while in progress. The current implementation does a
> > > lockless check for decoders being active, but then does nothing to
> > > prevent decoders from racing to be committed. Introduce state tracking
> > > to resolve this race.
> > > 
> > > This incidentally cleans up unpriveleged userspace from triggering mmio
> > > read cycles by spinning on reading the 'securiry/state' attribute. Which
> > > at a minimum is a waste since the kernel state machine can cache the
> > > completion result.
> > > 
> > > Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> > > original implementation, but an export was never required.
> > > 
> > > Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> > > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > 
> > [snip]
> > 
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> > >  		return -EBUSY;
> > >  	}
> > >  
> > > +	/*
> > > +	 * For endpoint decoders hosted on CXL memory devices that
> > > +	 * support the sanitize operation, make sure sanitize is not in-flight.
> > > +	 */
> > > +	if (is_endpoint_decoder(&cxld->dev)) {
> > > +		struct cxl_endpoint_decoder *cxled =
> > > +			to_cxl_endpoint_decoder(&cxld->dev);
> > > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > > +		struct cxl_memdev_state *mds =
> > > +			to_cxl_memdev_state(cxlmd->cxlds);
> > > +
> > > +		if (mds && mds->security.sanitize_active) {
> > 
> > I'm curious why this check does not need to hold the mds->mbox_mutex?  Or
> > how this may be protected by the cxl_dpa_rwsem?
> 
> ...because cxl_decoder_commit() knows it is holding the cxl_dpa_rwsem for

s/cxl_dpa_rwsem/cxl_region_rwsem/

> write which means that sanitize_active can not transition from false to
> true in cxl_mem_sanitize().
> 
> It does mean that in-flight completions may race, but that's a benign race
> where whatever triggered cxl_decoder_commit() is already in the position of
> needing to wait for cxl_mbox_sanitize_work() to fire.

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Jonathan Cameron]

On Fri, 06 Oct 2023 00:26:39 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The sanitize operation is destructive and the expectation is that the
> device is unmapped while in progress. The current implementation does a
> lockless check for decoders being active, but then does nothing to
> prevent decoders from racing to be committed. Introduce state tracking
> to resolve this race.
> 
> This incidentally cleans up unpriveleged userspace from triggering mmio
> read cycles by spinning on reading the 'securiry/state' attribute. Which

Needs a spell check.  security

> at a minimum is a waste since the kernel state machine can cache the
> completion result.
> 
> Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> original implementation, but an export was never required.
> 
> Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 06 Oct 2023 00:26:39 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The sanitize operation is destructive and the expectation is that the
> > device is unmapped while in progress. The current implementation does a
> > lockless check for decoders being active, but then does nothing to
> > prevent decoders from racing to be committed. Introduce state tracking
> > to resolve this race.
> > 
> > This incidentally cleans up unpriveleged userspace from triggering mmio
> > read cycles by spinning on reading the 'securiry/state' attribute. Which
> 
> Needs a spell check.  security

Fixed.

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Jonathan Cameron]

On Mon, 9 Oct 2023 11:36:25 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 06 Oct 2023 00:26:39 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > The sanitize operation is destructive and the expectation is that the
> > > device is unmapped while in progress. The current implementation does a
> > > lockless check for decoders being active, but then does nothing to
> > > prevent decoders from racing to be committed. Introduce state tracking
> > > to resolve this race.
> > > 
> > > This incidentally cleans up unpriveleged userspace from triggering mmio
> > > read cycles by spinning on reading the 'securiry/state' attribute. Which  
> > 
> > Needs a spell check.  security  
> 
> Fixed.
> 

Given you answered Ira's question which I was waiting for,
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Davidlohr Bueso]

On Fri, 06 Oct 2023, Dan Williams wrote:

>The sanitize operation is destructive and the expectation is that the
>device is unmapped while in progress. The current implementation does a
>lockless check for decoders being active, but then does nothing to
>prevent decoders from racing to be committed. Introduce state tracking
>to resolve this race.
>
>This incidentally cleans up unpriveleged userspace from triggering mmio
>read cycles by spinning on reading the 'securiry/state' attribute. Which
>at a minimum is a waste since the kernel state machine can cache the
>completion result.
>
>Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
>original implementation, but an export was never required.
>
>Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
>Cc: Davidlohr Bueso <dave@stgolabs.net>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> The sanitize operation is destructive and the expectation is that the
> device is unmapped while in progress. The current implementation does a
> lockless check for decoders being active, but then does nothing to
> prevent decoders from racing to be committed. Introduce state tracking
> to resolve this race.
> 
> This incidentally cleans up unpriveleged userspace from triggering mmio

s/unpriveleged/unprivileged/

> read cycles by spinning on reading the 'securiry/state' attribute. Which
> at a minimum is a waste since the kernel state machine can cache the
> completion result.
> 
> Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> original implementation, but an export was never required.
> 
> Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/core.h   |    1 +
>  drivers/cxl/core/hdm.c    |   19 ++++++++++++++++
>  drivers/cxl/core/mbox.c   |   55 +++++++++++++++++++++++++++++++++------------
>  drivers/cxl/core/memdev.c |   43 +++++++++++++----------------------
>  drivers/cxl/core/port.c   |    6 +++++
>  drivers/cxl/core/region.c |    6 -----
>  drivers/cxl/cxlmem.h      |    4 ++-
>  drivers/cxl/pci.c         |    5 ++++
>  8 files changed, 90 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 45e7e044cf4a..8e5f3d84311e 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -75,6 +75,7 @@ resource_size_t __rcrb_to_component(struct device *dev,
>  				    enum cxl_rcrb which);
>  
>  extern struct rw_semaphore cxl_dpa_rwsem;
> +extern struct rw_semaphore cxl_region_rwsem;
>  
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 4449b34a80cc..506c9e14cdf9 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  		return -EBUSY;
>  	}
>  
> +	/*
> +	 * For endpoint decoders hosted on CXL memory devices that
> +	 * support the sanitize operation, make sure sanitize is not in-flight.
> +	 */
> +	if (is_endpoint_decoder(&cxld->dev)) {
> +		struct cxl_endpoint_decoder *cxled =
> +			to_cxl_endpoint_decoder(&cxld->dev);
> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +		struct cxl_memdev_state *mds =
> +			to_cxl_memdev_state(cxlmd->cxlds);
> +
> +		if (mds && mds->security.sanitize_active) {
> +			dev_dbg(&cxlmd->dev,
> +				"attempted to commit %s during sanitize\n",
> +				dev_name(&cxld->dev));
> +			return -EBUSY;
> +		}
> +	}
> +
>  	down_read(&cxl_dpa_rwsem);
>  	/* common decoder settings */
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 4df4f614f490..67aec57cc12f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1125,20 +1125,7 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_dev_state_identify, CXL);
>  
> -/**
> - * cxl_mem_sanitize() - Send a sanitization command to the device.
> - * @mds: The device data for the operation
> - * @cmd: The specific sanitization command opcode
> - *
> - * Return: 0 if the command was executed successfully, regardless of
> - * whether or not the actual security operation is done in the background,
> - * such as for the Sanitize case.
> - * Error return values can be the result of the mailbox command, -EINVAL
> - * when security requirements are not met or invalid contexts.
> - *
> - * See CXL 3.0 @8.2.9.8.5.1 Sanitize and @8.2.9.8.5.2 Secure Erase.
> - */
> -int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
> +static int __cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
>  {
>  	int rc;
>  	u32 sec_out = 0;
> @@ -1183,7 +1170,45 @@ int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
> +
> +
> +/**
> + * cxl_mem_sanitize() - Send a sanitization command to the device.
> + * @mds: The device for the operation
> + * @cmd: The specific sanitization command opcode
> + *
> + * Return: 0 if the command was executed successfully, regardless of
> + * whether or not the actual security operation is done in the background,
> + * such as for the Sanitize case.
> + * Error return values can be the result of the mailbox command, -EINVAL
> + * when security requirements are not met or invalid contexts, or -EBUSY
> + * if the sanitize operation is already in flight.
> + *
> + * See CXL 3.0 @8.2.9.8.5.1 Sanitize and @8.2.9.8.5.2 Secure Erase.
> + */
> +int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> +{
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +	struct cxl_port  *endpoint;
> +	int rc;
> +
> +	/* synchronize with cxl_mem_probe() and decoder write operations */
> +	device_lock(&cxlmd->dev);
> +	endpoint = cxlmd->endpoint;
> +	down_read(&cxl_region_rwsem);
> +	/*
> +	 * Require an endpoint to be safe otherwise the driver can not
> +	 * be sure that the device is unmapped.
> +	 */
> +	if (endpoint && endpoint->commit_end == -1)
> +		rc = __cxl_mem_sanitize(mds, cmd);
> +	else
> +		rc = -EBUSY;
> +	up_read(&cxl_region_rwsem);
> +	device_unlock(&cxlmd->dev);
> +
> +	return rc;
> +}
>  
>  static int add_dpa_res(struct device *dev, struct resource *parent,
>  		       struct resource *res, resource_size_t start,
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 4c2e24a1a89c..a02061028b71 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -125,13 +125,16 @@ static ssize_t security_state_show(struct device *dev,
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	u64 reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> -	u32 pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, reg);
> -	u16 cmd = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
>  	unsigned long state = mds->security.state;
> +	int rc = 0;
>  
> -	if (cmd == CXL_MBOX_OP_SANITIZE && pct != 100)
> -		return sysfs_emit(buf, "sanitize\n");
> +	/* sync with latest submission state */
> +	mutex_lock(&mds->mbox_mutex);
> +	if (mds->security.sanitize_active)
> +		rc = sysfs_emit(buf, "sanitize\n");
> +	mutex_unlock(&mds->mbox_mutex);
> +	if (rc)
> +		return rc;
>  
>  	if (!(state & CXL_PMEM_SEC_STATE_USER_PASS_SET))
>  		return sysfs_emit(buf, "disabled\n");
> @@ -152,24 +155,17 @@ static ssize_t security_sanitize_store(struct device *dev,
>  				       const char *buf, size_t len)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> -	struct cxl_port *port = cxlmd->endpoint;
>  	bool sanitize;
>  	ssize_t rc;
>  
>  	if (kstrtobool(buf, &sanitize) || !sanitize)
>  		return -EINVAL;
>  
> -	if (!port || !is_cxl_endpoint(port))
> -		return -EINVAL;
> -
> -	/* ensure no regions are mapped to this memdev */
> -	if (port->commit_end != -1)
> -		return -EBUSY;
> -
> -	rc = cxl_mem_sanitize(mds, CXL_MBOX_OP_SANITIZE);
> +	rc = cxl_mem_sanitize(cxlmd, CXL_MBOX_OP_SANITIZE);
> +	if (rc)
> +		return rc;
>  
> -	return rc ? rc : len;
> +	return len;
>  }
>  static struct device_attribute dev_attr_security_sanitize =
>  	__ATTR(sanitize, 0200, NULL, security_sanitize_store);
> @@ -179,24 +175,17 @@ static ssize_t security_erase_store(struct device *dev,
>  				    const char *buf, size_t len)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> -	struct cxl_port *port = cxlmd->endpoint;
>  	ssize_t rc;
>  	bool erase;
>  
>  	if (kstrtobool(buf, &erase) || !erase)
>  		return -EINVAL;
>  
> -	if (!port || !is_cxl_endpoint(port))
> -		return -EINVAL;
> -
> -	/* ensure no regions are mapped to this memdev */
> -	if (port->commit_end != -1)
> -		return -EBUSY;
> -
> -	rc = cxl_mem_sanitize(mds, CXL_MBOX_OP_SECURE_ERASE);
> +	rc = cxl_mem_sanitize(cxlmd, CXL_MBOX_OP_SECURE_ERASE);
> +	if (rc)
> +		return rc;
>  
> -	return rc ? rc : len;
> +	return len;
>  }
>  static struct device_attribute dev_attr_security_erase =
>  	__ATTR(erase, 0200, NULL, security_erase_store);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 7ca01a834e18..5ba606c6e03f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -28,6 +28,12 @@
>   * instantiated by the core.
>   */
>  
> +/*
> + * All changes to the interleave configuration occur with this lock held
> + * for write.
> + */
> +DECLARE_RWSEM(cxl_region_rwsem);
> +
>  static DEFINE_IDA(cxl_port_ida);
>  static DEFINE_XARRAY(cxl_root_buses);
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..d74bf1b664b6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -28,12 +28,6 @@
>   * 3. Decoder targets
>   */
>  
> -/*
> - * All changes to the interleave configuration occur with this lock held
> - * for write.
> - */
> -static DECLARE_RWSEM(cxl_region_rwsem);
> -
>  static struct cxl_region *to_cxl_region(struct device *dev);
>  
>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index fbdee1d63717..6933bc20e76b 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -364,6 +364,7 @@ struct cxl_fw_state {
>   * @state: state of last security operation
>   * @enabled_cmds: All security commands enabled in the CEL
>   * @poll_tmo_secs: polling timeout
> + * @sanitize_active: sanitize completion pending
>   * @poll_dwork: polling work item
>   * @sanitize_node: sanitation sysfs file to notify
>   */
> @@ -371,6 +372,7 @@ struct cxl_security_state {
>  	unsigned long state;
>  	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
>  	int poll_tmo_secs;
> +	bool sanitize_active;
>  	struct delayed_work poll_dwork;
>  	struct kernfs_node *sanitize_node;
>  };
> @@ -884,7 +886,7 @@ static inline void cxl_mem_active_dec(void)
>  }
>  #endif
>  
> -int cxl_mem_sanitize(struct cxl_memdev_state *mds, u16 cmd);
> +int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd);
>  
>  struct cxl_hdm {
>  	struct cxl_component_regs regs;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 9955871e9ec1..06fafe59c054 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -154,6 +154,7 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
>  		mds->security.poll_tmo_secs = 0;
>  		if (mds->security.sanitize_node)
>  			sysfs_notify_dirent(mds->security.sanitize_node);
> +		mds->security.sanitize_active = false;
>  
>  		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
>  	} else {
> @@ -292,9 +293,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
>  		 * and allow userspace to poll(2) for completion.
>  		 */
>  		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
> +			if (mds->security.sanitize_active)
> +				return -EBUSY;
> +
>  			/* give first timeout a second */
>  			timeout = 1;
>  			mds->security.poll_tmo_secs = timeout;
> +			mds->security.sanitize_active = true;
>  			schedule_delayed_work(&mds->security.poll_dwork,
>  					      timeout * HZ);
>  			dev_dbg(dev, "Sanitization operation started\n");
> 
> 

[PATCH v3 08/10] cxl/mem: Fix shutdown order

[Dan Williams]

Ira reports that removing cxl_mock_mem causes a crash with the following
trace:

 BUG: kernel NULL pointer dereference, address: 0000000000000044
 [..]
 RIP: 0010:cxl_region_decode_reset+0x7f/0x180 [cxl_core]
 [..]
 Call Trace:
  <TASK>
  cxl_region_detach+0xe8/0x210 [cxl_core]
  cxl_decoder_kill_region+0x27/0x40 [cxl_core]
  cxld_unregister+0x29/0x40 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  device_unregister+0x13/0x60
  devm_release_action+0x4d/0x90
  ? __pfx_unregister_port+0x10/0x10 [cxl_core]
  delete_endpoint+0x121/0x130 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  ? lock_release+0x142/0x290
  cdev_device_del+0x15/0x50
  cxl_memdev_unregister+0x54/0x70 [cxl_core]

This crash is due to the clearing out the cxl_memdev's driver context
(@cxlds) before the subsystem is done with it. This is ultimately due to
the region(s), that this memdev is a member, being torn down and expecting
to be able to de-reference @cxlds, like here:

static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
...
                if (cxlds->rcd)
                        goto endpoint_reset;
...

Fix it by keeping the driver context valid until memdev-device
unregistration, and subsequently the entire stack of related
dependencies, unwinds.

Fixes: 9cc238c7a526 ("cxl/pci: Introduce cdevm_file_operations")
Reported-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index a02061028b71..fed9573cf355 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -559,8 +559,8 @@ static void cxl_memdev_unregister(void *_cxlmd)
 	struct cxl_memdev *cxlmd = _cxlmd;
 	struct device *dev = &cxlmd->dev;
 
-	cxl_memdev_shutdown(dev);
 	cdev_device_del(&cxlmd->cdev, dev);
+	cxl_memdev_shutdown(dev);
 	put_device(dev);
 }
 

[PATCH v3 09/10] tools/testing/cxl: Make cxl_memdev_state available to other command emulation

[Dan Williams]

Move @mds out of the event specific 'struct mock_event_store' and into
the base 'struct cxl_mockmem_data' directly. This is in preparation for
enabling cxl_test to exercise the notifier flow for 'sanitize' operation
completion.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/mem.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 68118c37f0b5..ab311b59899a 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -133,7 +133,6 @@ struct mock_event_log {
 };
 
 struct mock_event_store {
-	struct cxl_memdev_state *mds;
 	struct mock_event_log mock_logs[CXL_EVENT_TYPE_MAX];
 	u32 ev_status;
 };
@@ -150,6 +149,7 @@ struct cxl_mockmem_data {
 	int user_limit;
 	int master_limit;
 	struct mock_event_store mes;
+	struct cxl_memdev_state *mds;
 	u8 event_buf[SZ_4K];
 	u64 timestamp;
 };
@@ -326,7 +326,7 @@ static void cxl_mock_event_trigger(struct device *dev)
 			event_reset_log(log);
 	}
 
-	cxl_mem_get_event_records(mes->mds, mes->ev_status);
+	cxl_mem_get_event_records(mdata->mds, mes->ev_status);
 }
 
 struct cxl_event_record_raw maint_needed = {
@@ -1415,6 +1415,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (IS_ERR(mds))
 		return PTR_ERR(mds);
 
+	mdata->mds = mds;
 	mds->mbox_send = cxl_mock_mbox_send;
 	mds->payload_size = SZ_4K;
 	mds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
@@ -1447,7 +1448,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	mdata->mes.mds = mds;
 	cxl_mock_add_event_logs(&mdata->mes);
 
 	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);

Re: [PATCH v3 09/10] tools/testing/cxl: Make cxl_memdev_state available to other command emulation

[Ira Weiny]

Dan Williams wrote:
> Move @mds out of the event specific 'struct mock_event_store' and into
> the base 'struct cxl_mockmem_data' directly. This is in preparation for
> enabling cxl_test to exercise the notifier flow for 'sanitize' operation
> completion.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH v3 09/10] tools/testing/cxl: Make cxl_memdev_state available to other command emulation

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> Move @mds out of the event specific 'struct mock_event_store' and into
> the base 'struct cxl_mockmem_data' directly. This is in preparation for
> enabling cxl_test to exercise the notifier flow for 'sanitize' operation
> completion.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tools/testing/cxl/test/mem.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 68118c37f0b5..ab311b59899a 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -133,7 +133,6 @@ struct mock_event_log {
>  };
>  
>  struct mock_event_store {
> -	struct cxl_memdev_state *mds;
>  	struct mock_event_log mock_logs[CXL_EVENT_TYPE_MAX];
>  	u32 ev_status;
>  };
> @@ -150,6 +149,7 @@ struct cxl_mockmem_data {
>  	int user_limit;
>  	int master_limit;
>  	struct mock_event_store mes;
> +	struct cxl_memdev_state *mds;
>  	u8 event_buf[SZ_4K];
>  	u64 timestamp;
>  };
> @@ -326,7 +326,7 @@ static void cxl_mock_event_trigger(struct device *dev)
>  			event_reset_log(log);
>  	}
>  
> -	cxl_mem_get_event_records(mes->mds, mes->ev_status);
> +	cxl_mem_get_event_records(mdata->mds, mes->ev_status);
>  }
>  
>  struct cxl_event_record_raw maint_needed = {
> @@ -1415,6 +1415,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (IS_ERR(mds))
>  		return PTR_ERR(mds);
>  
> +	mdata->mds = mds;
>  	mds->mbox_send = cxl_mock_mbox_send;
>  	mds->payload_size = SZ_4K;
>  	mds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
> @@ -1447,7 +1448,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> -	mdata->mes.mds = mds;
>  	cxl_mock_add_event_logs(&mdata->mes);
>  
>  	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
> 
> 

[PATCH v3 10/10] tools/testing/cxl: Add 'sanitize notifier' support

[Dan Williams]

Allow for cxl_test regression of the sanitize notifier. Reuse the core
setup infrastructure, and trigger notifications upon any sanitize
submission with a programmable notification delay.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/mem.c |   68 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index ab311b59899a..76bdb1ac5816 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -89,6 +89,12 @@ static struct cxl_cel_entry mock_cel[] = {
 		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_COLD_RESET) |
 				      EFFECT(CONF_CHANGE_IMMEDIATE)),
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_SANITIZE),
+		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE) |
+				      EFFECT(SECURITY_CHANGE_IMMEDIATE) |
+				      EFFECT(BACKGROUND_OP)),
+	},
 };
 
 /* See CXL 2.0 Table 181 Get Health Info Output Payload */
@@ -152,6 +158,7 @@ struct cxl_mockmem_data {
 	struct cxl_memdev_state *mds;
 	u8 event_buf[SZ_4K];
 	u64 timestamp;
+	unsigned long sanitize_timeout;
 };
 
 static struct mock_event_log *event_find_log(struct device *dev, int log_type)
@@ -567,9 +574,26 @@ static int mock_partition_info(struct cxl_mbox_cmd *cmd)
 	return 0;
 }
 
+void cxl_mockmem_sanitize_work(struct work_struct *work)
+{
+	struct cxl_memdev_state *mds =
+		container_of(work, typeof(*mds), security.poll_dwork.work);
+
+	mutex_lock(&mds->mbox_mutex);
+	if (mds->security.sanitize_node)
+		sysfs_notify_dirent(mds->security.sanitize_node);
+	mds->security.sanitize_active = false;
+	mutex_unlock(&mds->mbox_mutex);
+
+	dev_dbg(mds->cxlds.dev, "sanitize complete\n");
+}
+
 static int mock_sanitize(struct cxl_mockmem_data *mdata,
 			 struct cxl_mbox_cmd *cmd)
 {
+	struct cxl_memdev_state *mds = mdata->mds;
+	int rc = 0;
+
 	if (cmd->size_in != 0)
 		return -EINVAL;
 
@@ -585,7 +609,16 @@ static int mock_sanitize(struct cxl_mockmem_data *mdata,
 		return -ENXIO;
 	}
 
-	return 0; /* assume less than 2 secs, no bg */
+	mutex_lock(&mds->mbox_mutex);
+	if (schedule_delayed_work(&mds->security.poll_dwork,
+				  msecs_to_jiffies(mdata->sanitize_timeout))) {
+		mds->security.sanitize_active = true;
+		dev_dbg(mds->cxlds.dev, "sanitize issued\n");
+	} else
+		rc = -EBUSY;
+	mutex_unlock(&mds->mbox_mutex);
+
+	return rc;
 }
 
 static int mock_secure_erase(struct cxl_mockmem_data *mdata,
@@ -1419,6 +1452,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	mds->mbox_send = cxl_mock_mbox_send;
 	mds->payload_size = SZ_4K;
 	mds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
+	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mockmem_sanitize_work);
 
 	cxlds = &mds->cxlds;
 	cxlds->serial = pdev->id;
@@ -1458,6 +1492,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
+	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
+	if (rc)
+		return rc;
+
 	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
 
 	return 0;
@@ -1526,10 +1564,38 @@ static ssize_t fw_buf_checksum_show(struct device *dev,
 
 static DEVICE_ATTR_RO(fw_buf_checksum);
 
+static ssize_t sanitize_timeout_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%lu\n", mdata->sanitize_timeout);
+}
+
+static ssize_t sanitize_timeout_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
+	unsigned long val;
+	int rc;
+
+	rc = kstrtoul(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	mdata->sanitize_timeout = val;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(sanitize_timeout);
+
 static struct attribute *cxl_mock_mem_attrs[] = {
 	&dev_attr_security_lock.attr,
 	&dev_attr_event_trigger.attr,
 	&dev_attr_fw_buf_checksum.attr,
+	&dev_attr_sanitize_timeout.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(cxl_mock_mem);

Re: [PATCH v3 10/10] tools/testing/cxl: Add 'sanitize notifier' support

[Ira Weiny]

Dan Williams wrote:
> Allow for cxl_test regression of the sanitize notifier. Reuse the core
> setup infrastructure, and trigger notifications upon any sanitize
> submission with a programmable notification delay.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH v3 10/10] tools/testing/cxl: Add 'sanitize notifier' support

[Dave Jiang]

On 10/6/23 00:26, Dan Williams wrote:
> Allow for cxl_test regression of the sanitize notifier. Reuse the core
> setup infrastructure, and trigger notifications upon any sanitize
> submission with a programmable notification delay.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tools/testing/cxl/test/mem.c |   68 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index ab311b59899a..76bdb1ac5816 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -89,6 +89,12 @@ static struct cxl_cel_entry mock_cel[] = {
>  		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_COLD_RESET) |
>  				      EFFECT(CONF_CHANGE_IMMEDIATE)),
>  	},
> +	{
> +		.opcode = cpu_to_le16(CXL_MBOX_OP_SANITIZE),
> +		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE) |
> +				      EFFECT(SECURITY_CHANGE_IMMEDIATE) |
> +				      EFFECT(BACKGROUND_OP)),
> +	},
>  };
>  
>  /* See CXL 2.0 Table 181 Get Health Info Output Payload */
> @@ -152,6 +158,7 @@ struct cxl_mockmem_data {
>  	struct cxl_memdev_state *mds;
>  	u8 event_buf[SZ_4K];
>  	u64 timestamp;
> +	unsigned long sanitize_timeout;
>  };
>  
>  static struct mock_event_log *event_find_log(struct device *dev, int log_type)
> @@ -567,9 +574,26 @@ static int mock_partition_info(struct cxl_mbox_cmd *cmd)
>  	return 0;
>  }
>  
> +void cxl_mockmem_sanitize_work(struct work_struct *work)
> +{
> +	struct cxl_memdev_state *mds =
> +		container_of(work, typeof(*mds), security.poll_dwork.work);
> +
> +	mutex_lock(&mds->mbox_mutex);
> +	if (mds->security.sanitize_node)
> +		sysfs_notify_dirent(mds->security.sanitize_node);
> +	mds->security.sanitize_active = false;
> +	mutex_unlock(&mds->mbox_mutex);
> +
> +	dev_dbg(mds->cxlds.dev, "sanitize complete\n");
> +}
> +
>  static int mock_sanitize(struct cxl_mockmem_data *mdata,
>  			 struct cxl_mbox_cmd *cmd)
>  {
> +	struct cxl_memdev_state *mds = mdata->mds;
> +	int rc = 0;
> +
>  	if (cmd->size_in != 0)
>  		return -EINVAL;
>  
> @@ -585,7 +609,16 @@ static int mock_sanitize(struct cxl_mockmem_data *mdata,
>  		return -ENXIO;
>  	}
>  
> -	return 0; /* assume less than 2 secs, no bg */
> +	mutex_lock(&mds->mbox_mutex);
> +	if (schedule_delayed_work(&mds->security.poll_dwork,
> +				  msecs_to_jiffies(mdata->sanitize_timeout))) {
> +		mds->security.sanitize_active = true;
> +		dev_dbg(mds->cxlds.dev, "sanitize issued\n");
> +	} else
> +		rc = -EBUSY;
> +	mutex_unlock(&mds->mbox_mutex);
> +
> +	return rc;
>  }
>  
>  static int mock_secure_erase(struct cxl_mockmem_data *mdata,
> @@ -1419,6 +1452,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	mds->mbox_send = cxl_mock_mbox_send;
>  	mds->payload_size = SZ_4K;
>  	mds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
> +	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mockmem_sanitize_work);
>  
>  	cxlds = &mds->cxlds;
>  	cxlds->serial = pdev->id;
> @@ -1458,6 +1492,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> +	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
> +	if (rc)
> +		return rc;
> +
>  	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
>  
>  	return 0;
> @@ -1526,10 +1564,38 @@ static ssize_t fw_buf_checksum_show(struct device *dev,
>  
>  static DEVICE_ATTR_RO(fw_buf_checksum);
>  
> +static ssize_t sanitize_timeout_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%lu\n", mdata->sanitize_timeout);
> +}
> +
> +static ssize_t sanitize_timeout_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int rc;
> +
> +	rc = kstrtoul(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	mdata->sanitize_timeout = val;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(sanitize_timeout);
> +
>  static struct attribute *cxl_mock_mem_attrs[] = {
>  	&dev_attr_security_lock.attr,
>  	&dev_attr_event_trigger.attr,
>  	&dev_attr_fw_buf_checksum.attr,
> +	&dev_attr_sanitize_timeout.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(cxl_mock_mem);
> 
>