Re: [PATCHv5 7/8] nvme-pci: create common sgl unmapping helper

[Christoph Hellwig <hch@lst.de>]

On Fri, Aug 08, 2025 at 08:58:25AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This can be reused by metadata sgls once that starts using the blk-mq
> dma api.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/pci.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)

> +static void __nvme_free_sgls(struct device *dma_dev, struct nvme_sgl_desc *sge,
> +		struct nvme_sgl_desc *sg_list, enum dma_data_direction dir)
> +{
> +	unsigned int len = le32_to_cpu(sge->length);
> +	unsigned int i, nr_entries;
> +
> +	if (sge->type == (NVME_SGL_FMT_DATA_DESC << 4)) {
> +		dma_unmap_page(dma_dev, le64_to_cpu(sge->addr), len, dir);
> +		return;
> +	}
> +
> +	nr_entries = len / sizeof(*sg_list);
> +	for (i = 0; i < nr_entries; i++)


We can probably just do away with the nr_entries variable, the compiler
is not going to recompute this for every loop ieration.


>  {
>  	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>  	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
>  	struct device *dma_dev = nvmeq->dev->dev;
>  	struct nvme_sgl_desc *sg_list = iod->descriptors[0];
>  	enum dma_data_direction dir = rq_dma_dir(req);
>  
> +	__nvme_free_sgls(dma_dev, sge, sg_list, dir);

Shouldn't we move the calculation of nvmeq, dma_dev and dir into
__nvme_free_sgls as they are going to be the same for data and metadata.
And then maybe rename it to __nvme_free_sgls and just opencode the
iod->descriptors[0] and &iod->cmd.common.dptr.sgl dereferences in
nvme_unmap_data?