[RFC 0/4] cxl: Support for mailbox background abort operation

[RFC 0/4] cxl: Support for mailbox background abort operation

[Ravis OpenSrc]

This patchset introduces the Request Background Abort operation as outlined
in CXL 3.1, section 8.2.9.1.5. It ensures that only background operations
supporting request background abort are allowed. A default timeout of
5 seconds is specified for operations where a timeout is not explicitly
defined. If this timeout expires, a Request Abort command is issued to
terminate the ongoing background operation.

This patchset is based on suggestion from Dan Williams, as referenced here:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

(1) Patch 1: Enables only those background mailbox commands
    that also support request abort through their CEL.
(2) Patch 2: A default timeout of 5 secs for mailbox commands
    that do not have an explicitly mentioned timeout.
(3) Patch 3: Adds support for sending an abort when a mailbox
    background operation times out.
(4) Patch 4: Adds support for the Populate Log command,
    a background operation that can potentially support abort.

Ravi Shankar (4):
  cxl: Enable mailbox ops with background only if request abort
    operation is supported.
  cxl: Add default timeout for bg mailbox commands
  cxl: Abort background operation in case of timeout
  cxl/mbox: Add Populate Log support

 drivers/cxl/core/mbox.c      | 13 +++++++++++--
 drivers/cxl/cxlmem.h         | 18 ++++++++++++++++++
 drivers/cxl/pci.c            | 24 ++++++++++++++++++++++++
 include/uapi/linux/cxl_mem.h |  1 +
 4 files changed, 54 insertions(+), 2 deletions(-)

-- 
2.43.0

    

[RFC PATCH 2/4] cxl: Add default timeout for bg mailbox commands

[Ravis OpenSrc]

Allows 5s wait when no timeout parameter is explicitly mentioned.

It is useful for mailbox commands to be executed in background
when initiated from userspace.

Link:
https://lore.kernel.org/linux-mm/20240215123410.00003b8c@Huawei.com/T/

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 188412d45e0d..d5d6142f6aa3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -41,6 +41,10 @@
 /* CXL 2.0 - 8.2.8.4 */
 #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
 
