Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On 12/06/18 09:48 AM, Jon Mason wrote: > ntb.c is more of a glue layer, and this is more device specific. > While I like adding it here for more common code, it should probably > reside in the ntb_hw_*.c files to enforce the hw specific code all > reside in that layer. So, this probably needs to be replaced with a > patch which adds the setting of the mask to the switchtec driver. I disagree strongly. You can tell it's not device specific seeing we have 4 devices that need the exact same code. In fact, there is nothing device specific in those lines of code as the device specific part comes when a driver sets the PCI parent device's DMA mask. These lines just initialize the dma_mask for the new NTB device with its parent's mask. This is just sensible given that nothing now works if it is not done and trusting driver writers to get it right is not a good idea seeing we already screwed it up once. Furthermore, it violates DRY (do not repeat yourself). If there is something driver specific that must be done (although I can't actually imagine what this would be) the drivers are free to change the mask after calling ntb_register but getting the common case setup in common code is just good design. Logan
Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On 12/06/18 09:48 AM, Jon Mason wrote: > ntb.c is more of a glue layer, and this is more device specific. > While I like adding it here for more common code, it should probably > reside in the ntb_hw_*.c files to enforce the hw specific code all > reside in that layer. So, this probably needs to be replaced with a > patch which adds the setting of the mask to the switchtec driver. I disagree strongly. You can tell it's not device specific seeing we have 4 devices that need the exact same code. In fact, there is nothing device specific in those lines of code as the device specific part comes when a driver sets the PCI parent device's DMA mask. These lines just initialize the dma_mask for the new NTB device with its parent's mask. This is just sensible given that nothing now works if it is not done and trusting driver writers to get it right is not a good idea seeing we already screwed it up once. Furthermore, it violates DRY (do not repeat yourself). If there is something driver specific that must be done (although I can't actually imagine what this would be) the drivers are free to change the mask after calling ntb_register but getting the common case setup in common code is just good design. Logan
Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On Fri, Jun 8, 2018 at 8:08 PM, Logan Gunthorpe wrote:
> Commit 417cf39cfea9 ("NTB: Set dma mask and dma coherent mask to NTB
> devices") added code to set the DMA mask for the NTB device
> to each driver individually. However, it neglected to set it for the
> Switchtec driver. So when the monolithic commit 7f46c8b3a552 ("NTB:
> ntb_tool: Add full multi-port NTB API support") started allocating
> DMA memory against the NTB device it broke the Switchtec driver.
>
> Seeing this is setting up a property of the NTB device, it should be
> done by the common NTB code (inside ntb_register_device()) so we can be
> sure it's done properly for all drivers. This avoids each driver needing
> to duplicate the code and helps prevent us from inadvertently breaking
> one of the drivers in the future if we have to make changes in this area.
>
> Fixes: 7f46c8b3a552 ("NTB: ntb_tool: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe
> ---
> drivers/ntb/hw/amd/ntb_hw_amd.c | 4
> drivers/ntb/hw/idt/ntb_hw_idt.c | 6 --
> drivers/ntb/hw/intel/ntb_hw_intel.c | 4
> drivers/ntb/ntb.c | 13 -
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 3cfa46876239..f0788aae05c9 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -1020,10 +1020,6 @@ static int amd_ntb_init_pci(struct amd_ntb_dev *ndev,
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 8d98872d0983..1918a2db1c43 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2447,12 +2447,6 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
> dev_warn(>dev,
> "Cannot set consistent DMA highmem bit mask\n");
> }
> - ret = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>dev));
> - if (ret != 0) {
> - dev_err(>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 156b45cd4a19..341a3d5baa3f 100644
> --- a/drivers/ntb/hw/intel/ntb_hw_intel.c
> +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
> @@ -2334,10 +2334,6 @@ static int intel_ntb_init_pci(struct intel_ntb_dev
> *ndev, struct pci_dev *pdev)
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 2581ab724c34..93f24440d11d 100644
> --- a/drivers/ntb/ntb.c
> +++ b/drivers/ntb/ntb.c
> @@ -100,6 +100,8 @@ EXPORT_SYMBOL(ntb_unregister_client);
>
> int ntb_register_device(struct ntb_dev *ntb)
> {
> + int ret;
> +
> if (!ntb)
> return -EINVAL;
> if (!ntb->pdev)
> @@ -120,7 +122,16 @@ int ntb_register_device(struct ntb_dev *ntb)
> ntb->ctx_ops = NULL;
> spin_lock_init(>ctx_lock);
>
> - return device_register(>dev);
> + device_initialize(>dev);
> +
> + ret = dma_coerce_mask_and_coherent(>dev,
> + dma_get_mask(>pdev->dev));
ntb.c is more of a glue layer, and this is more device specific.
While I like adding it here for more common code, it should probably
reside in the ntb_hw_*.c files to enforce the hw specific code all
reside in that layer. So, this probably needs to be replaced with a
patch which adds the setting of the mask to the switchtec driver.
Thanks,
Jon
> + if (ret != 0) {
> + dev_err(>dev, "Failed to set NTB device DMA bit mask\n");
> + return ret;
> + }
> +
> + return device_add(>dev);
> }
> EXPORT_SYMBOL(ntb_register_device);
>
> --
> 2.11.0
>
Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On Fri, Jun 8, 2018 at 8:08 PM, Logan Gunthorpe wrote:
> Commit 417cf39cfea9 ("NTB: Set dma mask and dma coherent mask to NTB
> devices") added code to set the DMA mask for the NTB device
> to each driver individually. However, it neglected to set it for the
> Switchtec driver. So when the monolithic commit 7f46c8b3a552 ("NTB:
> ntb_tool: Add full multi-port NTB API support") started allocating
> DMA memory against the NTB device it broke the Switchtec driver.
>
> Seeing this is setting up a property of the NTB device, it should be
> done by the common NTB code (inside ntb_register_device()) so we can be
> sure it's done properly for all drivers. This avoids each driver needing
> to duplicate the code and helps prevent us from inadvertently breaking
> one of the drivers in the future if we have to make changes in this area.
>
> Fixes: 7f46c8b3a552 ("NTB: ntb_tool: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe
> ---
> drivers/ntb/hw/amd/ntb_hw_amd.c | 4
> drivers/ntb/hw/idt/ntb_hw_idt.c | 6 --
> drivers/ntb/hw/intel/ntb_hw_intel.c | 4
> drivers/ntb/ntb.c | 13 -
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 3cfa46876239..f0788aae05c9 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -1020,10 +1020,6 @@ static int amd_ntb_init_pci(struct amd_ntb_dev *ndev,
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 8d98872d0983..1918a2db1c43 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2447,12 +2447,6 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
> dev_warn(>dev,
> "Cannot set consistent DMA highmem bit mask\n");
> }
> - ret = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>dev));
> - if (ret != 0) {
> - dev_err(>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 156b45cd4a19..341a3d5baa3f 100644
> --- a/drivers/ntb/hw/intel/ntb_hw_intel.c
> +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
> @@ -2334,10 +2334,6 @@ static int intel_ntb_init_pci(struct intel_ntb_dev
> *ndev, struct pci_dev *pdev)
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 2581ab724c34..93f24440d11d 100644
> --- a/drivers/ntb/ntb.c
> +++ b/drivers/ntb/ntb.c
> @@ -100,6 +100,8 @@ EXPORT_SYMBOL(ntb_unregister_client);
>
> int ntb_register_device(struct ntb_dev *ntb)
> {
> + int ret;
> +
> if (!ntb)
> return -EINVAL;
> if (!ntb->pdev)
> @@ -120,7 +122,16 @@ int ntb_register_device(struct ntb_dev *ntb)
> ntb->ctx_ops = NULL;
> spin_lock_init(>ctx_lock);
>
> - return device_register(>dev);
> + device_initialize(>dev);
> +
> + ret = dma_coerce_mask_and_coherent(>dev,
> + dma_get_mask(>pdev->dev));
ntb.c is more of a glue layer, and this is more device specific.
While I like adding it here for more common code, it should probably
reside in the ntb_hw_*.c files to enforce the hw specific code all
reside in that layer. So, this probably needs to be replaced with a
patch which adds the setting of the mask to the switchtec driver.
Thanks,
Jon
> + if (ret != 0) {
> + dev_err(>dev, "Failed to set NTB device DMA bit mask\n");
> + return ret;
> + }
> +
> + return device_add(>dev);
> }
> EXPORT_SYMBOL(ntb_register_device);
>
> --
> 2.11.0
>
Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On 06/08/2018 05:08 PM, Logan Gunthorpe wrote:
> Commit 417cf39cfea9 ("NTB: Set dma mask and dma coherent mask to NTB
> devices") added code to set the DMA mask for the NTB device
> to each driver individually. However, it neglected to set it for the
> Switchtec driver. So when the monolithic commit 7f46c8b3a552 ("NTB:
> ntb_tool: Add full multi-port NTB API support") started allocating
> DMA memory against the NTB device it broke the Switchtec driver.
>
> Seeing this is setting up a property of the NTB device, it should be
> done by the common NTB code (inside ntb_register_device()) so we can be
> sure it's done properly for all drivers. This avoids each driver needing
> to duplicate the code and helps prevent us from inadvertently breaking
> one of the drivers in the future if we have to make changes in this area.
>
> Fixes: 7f46c8b3a552 ("NTB: ntb_tool: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe
Acked-by: Dave Jiang
for the Intel parts and the generic parts
> ---
> drivers/ntb/hw/amd/ntb_hw_amd.c | 4
> drivers/ntb/hw/idt/ntb_hw_idt.c | 6 --
> drivers/ntb/hw/intel/ntb_hw_intel.c | 4
> drivers/ntb/ntb.c | 13 -
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 3cfa46876239..f0788aae05c9 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -1020,10 +1020,6 @@ static int amd_ntb_init_pci(struct amd_ntb_dev *ndev,
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 8d98872d0983..1918a2db1c43 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2447,12 +2447,6 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
> dev_warn(>dev,
> "Cannot set consistent DMA highmem bit mask\n");
> }
> - ret = dma_coerce_mask_and_coherent(>ntb.dev,
> -dma_get_mask(>dev));
> - if (ret != 0) {
> - dev_err(>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 156b45cd4a19..341a3d5baa3f 100644
> --- a/drivers/ntb/hw/intel/ntb_hw_intel.c
> +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
> @@ -2334,10 +2334,6 @@ static int intel_ntb_init_pci(struct intel_ntb_dev
> *ndev, struct pci_dev *pdev)
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 2581ab724c34..93f24440d11d 100644
> --- a/drivers/ntb/ntb.c
> +++ b/drivers/ntb/ntb.c
> @@ -100,6 +100,8 @@ EXPORT_SYMBOL(ntb_unregister_client);
>
> int ntb_register_device(struct ntb_dev *ntb)
> {
> + int ret;
> +
> if (!ntb)
> return -EINVAL;
> if (!ntb->pdev)
> @@ -120,7 +122,16 @@ int ntb_register_device(struct ntb_dev *ntb)
> ntb->ctx_ops = NULL;
> spin_lock_init(>ctx_lock);
>
> - return device_register(>dev);
> + device_initialize(>dev);
> +
> + ret = dma_coerce_mask_and_coherent(>dev,
> +dma_get_mask(>pdev->dev));
> + if (ret != 0) {
> + dev_err(>dev, "Failed to set NTB device DMA bit mask\n");
> + return ret;
> + }
> +
> + return device_add(>dev);
> }
> EXPORT_SYMBOL(ntb_register_device);
>
>
Re: [PATCH 2/8] NTB: Setup the DMA mask globally for all drivers
On 06/08/2018 05:08 PM, Logan Gunthorpe wrote:
> Commit 417cf39cfea9 ("NTB: Set dma mask and dma coherent mask to NTB
> devices") added code to set the DMA mask for the NTB device
> to each driver individually. However, it neglected to set it for the
> Switchtec driver. So when the monolithic commit 7f46c8b3a552 ("NTB:
> ntb_tool: Add full multi-port NTB API support") started allocating
> DMA memory against the NTB device it broke the Switchtec driver.
>
> Seeing this is setting up a property of the NTB device, it should be
> done by the common NTB code (inside ntb_register_device()) so we can be
> sure it's done properly for all drivers. This avoids each driver needing
> to duplicate the code and helps prevent us from inadvertently breaking
> one of the drivers in the future if we have to make changes in this area.
>
> Fixes: 7f46c8b3a552 ("NTB: ntb_tool: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe
Acked-by: Dave Jiang
for the Intel parts and the generic parts
> ---
> drivers/ntb/hw/amd/ntb_hw_amd.c | 4
> drivers/ntb/hw/idt/ntb_hw_idt.c | 6 --
> drivers/ntb/hw/intel/ntb_hw_intel.c | 4
> drivers/ntb/ntb.c | 13 -
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 3cfa46876239..f0788aae05c9 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -1020,10 +1020,6 @@ static int amd_ntb_init_pci(struct amd_ntb_dev *ndev,
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 8d98872d0983..1918a2db1c43 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2447,12 +2447,6 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
> dev_warn(>dev,
> "Cannot set consistent DMA highmem bit mask\n");
> }
> - ret = dma_coerce_mask_and_coherent(>ntb.dev,
> -dma_get_mask(>dev));
> - if (ret != 0) {
> - dev_err(>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 156b45cd4a19..341a3d5baa3f 100644
> --- a/drivers/ntb/hw/intel/ntb_hw_intel.c
> +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
> @@ -2334,10 +2334,6 @@ static int intel_ntb_init_pci(struct intel_ntb_dev
> *ndev, struct pci_dev *pdev)
> goto err_dma_mask;
> dev_warn(>dev, "Cannot DMA consistent highmem\n");
> }
> - rc = dma_coerce_mask_and_coherent(>ntb.dev,
> - dma_get_mask(>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 2581ab724c34..93f24440d11d 100644
> --- a/drivers/ntb/ntb.c
> +++ b/drivers/ntb/ntb.c
> @@ -100,6 +100,8 @@ EXPORT_SYMBOL(ntb_unregister_client);
>
> int ntb_register_device(struct ntb_dev *ntb)
> {
> + int ret;
> +
> if (!ntb)
> return -EINVAL;
> if (!ntb->pdev)
> @@ -120,7 +122,16 @@ int ntb_register_device(struct ntb_dev *ntb)
> ntb->ctx_ops = NULL;
> spin_lock_init(>ctx_lock);
>
> - return device_register(>dev);
> + device_initialize(>dev);
> +
> + ret = dma_coerce_mask_and_coherent(>dev,
> +dma_get_mask(>pdev->dev));
> + if (ret != 0) {
> + dev_err(>dev, "Failed to set NTB device DMA bit mask\n");
> + return ret;
> + }
> +
> + return device_add(>dev);
> }
> EXPORT_SYMBOL(ntb_register_device);
>
>
