Re: [PATCH 12/16] nvme-pci: Check DMA ops when indicating support for PCI P2PDMA

[John Hubbard <jhubbard@nvidia.com>]

On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
> Introduce a supports_pci_p2pdma() operation in nvme_ctrl_ops to
> replace the fixed NVME_F_PCI_P2PDMA flag such that the dma_map_ops
> flags can be checked for PCI P2PDMA support.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>   drivers/nvme/host/core.c |  3 ++-
>   drivers/nvme/host/nvme.h |  2 +-
>   drivers/nvme/host/pci.c  | 11 +++++++++--
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0896e21642be..223419454516 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3907,7 +3907,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>   		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
>   
>   	blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue);
> -	if (ctrl->ops->flags & NVME_F_PCI_P2PDMA)
> +	if (ctrl->ops->supports_pci_p2pdma &&
> +	    ctrl->ops->supports_pci_p2pdma(ctrl))

This is a little excessive, as I suspected. How about providing a
default .supports_pci_p2pdma routine that returns false, so that
the op is always available (non-null)? By "default", maybe that
means either requiring an init_the_ops_struct() routine to be
used, and/or checking all the users of struct nvme_ctrl_ops.

Another idea: maybe you don't really need a bool .supports_pci_p2pdma()
routine at all, because the existing .flags really is about right.
You just need the flags to be filled in dynamically. So, do that
during nvme_pci setup/init time: that's when this module would call
dma_pci_p2pdma_supported().

Actually, I think that second idea simplifies things quite a
bit, but only if it's possible. I haven't worked through the
startup order of calls in nvme_pci.

thanks,
-- 
John Hubbard
NVIDIA

>   		blk_queue_flag_set(QUEUE_FLAG_PCI_P2PDMA, ns->queue);
>   
>   	ns->queue->queuedata = ns;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 07b34175c6ce..9c04df982d2c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -473,7 +473,6 @@ struct nvme_ctrl_ops {
>   	unsigned int flags;
>   #define NVME_F_FABRICS			(1 << 0)
>   #define NVME_F_METADATA_SUPPORTED	(1 << 1)
> -#define NVME_F_PCI_P2PDMA		(1 << 2)
>   	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
>   	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
>   	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> @@ -481,6 +480,7 @@ struct nvme_ctrl_ops {
>   	void (*submit_async_event)(struct nvme_ctrl *ctrl);
>   	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
>   	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
> +	bool (*supports_pci_p2pdma)(struct nvme_ctrl *ctrl);
>   };
>   
>   #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7249ae74f71f..14f092973792 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2759,17 +2759,24 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
>   	return snprintf(buf, size, "%s\n", dev_name(&pdev->dev));
>   }
>   
> +static bool nvme_pci_supports_pci_p2pdma(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_dev *dev = to_nvme_dev(ctrl);
> +
> +	return dma_pci_p2pdma_supported(dev->dev);
> +}
> +
>   static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
>   	.name			= "pcie",
>   	.module			= THIS_MODULE,
> -	.flags			= NVME_F_METADATA_SUPPORTED |
> -				  NVME_F_PCI_P2PDMA,
> +	.flags			= NVME_F_METADATA_SUPPORTED,
>   	.reg_read32		= nvme_pci_reg_read32,
>   	.reg_write32		= nvme_pci_reg_write32,
>   	.reg_read64		= nvme_pci_reg_read64,
>   	.free_ctrl		= nvme_pci_free_ctrl,
>   	.submit_async_event	= nvme_pci_submit_async_event,
>   	.get_address		= nvme_pci_get_address,
> +	.supports_pci_p2pdma	= nvme_pci_supports_pci_p2pdma,
>   };
>   
>   static int nvme_dev_map(struct nvme_dev *dev)
>