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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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;