+/* Default timeout for background operations */
+#define CXL_BG_POLL_CNT                        5
+#define CXL_BG_POLL_INTERVAL_MS                1000
+
 /*
  * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
  * dictate how long to wait for the mailbox to become ready. The new
@@ -317,6 +321,15 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
                 dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
                         mbox_cmd->opcode);
 
+               /*
+                * Add a default timeout of 5 seconds when background operation
+                * starts but no timeout is specified.
+                */
+               if (!mbox_cmd->poll_interval_ms) {
+                       mbox_cmd->poll_interval_ms = CXL_BG_POLL_INTERVAL_MS;
+                       mbox_cmd->poll_count = CXL_BG_POLL_CNT;
+               }
+
                 timeout = mbox_cmd->poll_interval_ms;
                 for (i = 0; i < mbox_cmd->poll_count; i++) {
                         if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
-- 
2.43.0

    

[RFC PATCH v2 2/4] cxl: Add default timeout for bg mailbox commands

[Ravis OpenSrc]

Allows 5s wait when no timeout parameter is explicitly mentioned.

It is useful for mailbox commands to be executed in background
when initiated from userspace.

Link:
https://lore.kernel.org/linux-mm/20240215123410.00003b8c@Huawei.com/T/

Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 188412d45e0d..d5d6142f6aa3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -41,6 +41,10 @@
 /* CXL 2.0 - 8.2.8.4 */
 #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
 
+/* Default timeout for background operations */
+#define CXL_BG_POLL_CNT                        5
+#define CXL_BG_POLL_INTERVAL_MS                1000
+
 /*
  * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
  * dictate how long to wait for the mailbox to become ready. The new
@@ -317,6 +321,15 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
                 dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
                         mbox_cmd->opcode);
 
+               /*
+                * Add a default timeout of 5 seconds when background operation
+                * starts but no timeout is specified.
+                */
+               if (!mbox_cmd->poll_interval_ms) {
+                       mbox_cmd->poll_interval_ms = CXL_BG_POLL_INTERVAL_MS;
+                       mbox_cmd->poll_count = CXL_BG_POLL_CNT;
+               }
+
                 timeout = mbox_cmd->poll_interval_ms;
                 for (i = 0; i < mbox_cmd->poll_count; i++) {
                         if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
-- 
2.43.0

    

Re: [RFC PATCH v2 2/4] cxl: Add default timeout for bg mailbox commands

[Jonathan Cameron]

On Wed, 16 Oct 2024 04:59:58 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:

> Allows 5s wait when no timeout parameter is explicitly mentioned.
> 
> It is useful for mailbox commands to be executed in background
> when initiated from userspace.
> 
> Link:
> https://lore.kernel.org/linux-mm/20240215123410.00003b8c@Huawei.com/T/
> 
> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
Other than patch formatting, this seems reasonable to me.

Jonathan

> ---
>  drivers/cxl/pci.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 188412d45e0d..d5d6142f6aa3 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -41,6 +41,10 @@
>  /* CXL 2.0 - 8.2.8.4 */
>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>  
> +/* Default timeout for background operations */
> +#define CXL_BG_POLL_CNT                        5
> +#define CXL_BG_POLL_INTERVAL_MS                1000
> +
>  /*
>   * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
>   * dictate how long to wait for the mailbox to become ready. The new
> @@ -317,6 +321,15 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>                  dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
>                          mbox_cmd->opcode);
>  
> +               /*
> +                * Add a default timeout of 5 seconds when background operation
> +                * starts but no timeout is specified.
> +                */
> +               if (!mbox_cmd->poll_interval_ms) {
> +                       mbox_cmd->poll_interval_ms = CXL_BG_POLL_INTERVAL_MS;
> +                       mbox_cmd->poll_count = CXL_BG_POLL_CNT;
> +               }
> +
>                  timeout = mbox_cmd->poll_interval_ms;
>                  for (i = 0; i < mbox_cmd->poll_count; i++) {
>                          if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,

RE: [EXT] Re: [RFC PATCH v2 2/4] cxl: Add default timeout for bg mailbox commands

[Srinivasulu Opensrc]

Micron Confidential



Micron Confidential
>-----Original Message-----
>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>Sent: Thursday, October 17, 2024 9:02 PM
>To: Ravis OpenSrc <Ravis.OpenSrc@micron.com>
>Cc: linux-cxl@vger.kernel.org; dan.j.williams@intel.com; dave.jiang@intel.com;
>Srinivasulu Opensrc <sthanneeru.opensrc@micron.com>; john@jagalactic.com;
>Ajay Joshi <ajayjoshi@micron.com>
>Subject: [EXT] Re: [RFC PATCH v2 2/4] cxl: Add default timeout for bg mailbox
>commands
>
>CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless you
>recognize the sender and were expecting this message.
>
>
>On Wed, 16 Oct 2024 04:59:58 +0000
>Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:
>
>> Allows 5s wait when no timeout parameter is explicitly mentioned.
>>
>> It is useful for mailbox commands to be executed in background
>> when initiated from userspace.
>>
>> Link:
>>
>https://lore.kernel/
>.org%2Flinux-
>mm%2F20240215123410.00003b8c%40Huawei.com%2FT%2F&data=05%7C02%7
>Csthanneeru.opensrc%40micron.com%7Cb92d2c58a0e14335108508dceec0e378
>%7Cf38a5ecd28134862b11bac1d563c806f%7C0%7C0%7C638647759553538421
>%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
>I6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=3mRf9MkO9GO2L%2F%
>2B7Vz9h92vVpVmX2AX%2Ba2unp5mBzKU%3D&reserved=0
>>
>> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
>> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>

This patch mostly of resend of previous version from https://lore.kernel.org/linux-mm/20240215123410.00003b8c@Huawei.com/T/
Hence, need to add original author's s/o.

Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>


>Other than patch formatting, this seems reasonable to me.
>
>Jonathan
>
>> ---
>>  drivers/cxl/pci.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 188412d45e0d..d5d6142f6aa3 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -41,6 +41,10 @@
>>  /* CXL 2.0 - 8.2.8.4 */
>>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>>
>> +/* Default timeout for background operations */
>> +#define CXL_BG_POLL_CNT                        5
>> +#define CXL_BG_POLL_INTERVAL_MS                1000
>> +
>>  /*
>>   * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
>>   * dictate how long to wait for the mailbox to become ready. The new
>> @@ -317,6 +321,15 @@ static int __cxl_pci_mbox_send_cmd(struct
>cxl_mailbox *cxl_mbox,
>>                  dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
>>                          mbox_cmd->opcode);
>>
>> +               /*
>> +                * Add a default timeout of 5 seconds when background operation
>> +                * starts but no timeout is specified.
>> +                */
>> +               if (!mbox_cmd->poll_interval_ms) {
>> +                       mbox_cmd->poll_interval_ms = CXL_BG_POLL_INTERVAL_MS;
>> +                       mbox_cmd->poll_count = CXL_BG_POLL_CNT;
>> +               }
>> +
>>                  timeout = mbox_cmd->poll_interval_ms;
>>                  for (i = 0; i < mbox_cmd->poll_count; i++) {
>>                          if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,

[RFC PATCH 4/4] cxl/mbox: Add Populate Log support

[Ravis OpenSrc]

Adding UAPI support for
 CXL r3.1 8.2.9.5.5 Populate Log.

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c      | 1 +
 drivers/cxl/cxlmem.h         | 1 +
 include/uapi/linux/cxl_mem.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 8c0144913b9e..48c2fc8b4bcd 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -59,6 +59,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
         CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
         CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
         CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
+       CXL_CMD(POPULATE_LOG, 0x10, 0, 0),
         CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
         CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
         CXL_CMD(GET_ALERT_CONFIG, 0, 0x10, 0),
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 808fb8712145..6320d8cd3ca3 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -530,6 +530,7 @@ enum cxl_opcode {
         CXL_MBOX_OP_GET_LOG             = 0x0401,
         CXL_MBOX_OP_GET_LOG_CAPS        = 0x0402,
         CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
+       CXL_MBOX_OP_POPULATE_LOG        = 0x0404,
         CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
         CXL_MBOX_OP_IDENTIFY            = 0x4000,
         CXL_MBOX_OP_GET_PARTITION_INFO  = 0x4100,
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c6c0fe27495d..040ca37046ed 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -50,6 +50,7 @@
         ___C(GET_LOG_CAPS, "Get Log Capabilities"),                       \
         ___C(CLEAR_LOG, "Clear Log"),                                     \
         ___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),         \
+       ___C(POPULATE_LOG, "Populate Log"),       \
         ___C(MAX, "invalid / last command")
 
 #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
-- 
2.43.0

    

[RFC PATCH v2 4/4] cxl/mbox: Add Populate Log support

[Ravis OpenSrc]

Adding UAPI support for
 CXL r3.1 8.2.9.5.5 Populate Log.

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c      | 1 +
 drivers/cxl/cxlmem.h         | 1 +
 include/uapi/linux/cxl_mem.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 8c0144913b9e..48c2fc8b4bcd 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -59,6 +59,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
         CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
         CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
         CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
+       CXL_CMD(POPULATE_LOG, 0x10, 0, 0),
         CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
         CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
         CXL_CMD(GET_ALERT_CONFIG, 0, 0x10, 0),
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 808fb8712145..6320d8cd3ca3 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -530,6 +530,7 @@ enum cxl_opcode {
         CXL_MBOX_OP_GET_LOG             = 0x0401,
         CXL_MBOX_OP_GET_LOG_CAPS        = 0x0402,
         CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
+       CXL_MBOX_OP_POPULATE_LOG        = 0x0404,
         CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
         CXL_MBOX_OP_IDENTIFY            = 0x4000,
         CXL_MBOX_OP_GET_PARTITION_INFO  = 0x4100,
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c6c0fe27495d..040ca37046ed 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -50,6 +50,7 @@
         ___C(GET_LOG_CAPS, "Get Log Capabilities"),                       \
         ___C(CLEAR_LOG, "Clear Log"),                                     \
         ___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),         \
+       ___C(POPULATE_LOG, "Populate Log"),       \
         ___C(MAX, "invalid / last command")
 
 #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
-- 
2.43.0

        

Re: [RFC PATCH v2 4/4] cxl/mbox: Add Populate Log support

[Jonathan Cameron]

On Wed, 16 Oct 2024 05:00:02 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:

> Adding UAPI support for
>  CXL r3.1 8.2.9.5.5 Populate Log.
> 
> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
hmm. Exciting threading ;) Definitely want to sort
that out for future postings.

Other than formatting this looks ok to me.

Jonathan

> ---
>  drivers/cxl/core/mbox.c      | 1 +
>  drivers/cxl/cxlmem.h         | 1 +
>  include/uapi/linux/cxl_mem.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 8c0144913b9e..48c2fc8b4bcd 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -59,6 +59,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>          CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
>          CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
>          CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
> +       CXL_CMD(POPULATE_LOG, 0x10, 0, 0),
>          CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
>          CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
>          CXL_CMD(GET_ALERT_CONFIG, 0, 0x10, 0),
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 808fb8712145..6320d8cd3ca3 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -530,6 +530,7 @@ enum cxl_opcode {
>          CXL_MBOX_OP_GET_LOG             = 0x0401,
>          CXL_MBOX_OP_GET_LOG_CAPS        = 0x0402,
>          CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
> +       CXL_MBOX_OP_POPULATE_LOG        = 0x0404,
>          CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
>          CXL_MBOX_OP_IDENTIFY            = 0x4000,
>          CXL_MBOX_OP_GET_PARTITION_INFO  = 0x4100,
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index c6c0fe27495d..040ca37046ed 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -50,6 +50,7 @@
>          ___C(GET_LOG_CAPS, "Get Log Capabilities"),                       \
>          ___C(CLEAR_LOG, "Clear Log"),                                     \
>          ___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),         \
> +       ___C(POPULATE_LOG, "Populate Log"),       \
>          ___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a

[RFC v2 0/4] cxl: Support for mailbox background abort operation

[Ravis OpenSrc]

This patchset introduces the Request Background Abort operation as outlined
in CXL 3.1, section 8.2.9.1.5. It ensures that only background operations
supporting request background abort are allowed. A default timeout of
5 seconds is specified for operations where a timeout is not explicitly
defined. If this timeout expires, a Request Abort command is issued to
terminate the ongoing background operation.

This patchset is based on suggestion from Dan Williams, as referenced here:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

(1) Patch 1: Enables only those background mailbox commands
    that also support request abort through their CEL.
(2) Patch 2: A default timeout of 5 secs for mailbox commands
    that do not have an explicitly mentioned timeout.
(3) Patch 3: Adds support for sending an abort when a mailbox
    background operation times out.
(4) Patch 4: Adds support for the Populate Log command,
    a background operation that can potentially support abort.

v2:
* Added missing signature tags

v1:
https://lore.kernel.org/linux-cxl/c3312b1ce2374fb28a899b0bdb01c6a4@micron.com/T/#t

Ravi Shankar (4):
  cxl: Enable mailbox ops with background only if request abort
    operation is supported.
  cxl: Add default timeout for bg mailbox commands
  cxl: Abort background operation in case of timeout
  cxl/mbox: Add Populate Log support

 drivers/cxl/core/mbox.c      | 13 +++++++++++--
 drivers/cxl/cxlmem.h         | 18 ++++++++++++++++++
 drivers/cxl/pci.c            | 24 ++++++++++++++++++++++++
 include/uapi/linux/cxl_mem.h |  1 +
 4 files changed, 54 insertions(+), 2 deletions(-)

-- 
2.43.0

    

[RFC PATCH 1/4] cxl: Enable mailbox ops with background only if request abort operation is supported.

[Ravis OpenSrc]

Enabling mailbox commands only if background operation and
request abort cancellation are both supported.

This check is to allow user space commands to initiate
background commands responsibly while complying with any
default background timeout implemented in kernel.

Link:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c | 12 ++++++++++--
 drivers/cxl/cxlmem.h    | 16 ++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 5175138c4fb7..8c0144913b9e 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -733,12 +733,20 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 
         for (i = 0; i < cel_entries; i++) {
                 u16 opcode = le16_to_cpu(cel_entry[i].opcode);
+               u16 effect = le16_to_cpu(cel_entry[i].effect);
                 struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
                 int enabled = 0;
 
                 if (cmd) {
-                       set_bit(cmd->info.id, mds->enabled_cmds);
-                       enabled++;
+                       /*
+                        * For background operation commands, enable only if
+                        * Request abort background operation is supported.
+                        */
+                       if (!(effect & CXL_CEL_FLAG_BACKGROUND_OPERATION) ||
+                          (effect & CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED)) {
+                               set_bit(cmd->info.id, mds->enabled_cmds);
+                               enabled++;
+                       }
                 }
 
                 if (cxl_is_poison_command(opcode)) {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 2a25d1957ddb..d8c0894797ac 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -579,6 +579,22 @@ struct cxl_cel_entry {
         __le16 effect;
 } __packed;
 
+/*
+ * CEL Entry Effects
+ * CXL rev 3.1 Section 8.2.9.5.2.1; Table 8-75
+ */
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_RESET BIT(0)
+#define        CXL_CEL_FLAG_CFG_CHANGE_IMMEDIATE BIT(1)
+#define        CXL_CEL_FLAG_DATA_CHANGE_IMMEDIATE BIT(2)
+#define        CXL_CEL_FLAG_POLICY_CHANGE_IMMEDIATE BIT(3)
+#define        CXL_CEL_FLAG_LOG_CHANGE_IMMEDIATE BIT(4)
+#define        CXL_CEL_FLAG_SECURITY_CHANGE BIT(5)
+#define        CXL_CEL_FLAG_BACKGROUND_OPERATION BIT(6)
+#define        CXL_CEL_FLAG_SECONDARY_MAILBOX_SUPPORTED BIT(7)
+#define        CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED BIT(8)
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CONV_RESET BIT(10)
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CXL_RESET BIT(11)
+
 struct cxl_mbox_get_log {
         uuid_t uuid;
         __le32 offset;
-- 
2.43.0

    

[RFC PATCH v2 1/4] cxl: Enable mailbox ops with background only if request abort operation is supported.

[Ravis OpenSrc]

Enabling mailbox commands only if background operation and
request abort cancellation are both supported.

This check is to allow user space commands to initiate
background commands responsibly while complying with any
default background timeout implemented in kernel.

Link:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c | 12 ++++++++++--
 drivers/cxl/cxlmem.h    | 16 ++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 5175138c4fb7..8c0144913b9e 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -733,12 +733,20 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 
         for (i = 0; i < cel_entries; i++) {
                 u16 opcode = le16_to_cpu(cel_entry[i].opcode);
+               u16 effect = le16_to_cpu(cel_entry[i].effect);
                 struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
                 int enabled = 0;
 
                 if (cmd) {
-                       set_bit(cmd->info.id, mds->enabled_cmds);
-                       enabled++;
+                       /*
+                        * For background operation commands, enable only if
+                        * Request abort background operation is supported.
+                        */
+                       if (!(effect & CXL_CEL_FLAG_BACKGROUND_OPERATION) ||
+                          (effect & CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED)) {
+                               set_bit(cmd->info.id, mds->enabled_cmds);
+                               enabled++;
+                       }
                 }
 
                 if (cxl_is_poison_command(opcode)) {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 2a25d1957ddb..d8c0894797ac 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -579,6 +579,22 @@ struct cxl_cel_entry {
         __le16 effect;
 } __packed;
 
+/*
+ * CEL Entry Effects
+ * CXL rev 3.1 Section 8.2.9.5.2.1; Table 8-75
+ */
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_RESET BIT(0)
+#define        CXL_CEL_FLAG_CFG_CHANGE_IMMEDIATE BIT(1)
+#define        CXL_CEL_FLAG_DATA_CHANGE_IMMEDIATE BIT(2)
+#define        CXL_CEL_FLAG_POLICY_CHANGE_IMMEDIATE BIT(3)
+#define        CXL_CEL_FLAG_LOG_CHANGE_IMMEDIATE BIT(4)
+#define        CXL_CEL_FLAG_SECURITY_CHANGE BIT(5)
+#define        CXL_CEL_FLAG_BACKGROUND_OPERATION BIT(6)
+#define        CXL_CEL_FLAG_SECONDARY_MAILBOX_SUPPORTED BIT(7)
+#define        CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED BIT(8)
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CONV_RESET BIT(10)
+#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CXL_RESET BIT(11)
+
 struct cxl_mbox_get_log {
         uuid_t uuid;
         __le32 offset;
-- 
2.43.0

    

Re: [RFC PATCH v2 1/4] cxl: Enable mailbox ops with background only if request abort operation is supported.

[Jonathan Cameron]

On Wed, 16 Oct 2024 04:59:56 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:
Hi Ravi,

Change the author in git with
git commit --amend --author=Ravi Shankar <ravis.opensrc@micron.com>

Then when you do git format-email or similar ti
will add an explicit
From: Ravi Shankar <ravis.opensrc@micron.com>
To the top of the patch description that git will then pick up
as the author.

> Enabling mailbox commands only if background operation and
> request abort cancellation are both supported.

Key here is the userspace access point.  We let the kernel
use these commands if it wants to as it can pick up the
pieces.

> 
> This check is to allow user space commands to initiate
> background commands responsibly while complying with any
> default background timeout implemented in kernel.
> 
> Link:
> https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/
This is a tag just like the ones below.
So all on one line and no blank lines before the tag block.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>

What was Ajay's role in this?
Maybe a Co-developed tag is appropriate?

> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
> ---
>  drivers/cxl/core/mbox.c | 12 ++++++++++--
>  drivers/cxl/cxlmem.h    | 16 ++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 5175138c4fb7..8c0144913b9e 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -733,12 +733,20 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  
>          for (i = 0; i < cel_entries; i++) {
>                  u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> +               u16 effect = le16_to_cpu(cel_entry[i].effect);
Your email client has I think mangled this to use spaces instead of tabs.

Given the fun that is corporate email systems, maybe take a look at
using b4 and the webproxy that allows patches to be sent to the mailing
list without involving any company email servers :)

>                  struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>                  int enabled = 0;
>  
>                  if (cmd) {
> -                       set_bit(cmd->info.id, mds->enabled_cmds);
> -                       enabled++;
> +                       /*
> +                        * For background operation commands, enable only if
> +                        * Request abort background operation is supported.
> +                        */
> +                       if (!(effect & CXL_CEL_FLAG_BACKGROUND_OPERATION) ||
> +                          (effect & CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED)) {
> +                               set_bit(cmd->info.id, mds->enabled_cmds);
> +                               enabled++;
> +                       }
>                  }
>  
>                  if (cxl_is_poison_command(opcode)) {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 2a25d1957ddb..d8c0894797ac 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -579,6 +579,22 @@ struct cxl_cel_entry {
>          __le16 effect;
>  } __packed;
>  
> +/*
> + * CEL Entry Effects
> + * CXL rev 3.1 Section 8.2.9.5.2.1; Table 8-75
> + */
> +#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_RESET BIT(0)

AFTER_COLD_RESET (subtle but we should include the COLD)

> +#define        CXL_CEL_FLAG_CFG_CHANGE_IMMEDIATE BIT(1)
> +#define        CXL_CEL_FLAG_DATA_CHANGE_IMMEDIATE BIT(2)
> +#define        CXL_CEL_FLAG_POLICY_CHANGE_IMMEDIATE BIT(3)
> +#define        CXL_CEL_FLAG_LOG_CHANGE_IMMEDIATE BIT(4)
> +#define        CXL_CEL_FLAG_SECURITY_CHANGE BIT(5)
> +#define        CXL_CEL_FLAG_BACKGROUND_OPERATION BIT(6)
> +#define        CXL_CEL_FLAG_SECONDARY_MAILBOX_SUPPORTED BIT(7)
> +#define        CXL_CEL_FLAG_REQ_ABORT_BACKGROUND_SUPPORTED BIT(8)
Should define the fun of bit 9 which is kind of a cheeky version
bit hiding in here that says if we should pay attention to next tow
or have no idea.

> +#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CONV_RESET BIT(10)
> +#define        CXL_CEL_FLAG_CFG_CHANGE_AFTER_CXL_RESET BIT(11)
> +
>  struct cxl_mbox_get_log {
>          uuid_t uuid;
>          __le32 offset;

[RFC PATCH 3/4] cxl: Abort background operation in case of timeout

[Ravis OpenSrc]

Adding support for aborting timed out background operations

CXL r3.1 8.2.9.1.5 Request Abort Background Operation.

If the status of a mailbox command is identified as timedout,
an abort background operation request is sent to the device.

Link:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/cxlmem.h |  1 +
 drivers/cxl/pci.c    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index d8c0894797ac..808fb8712145 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
 enum cxl_opcode {
         CXL_MBOX_OP_INVALID             = 0x0000,
         CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
+       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
         CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
         CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
         CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d5d6142f6aa3..95c1f329bca2 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 
         mutex_lock_io(&cxl_mbox->mbox_mutex);
         rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
+       if (rc == -ETIMEDOUT &&
+               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
+               struct cxl_mbox_cmd abort_cmd = {
+                       .opcode = CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION
+               };
+
+               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
+               if (!rc)
+                       rc = -ECANCELED;
+       }
+
         mutex_unlock(&cxl_mbox->mbox_mutex);
 
         return rc;
-- 
2.43.0

    

[RFC PATCH v2 3/4] cxl: Abort background operation in case of timeout

[Ravis OpenSrc]

Adding support for aborting timed out background operations

CXL r3.1 8.2.9.1.5 Request Abort Background Operation.

If the status of a mailbox command is identified as timedout,
an abort background operation request is sent to the device.

Link:
https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
---
 drivers/cxl/cxlmem.h |  1 +
 drivers/cxl/pci.c    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index d8c0894797ac..808fb8712145 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
 enum cxl_opcode {
         CXL_MBOX_OP_INVALID             = 0x0000,
         CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
+       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
         CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
         CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
         CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d5d6142f6aa3..95c1f329bca2 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 
         mutex_lock_io(&cxl_mbox->mbox_mutex);
         rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
+       if (rc == -ETIMEDOUT &&
+               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
+               struct cxl_mbox_cmd abort_cmd = {
+                       .opcode = CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION
+               };
+
+               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
+               if (!rc)
+                       rc = -ECANCELED;
+       }
+
         mutex_unlock(&cxl_mbox->mbox_mutex);
 
         return rc;
-- 
2.43.0

    

Re: [RFC PATCH v2 3/4] cxl: Abort background operation in case of timeout

[Jonathan Cameron]

On Wed, 16 Oct 2024 05:00:00 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:

> Adding support for aborting timed out background operations
> 
> CXL r3.1 8.2.9.1.5 Request Abort Background Operation.
> 
> If the status of a mailbox command is identified as timedout,
> an abort background operation request is sent to the device.
> 
> Link:
> https://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>

I was kind of expecting that we'd do this more on an ondemand
basis. So if nothing is going on, the command can have ages.
If the kernel wants to access the device and it has had a
reasonable amount of time we abort.

I'm not against this simpler approach though if it turns out
to be good enough for likely use cases.

My worry is error paths crossing with a slow command where
we want to find out what went wrong as quick as possible
Is 5 seconds ok for that?  Maybe not.

Jonathan

> ---
>  drivers/cxl/cxlmem.h |  1 +
>  drivers/cxl/pci.c    | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index d8c0894797ac..808fb8712145 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
>  enum cxl_opcode {
>          CXL_MBOX_OP_INVALID             = 0x0000,
>          CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
> +       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
>          CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
>          CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
>          CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index d5d6142f6aa3..95c1f329bca2 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  
>          mutex_lock_io(&cxl_mbox->mbox_mutex);
>          rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
> +       if (rc == -ETIMEDOUT &&
> +               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {

align cmd just after ( - that is c is under the r of rc
After fixing the tabs thing.

> +               struct cxl_mbox_cmd abort_cmd = {
> +                       .opcode = CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION
> +               };
> +
> +               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
> +               if (!rc)
> +                       rc = -ECANCELED;
> +       }
> +
>          mutex_unlock(&cxl_mbox->mbox_mutex);
>  
>          return rc;

Re: [RFC PATCH v2 3/4] cxl: Abort background operation in case of timeout

[Ravis OpenSrc]

>On Thu, 17 Oct 2024 09:06:00 +0530
>Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>> 
>> On Wed, 16 Oct 2024 05:00:00 +0000
>> Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:
>> 
>>> Adding support for aborting timed out background operations
>>>
>>> CXL r3.1 8.2.9.1.5 Request Abort Background Operation.
>>>
>>> If the status of a mailbox command is identified as timedout, an abort
>>> background operation request is sent to the device.
>>>
>>> Link:
>>>
>>https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
>>> .kernel.org%2Flinux-cxl%2F66035c2e8ba17_770232948b%40dwillia2-
>>xfh.jf.i
>>>
>>ntel.com.notmuch%2F&data=05%7C02%7Cajayjoshi%40micron.com%7C0d
>>d742041c
>>>
>>ba47ec0dbd08dceec16a02%7Cf38a5ecd28134862b11bac1d563c806f%7C0
>>%7C0%7C63
>>>
>>8647761722049771%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
>>MDAiLCJQIjoiV
>>>
>>2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1vVK
>>PaqWSp6qT
>>> FmwsYF1z%2F7%2FdfEeVFD9cA0g1VVo00c%3D&reserved=0
>>>
>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
>>> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
>>
>I was kind of expecting that we'd do this more on an ondemand basis. 

By saying on demand, do you envision a different implementation for
abort like a sysfs interface or module param?

> So if nothing is going on, the command can have ages.

I did not quite get the comment about the ages.

>If the kernel wants to access the device and it has had a reasonable amount of
>time we abort.
>I'm not against this simpler approach though if it turns out to be good enough
>for likely use cases.
> 
>My worry is error paths crossing with a slow command where we want to find
>out what went wrong as quick as possible Is 5 seconds ok for that?  Maybe
> not.
Do you feel we need to check for "aborted" status along with background percentage,
to avoid waiting for 5 seconds for cases when the command has already aborted.

Would something like below make sense, if not let us know your thoughts.
+static bool cxl_mbox_background_aborted(struct cxl_dev_state *cxlds)
+{
+       u64 reg;
+       u16 ret_code;
+
+       reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+       ret_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK, reg);
+       if (ret_code == CXL_MBOX_CMD_RC_ABORT)
+               return true;
+
+       return false;
+}
+
 static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
 {
        u64 reg;
@@ -316,11 +329,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
                timeout = mbox_cmd->poll_interval_ms;
                for (i = 0; i < mbox_cmd->poll_count; i++) {
                        if (rcuwait_wait_event_timeout(&mds->mbox_wait,
-                                      cxl_mbox_background_complete(cxlds),
+                                      (cxl_mbox_background_complete(cxlds) |
+                                          cxl_mbox_background_aborted(cxlds)),
                                       TASK_UNINTERRUPTIBLE,
                                       msecs_to_jiffies(timeout)) > 0)
                                break;
                }
+               if (!cxl_mbox_background_aborted(cxlds)) {
+                       dev_err(dev, "aborted waiting for background (%d ms) by device\n",
+                               timeout * mbox_cmd->poll_count);
+                       return -ECANCELED;
+               }
> 
>Jonathan
>> 
>>> ---
>>>  drivers/cxl/cxlmem.h |  1 +
>>>  drivers/cxl/pci.c    | 11 +++++++++++
>>>  2 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>>> d8c0894797ac..808fb8712145 100644
>>> --- a/drivers/cxl/cxlmem.h
>>> +++ b/drivers/cxl/cxlmem.h
>>> @@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
>>> enum cxl_opcode {
>>>          CXL_MBOX_OP_INVALID             = 0x0000,
>>>          CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
>>> +       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
>>>          CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
>>>          CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
>>>          CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102, diff --git
>>> a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
>>> d5d6142f6aa3..95c1f329bca2 100644
>>> --- a/drivers/cxl/pci.c
>>> +++ b/drivers/cxl/pci.c
>>> @@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox
>>> *cxl_mbox,
>>>
>>>          mutex_lock_io(&cxl_mbox->mbox_mutex);
>>>          rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
>>> +       if (rc == -ETIMEDOUT &&
>>> +               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
>>
>>align cmd just after ( - that is c is under the r of rc After fixing the tabs >ting.
>>
>>> +               struct cxl_mbox_cmd abort_cmd = {
>>> +                       .opcode =
>>CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION
>>> +               };
>>> +
>>> +               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
>>> +               if (!rc)
>>> +                       rc = -ECANCELED;
>>> +       }
>>> +
>>>          mutex_unlock(&cxl_mbox->mbox_mutex);
>>>
>>>          return rc;

Re: [RFC PATCH v2 3/4] cxl: Abort background operation in case of timeout

[Jonathan Cameron]

On Fri, 18 Oct 2024 06:39:34 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:

> >On Thu, 17 Oct 2024 09:06:00 +0530
> >Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:  
> >> 
> >> On Wed, 16 Oct 2024 05:00:00 +0000
> >> Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:
> >>   
> >>> Adding support for aborting timed out background operations
> >>>
> >>> CXL r3.1 8.2.9.1.5 Request Abort Background Operation.
> >>>
> >>> If the status of a mailbox command is identified as timedout, an abort
> >>> background operation request is sent to the device.
> >>>
> >>> Link:
> >>>  
> >>https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore  
> >>> .kernel.org%2Flinux-cxl%2F66035c2e8ba17_770232948b%40dwillia2-  
> >>xfh.jf.i  
> >>>  
> >>ntel.com.notmuch%2F&data=05%7C02%7Cajayjoshi%40micron.com%7C0d
> >>d742041c  
> >>>  
> >>ba47ec0dbd08dceec16a02%7Cf38a5ecd28134862b11bac1d563c806f%7C0
> >>%7C0%7C63  
> >>>  
> >>8647761722049771%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> >>MDAiLCJQIjoiV  
> >>>  
> >>2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1vVK
> >>PaqWSp6qT  
> >>> FmwsYF1z%2F7%2FdfEeVFD9cA0g1VVo00c%3D&reserved=0
> >>>
> >>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> >>> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
> >>> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>  
> >>  
> >I was kind of expecting that we'd do this more on an ondemand basis.   
> 
> By saying on demand, do you envision a different implementation for
> abort like a sysfs interface or module param?

No. Abort by the kernel when it wants to do something rather than
on a timeout. It might also make sense ultimately to have a userspace
abort path (if it issued the command) but that is a different isseu.

> 
> > So if nothing is going on, the command can have ages.  
> 
> I did not quite get the comment about the ages.

Long time.  If no one wants to access the device interfaces, we don't mind if
the command takes much longer than 5 seconds.

> 
> >If the kernel wants to access the device and it has had a reasonable amount of
> >time we abort.
> >I'm not against this simpler approach though if it turns out to be good enough
> >for likely use cases.
> > 
> >My worry is error paths crossing with a slow command where we want to find
> >out what went wrong as quick as possible Is 5 seconds ok for that?  Maybe
> > not.  
> Do you feel we need to check for "aborted" status along with background percentage,
> to avoid waiting for 5 seconds for cases when the command has already aborted.

At the moment we don't have anything issuing the abort early than the 5 
seconds so I'm not sure how we'd hit this.

May need it in future though.
> 
> Would something like below make sense, if not let us know your thoughts.
> +static bool cxl_mbox_background_aborted(struct cxl_dev_state *cxlds)
> +{
> +       u64 reg;
> +       u16 ret_code;
> +
> +       reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> +       ret_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK, reg);
> +       if (ret_code == CXL_MBOX_CMD_RC_ABORT)
> +               return true;
> +
> +       return false;

	return ret_code == CXL_MBOX_CMD_RC_ABORT;

> +}
> +
>  static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
>  {
>         u64 reg;
> @@ -316,11 +329,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
>                 timeout = mbox_cmd->poll_interval_ms;
>                 for (i = 0; i < mbox_cmd->poll_count; i++) {
>                         if (rcuwait_wait_event_timeout(&mds->mbox_wait,
> -                                      cxl_mbox_background_complete(cxlds),
> +                                      (cxl_mbox_background_complete(cxlds) |
> +                                          cxl_mbox_background_aborted(cxlds)),
>                                        TASK_UNINTERRUPTIBLE,
>                                        msecs_to_jiffies(timeout)) > 0)
>                                 break;
>                 }
> +               if (!cxl_mbox_background_aborted(cxlds)) {
> +                       dev_err(dev, "aborted waiting for background (%d ms) by device\n",
> +                               timeout * mbox_cmd->poll_count);
> +                       return -ECANCELED;
> +               }
> > 
> >Jonathan  
> >>   
> >>> ---
> >>>  drivers/cxl/cxlmem.h |  1 +
> >>>  drivers/cxl/pci.c    | 11 +++++++++++
> >>>  2 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
> >>> d8c0894797ac..808fb8712145 100644
> >>> --- a/drivers/cxl/cxlmem.h
> >>> +++ b/drivers/cxl/cxlmem.h
> >>> @@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
> >>> enum cxl_opcode {
> >>>          CXL_MBOX_OP_INVALID             = 0x0000,
> >>>          CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
> >>> +       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
> >>>          CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
> >>>          CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
> >>>          CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102, diff --git
> >>> a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> >>> d5d6142f6aa3..95c1f329bca2 100644
> >>> --- a/drivers/cxl/pci.c
> >>> +++ b/drivers/cxl/pci.c
> >>> @@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox
> >>> *cxl_mbox,
> >>>
> >>>          mutex_lock_io(&cxl_mbox->mbox_mutex);
> >>>          rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
> >>> +       if (rc == -ETIMEDOUT &&
> >>> +               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {  
> >>
> >>align cmd just after ( - that is c is under the r of rc After fixing the tabs >ting.
> >>  
> >>> +               struct cxl_mbox_cmd abort_cmd = {
> >>> +                       .opcode =  
> >>CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION  
> >>> +               };
> >>> +
> >>> +               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
> >>> +               if (!rc)
> >>> +                       rc = -ECANCELED;
> >>> +       }
> >>> +
> >>>          mutex_unlock(&cxl_mbox->mbox_mutex);
> >>>
> >>>          return rc;