From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x231.google.com (mail-lf0-x231.google.com. [2a00:1450:4010:c07::231]) by gmr-mx.google.com with ESMTPS id q27si80161lfd.0.2017.12.05.09.48.30 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 05 Dec 2017 09:48:30 -0800 (PST) Received: by mail-lf0-x231.google.com with SMTP id x20so1285079lff.1 for ; Tue, 05 Dec 2017 09:48:30 -0800 (PST) Return-Path: Date: Tue, 5 Dec 2017 20:48:37 +0300 From: Serge Semin Subject: Re: [PATCH v2 02/15] NTB: Set dma mask and dma coherent mask to NTB devices Message-ID: <20171205174837.GC1701@mobilestation> References: <20171203191736.3399-1-fancer.lancer@gmail.com> <20171203191736.3399-3-fancer.lancer@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Jon Mason Cc: Dave Jiang , "Hubbe, Allen" , "S-k, Shyam-sundar" , "Yu, Xiangliang" , Gary R Hook , Sergey.Semin@t-platforms.ru, linux-ntb , linux-kernel List-ID: On Tue, Dec 05, 2017 at 11:51:08AM -0500, Jon Mason wrote: > On Sun, Dec 3, 2017 at 2:17 PM, Serge Semin wrote: > > The dma_mask and dma_coherent_mask fields of the NTB struct device > > weren't initialized in hardware drivers. In fact it should be done > > instead of PCIe interface usage, since NTB clients are supposed to > > use NTB API only and left unaware of real hardware implementation. > > In addition to that ntb_device_register() method shouldn't clear > > the passed ntb_dev structure, since it dma_mask is initialized > > by hardware drivers. > > This seems like a bug fix. Should this be broken out of this patch or > is it not noticed without your changes? > Yes, this is a bug fix. All the NTB API users shouldn't know what hardware type is hidden behind it, so the code would be portable for all NTB devices. In this case NTB device should have dma_masks being properly initialized. Since we pass DMA addresses to the PCI device then in hardware drivers, it's justified to copy the masks to NTB device, so the client drivers could allocate the proper memory addresses. It can't be broken out, since dma_alloc_coherent() is called in ntb_perf driver with NTB device passed now. We discovered the problem when the series was tested on the Intel platform. The same is with ntb_tool driver. Thanks, -Sergey > Thanks, > Jon > > > Signed-off-by: Serge Semin > > --- > > drivers/ntb/hw/amd/ntb_hw_amd.c | 4 ++++ > > drivers/ntb/hw/idt/ntb_hw_idt.c | 8 +++++++- > > drivers/ntb/hw/intel/ntb_hw_intel.c | 4 ++++ > > drivers/ntb/ntb.c | 1 - > > 4 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c > > index f0788aae05c9..3cfa46876239 100644 > > --- a/drivers/ntb/hw/amd/ntb_hw_amd.c > > +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c > > @@ -1020,6 +1020,10 @@ static int amd_ntb_init_pci(struct amd_ntb_dev *ndev, > > goto err_dma_mask; > > dev_warn(&pdev->dev, "Cannot DMA consistent highmem\n"); > > } > > + rc = dma_coerce_mask_and_coherent(&ndev->ntb.dev, > > + dma_get_mask(&pdev->dev)); > > + if (rc) > > + goto err_dma_mask; > > > > ndev->self_mmio = pci_iomap(pdev, 0, 0); > > if (!ndev->self_mmio) { > > diff --git a/drivers/ntb/hw/idt/ntb_hw_idt.c b/drivers/ntb/hw/idt/ntb_hw_idt.c > > index 6fb87c0f0d97..d87dbd4bc82c 100644 > > --- a/drivers/ntb/hw/idt/ntb_hw_idt.c > > +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c > > @@ -2426,7 +2426,7 @@ static int idt_init_pci(struct idt_ntb_dev *ndev) > > struct pci_dev *pdev = ndev->ntb.pdev; > > int ret; > > > > - /* Initialize the bit mask of DMA */ > > + /* Initialize the bit mask of PCI/NTB DMA */ > > ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64)); > > if (ret != 0) { > > ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); > > @@ -2447,6 +2447,12 @@ static int idt_init_pci(struct idt_ntb_dev *ndev) > > dev_warn(&pdev->dev, > > "Cannot set consistent DMA highmem bit mask\n"); > > } > > + ret = dma_coerce_mask_and_coherent(&ndev->ntb.dev, > > + dma_get_mask(&pdev->dev)); > > + if (ret != 0) { > > + dev_err(&pdev->dev, "Failed to set NTB device DMA bit mask\n"); > > + return ret; > > + } > > > > /* > > * Enable the device advanced error reporting. It's not critical to > > diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.c b/drivers/ntb/hw/intel/ntb_hw_intel.c > > index 4de074a86073..ea6c82f8655f 100644 > > --- a/drivers/ntb/hw/intel/ntb_hw_intel.c > > +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c > > @@ -2586,6 +2586,10 @@ static int intel_ntb_init_pci(struct intel_ntb_dev *ndev, struct pci_dev *pdev) > > goto err_dma_mask; > > dev_warn(&pdev->dev, "Cannot DMA consistent highmem\n"); > > } > > + rc = dma_coerce_mask_and_coherent(&ndev->ntb.dev, > > + dma_get_mask(&pdev->dev)); > > + if (rc) > > + goto err_dma_mask; > > > > ndev->self_mmio = pci_iomap(pdev, 0, 0); > > if (!ndev->self_mmio) { > > diff --git a/drivers/ntb/ntb.c b/drivers/ntb/ntb.c > > index 03b80d89b980..79e50a5f8902 100644 > > --- a/drivers/ntb/ntb.c > > +++ b/drivers/ntb/ntb.c > > @@ -112,7 +112,6 @@ int ntb_register_device(struct ntb_dev *ntb) > > > > init_completion(&ntb->released); > > > > - memset(&ntb->dev, 0, sizeof(ntb->dev)); > > ntb->dev.bus = &ntb_bus; > > ntb->dev.parent = &ntb->pdev->dev; > > ntb->dev.release = ntb_dev_release; > > -- > > 2.12.0 > >