linux-kernel - Re: [PATCH 04/16] PCI/P2PDMA: Refactor pci_p2pdma_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce04d398-e4a1-b3aa-2a4e-b1b868470144@nvidia.com>
Date:   Sun, 2 May 2021 13:41:45 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Logan Gunthorpe <logang@...tatee.com>,
        <linux-kernel@...r.kernel.org>, <linux-nvme@...ts.infradead.org>,
        <linux-block@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <linux-mm@...ck.org>, <iommu@...ts.linux-foundation.org>
CC:     Stephen Bates <sbates@...thlin.com>,
        Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Christian König <christian.koenig@....com>,
        Don Dutile <ddutile@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Minturn Dave B <dave.b.minturn@...el.com>,
        Jason Ekstrand <jason@...kstrand.net>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Xiong Jianxin <jianxin.xiong@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 04/16] PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take
 pagmap and device

On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
> All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a
> struct device (of the client doing the DMA transfer). Thus move the
> conversion to struct pci_devs for the provider and client into this
> function.

Actually, this is the wrong direction to go! All of these pre-existing
pci_*() functions have a small problem already: they are dealing with
struct device, instead of struct pci_dev. And so refactoring should be
pushing the conversion to pci_dev *up* the calling stack, not lower as
the patch here proposes.

Also, there is no improvement in clarity by passing in (pgmap, dev)
instead of the previous (provider, client). Now you have to do more type
checking in the leaf function, which is another indication of a problem.

Let's go that direction, please? Just convert to pci_dev much higher in
the calling stack, and you'll find that everything fits together better.
And it's OK to pass in extra params if that turns out to be necessary,
after all.

thanks,
-- 
John Hubbard
NVIDIA

> 
> Signed-off-by: Logan Gunthorpe <logang@...tatee.com>
> ---
>   drivers/pci/p2pdma.c | 29 +++++++++++------------------
>   1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 2574a062a255..bcb1a6d6119d 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -822,14 +822,21 @@ void pci_p2pmem_publish(struct pci_dev *pdev, bool publish)
>   }
>   EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
>   
> -static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider,
> -						    struct pci_dev *client)
> +static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct dev_pagemap *pgmap,
> +						    struct device *dev)
>   {
> +	struct pci_dev *provider = to_p2p_pgmap(pgmap)->provider;
>   	enum pci_p2pdma_map_type ret;
> +	struct pci_dev *client;
>   
>   	if (!provider->p2pdma)
>   		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
>   
> +	if (!dev_is_pci(dev))
> +		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
> +
> +	client = to_pci_dev(dev);
> +
>   	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
>   				  map_types_idx(client)));
>   	if (ret != PCI_P2PDMA_MAP_UNKNOWN)
> @@ -871,14 +878,8 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>   {
>   	struct pci_p2pdma_pagemap *p2p_pgmap =
>   		to_p2p_pgmap(sg_page(sg)->pgmap);
> -	struct pci_dev *client;
> -
> -	if (WARN_ON_ONCE(!dev_is_pci(dev)))
> -		return 0;
>   
> -	client = to_pci_dev(dev);
> -
> -	switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) {
> +	switch (pci_p2pdma_map_type(sg_page(sg)->pgmap, dev)) {
>   	case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE:
>   		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
>   	case PCI_P2PDMA_MAP_BUS_ADDR:
> @@ -901,17 +902,9 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
>   void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   		int nents, enum dma_data_direction dir, unsigned long attrs)
>   {
> -	struct pci_p2pdma_pagemap *p2p_pgmap =
> -		to_p2p_pgmap(sg_page(sg)->pgmap);
>   	enum pci_p2pdma_map_type map_type;
> -	struct pci_dev *client;
> -
> -	if (WARN_ON_ONCE(!dev_is_pci(dev)))
> -		return;
> -
> -	client = to_pci_dev(dev);
>   
> -	map_type = pci_p2pdma_map_type(p2p_pgmap->provider, client);
> +	map_type = pci_p2pdma_map_type(sg_page(sg)->pgmap, dev);
>   
>   	if (map_type == PCI_P2PDMA_MAP_THRU_HOST_BRIDGE)
>   		dma_unmap_sg_attrs(dev, sg, nents, dir, attrs);
>