Hi On Tue, Mar 2, 2021 at 4:43 PM Andre Przywara <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> > -- > 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
