[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Michael Nazzareno Trimarchi]

Hi

On Tue, Mar 2, 2021 at 4:43 PM Andre Przywara <andre.przywara at arm.com> wrote:
>
> At the moment the nvme_get_features() and nvme_set_features() functions
> carry a (somewhat misleading) comment about missing cache maintenance.
>
> As it turns out, nvme_get_features() has no caller at all in the tree,
> and nvme_set_features' only user doesn't use a DMA buffer.
>
> Mention that in the comment, and leave some breadcrumbs for the future,
> should those functions attract more users.
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
> Hi,
>
> just extending the comments as this caused some confusion lately, and to
> keep the pointer to the NVMe spec I checked.
>
> Cheers,
> Andre
>
>  drivers/nvme/nvme.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index 5d6331ad346..3ff3d5de08e 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -481,6 +481,7 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
>                       dma_addr_t dma_addr, u32 *result)
>  {
>         struct nvme_command c;
> +       int ret;
>
>         memset(&c, 0, sizeof(c));
>         c.features.opcode = nvme_admin_get_features;
> @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
>         c.features.prp1 = cpu_to_le64(dma_addr);
>         c.features.fid = cpu_to_le32(fid);
>
> +       ret = nvme_submit_admin_cmd(dev, &c, result);
> +
>         /*
> -        * TODO: add cache invalidate operation when the size of
> -        * the DMA buffer is known
> +        * TODO: Add some cache invalidation when a DMA buffer is involved
> +        * in the request, here and before the command gets submitted. The
> +        * buffer size varies by feature, also some features use a different
> +        * field in the command packet to hold the buffer address.
> +        * Section 5.21.1 (Set Features command) in the NVMe specification
> +        * details the buffer requirements for each feature.
> +        *
> +        * At the moment there is no user of this function.
>          */
>
> -       return nvme_submit_admin_cmd(dev, &c, result);
> +       return ret;
>  }
>
>  int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
> @@ -508,8 +517,14 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
>         c.features.dword11 = cpu_to_le32(dword11);
>
>         /*
> -        * TODO: add cache flush operation when the size of
> -        * the DMA buffer is known
> +        * TODO: Add a cache clean (aka flush) operation when a DMA buffer is
> +        * involved in the request. The buffer size varies by feature, also
> +        * some features use a different field in the command packet to hold
> +        * the buffer address. Section 5.21.1 (Set Features command) in the
> +        * NVMe specification details the buffer requirements for each
> +        * feature.
> +        * At the moment the only user of this function is not using
> +        * any DMA buffer at all.
>          */
>
>         return nvme_submit_admin_cmd(dev, &c, result);

Reviewed-By: Michael Trimarchi <michael at amarulasolutions.com>
> --
> 2.17.5
>


-- 
Michael Nazzareno Trimarchi
Amarula Solutions BV
COO Co-Founder
Cruquiuskade 47 Amsterdam 1018 AM NL
T. +31(0)851119172
M. +39(0)3479132170
[`as] https://www.amarulasolutions.com