Re: [PATCH v3 16/16] fwctl/cxl: Add documentation to FWCTL CXL

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> Add policy and operational documentation for FWCTL CXL.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Add a sentence on how entries are filtered. (Jonathan)
> ---
>  .../userspace-api/fwctl/fwctl-cxl.rst         | 137 ++++++++++++++++++
>  Documentation/userspace-api/fwctl/index.rst   |   1 +
>  MAINTAINERS                                   |   1 +
>  3 files changed, 139 insertions(+)
>  create mode 100644 Documentation/userspace-api/fwctl/fwctl-cxl.rst
> 
> diff --git a/Documentation/userspace-api/fwctl/fwctl-cxl.rst b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
> new file mode 100644
> index 000000000000..81c694f6a44d
> --- /dev/null
> +++ b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
> @@ -0,0 +1,137 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +================
> +fwctl cxl driver
> +================
> +
> +:Author: Dave Jiang
[..]

Thanks for the verb tense changes!

> +
> +Code example of a Get Feature
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. code-block:: c

Nice, thanks for the example, it finally allows me to put a finger on a
naming nit below...

> +
> +        static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature *feat_ctx,
> +                                                  const uint32_t expected_data)
> +        {
> +                struct cxl_mbox_get_feat_in feat_in __attribute__((aligned(16))) = {0};
> +                struct fwctl_rpc_cxl in __attribute__((aligned(16))) = {0};
> +                struct fwctl_rpc_cxl_out *out;
> +                struct fwctl_rpc rpc = {0};
> +                size_t out_size;
> +                uint32_t val;
> +                void *data;
> +                int rc;
> +
> +                uuid_copy(feat_in.uuid, feat_ctx->uuid);
> +                feat_in.count = feat_ctx->get_size;
> +
> +                out_size = sizeof(*out) + feat_in.count;
> +                out = aligned_alloc(16, out_size);
> +                if (!out)
> +                        return -ENOMEM;
> +
> +                memset(out, 0, out_size);
> +                data = out->payload;
> +
> +                in.command_id = CXL_FEATURE_ID_GET_FEATURE;

So this should probably be CXL_FWCTL_GET_FEATURE, right? There's no such
thing as a feature command id to userspace only FWCTL command ids that
translate to one of Get Suppported, Get, and Set Feature(s), and the
"id" is implied.

> +                in.op_size = sizeof(feat_in);
> +                in.in_payload = (uint64_t)(uint64_t *)&feat_in;
> +
> +                rpc.size = sizeof(rpc);
> +                rpc.scope = FWCTL_RPC_CONFIGURATION;
> +                rpc.in_len = sizeof(in);
> +                rpc.out_len = out_size;
> +                rpc.in = (uint64_t)(uint64_t *)&in;
> +                rpc.out = (uint64_t)(uint64_t *)out;
> +
> +                rc = send_command(fd, &rpc, out);
> +                if (rc)
> +                        goto out;
> +
> +                val = le32toh(*(__le32 *)data);
> +                if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
> +                        rc = -ENXIO;
> +                        goto out;
> +                }
> +
> +        out:
> +                free(out);
> +                return rc;
> +        }
> +
> +It is recommended to take a look at CXL CLI test directory

Passive -> imperative:

s/It is recommended to take/Take/

Other than that, this looks good!