Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1548364 > unrolled thread

[PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-12-29 21:50 +0100
    [PATCH 2/2] rcar-pcie: set host bridge's DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-12-29 21:50 +0100
    Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-12-30 10:50 +0100
      Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-12-30 11:10 +0100
    Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Will Deacon <will.deacon@arm.com> - 2017-01-03 19:50 +0100
      Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-03 20:10 +0100
        Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Grygorii Strashko <grygorii.strashko@ti.com> - 2017-01-03 21:20 +0100
          Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-03 21:40 +0100
      Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@arndb.de> - 2017-01-04 00:20 +0100
        Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-04 07:40 +0100
          Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@arndb.de> - 2017-01-04 14:30 +0100
            Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-04 15:40 +0100
              Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@arndb.de> - 2017-01-04 15:50 +0100
                Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-04 16:40 +0100
                  Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@arndb.de> - 2017-01-06 12:20 +0100
                    Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-06 14:50 +0100
                      [PATCH] arm64: do not set dma masks that device connection can't handle Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-06 15:40 +0100
                      [PATCH] arm64: do not set dma masks that device connection can't handle Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-06 16:00 +0100
                        Re: [PATCH] arm64: do not set dma masks that device connection can't  handle Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-01-08 08:20 +0100
                          Re: [PATCH] arm64: do not set dma masks that device connection can't  handle Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-09 08:00 +0100
                      Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@linaro.org> - 2017-01-09 15:10 +0100
                        Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit  DMA mask Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-09 21:40 +0100
                          Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit         DMA mask Christoph Hellwig <hch@lst.de> - 2017-01-09 22:00 +0100
                            NVMe vs DMA addressing limitations Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-10 07:50 +0100
                              Re: NVMe vs DMA addressing limitations Christoph Hellwig <hch@lst.de> - 2017-01-10 08:10 +0100
                                Re: NVMe vs DMA addressing limitations Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-01-10 08:40 +0100
                                  Re: NVMe vs DMA addressing limitations Arnd Bergmann <arnd@linaro.org> - 2017-01-10 12:10 +0100
                                    Re: NVMe vs DMA addressing limitations Christoph Hellwig <hch@lst.de> - 2017-01-10 15:50 +0100
                                      Re: NVMe vs DMA addressing limitations Arnd Bergmann <arnd@linaro.org> - 2017-01-10 16:10 +0100
                                      Re: NVMe vs DMA addressing limitations Sagi Grimberg <sagi@grimberg.me> - 2017-01-12 11:10 +0100
                                        Re: NVMe vs DMA addressing limitations Arnd Bergmann <arnd@linaro.org> - 2017-01-12 13:00 +0100
                                          Re: NVMe vs DMA addressing limitations Christoph Hellwig <hch@lst.de> - 2017-01-12 14:20 +0100
                                Re: NVMe vs DMA addressing limitations Arnd Bergmann <arnd@linaro.org> - 2017-01-10 12:00 +0100
                            Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@linaro.org> - 2017-01-10 11:50 +0100
                              Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit         DMA mask Christoph Hellwig <hch@lst.de> - 2017-01-10 15:50 +0100
                                Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask Arnd Bergmann <arnd@linaro.org> - 2017-01-10 16:10 +0100

Page 1 of 2  [1] 2  Next page →

#1548364 — [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

It is possible that PCI device supports 64-bit DMA addressing, and thus
it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host
bridge has limitations on inbound transactions addressing. Example of
such setup is NVME SSD device connected to RCAR PCIe controller.

Previously there was attempt to handle this via bus notifier: after
driver is attached to PCI device, bridge driver gets notifier callback,
and resets dma_mask from there. However, this is racy: PCI device driver
could already allocate buffers and/or start i/o in probe routine.
In NVME case, i/o is started in workqueue context, and this race gives
"sometimes works, sometimes not" effect.

Proper solution should make driver's dma_set_mask() call to fail if host
bridge can't support mask being set.

This patch makes __swiotlb_dma_supported() to check mask being set for
PCI device against dma_mask of struct device corresponding to PCI host
bridge (one with name "pciXXXX:YY"), if that dma_mask is set.

This is the least destructive approach: currently dma_mask of that device
object is not used anyhow, thus all existing setups will work as before,
and modification is required only in actually affected components -
driver of particular PCI host bridge, and dma_map_ops of particular
platform.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 arch/arm64/mm/dma-mapping.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..49645277 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -28,6 +28,7 @@
 #include <linux/dma-contiguous.h>
 #include <linux/vmalloc.h>
 #include <linux/swiotlb.h>
+#include <linux/pci.h>
 
 #include <asm/cacheflush.h>
 
@@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
 
 static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 {
+#ifdef CONFIG_PCI
+	if (dev_is_pci(hwdev)) {
+		struct pci_dev *pdev = to_pci_dev(hwdev);
+		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
+
+		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
+				(mask & (*br->dev.dma_mask)) != mask)
+			return 0;
+	}
+#endif
 	if (swiotlb)
 		return swiotlb_dma_supported(hwdev, mask);
 	return 1;
-- 
2.1.4

[toc] | [next] | [standalone]

#1548365 — [PATCH 2/2] rcar-pcie: set host bridge's DMA mask

This gives platform DMA mapping code a chance to disallow setting device
DMA mask to something that host bridge can't support.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/pci/host/pcie-rcar.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index aca85be..b1edc3c 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -451,6 +451,7 @@ static int rcar_pcie_enable(struct rcar_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct pci_bus *bus, *child;
+	struct pci_host_bridge *bridge;
 	LIST_HEAD(res);
 
 	/* Try setting 5 GT/s link speed */
@@ -480,6 +481,10 @@ static int rcar_pcie_enable(struct rcar_pcie *pcie)
 	list_for_each_entry(child, &bus->children, node)
 		pcie_bus_configure_settings(child);
 
+	bridge = pci_find_host_bridge(bus);
+	bridge->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	bridge->dev.dma_mask = &bridge->dev.coherent_dma_mask;
+
 	pci_bus_add_devices(bus);
 
 	return 0;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1548514 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

Hello!

On 12/29/2016 11:45 PM, Nikita Yushchenko wrote:

> It is possible that PCI device supports 64-bit DMA addressing, and thus
> it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host

    Its.

> bridge has limitations on inbound transactions addressing. Example of
> such setup is NVME

    Isn't it called NVMe?

> SSD device connected to RCAR PCIe controller.

    R=Car.

> Previously there was attempt to handle this via bus notifier: after
> driver is attached to PCI device, bridge driver gets notifier callback,
> and resets dma_mask from there. However, this is racy: PCI device driver
> could already allocate buffers and/or start i/o in probe routine.
> In NVME case, i/o is started in workqueue context, and this race gives
> "sometimes works, sometimes not" effect.
>
> Proper solution should make driver's dma_set_mask() call to fail if host
> bridge can't support mask being set.
>
> This patch makes __swiotlb_dma_supported() to check mask being set for

    "To" not needed here.

> PCI device against dma_mask of struct device corresponding to PCI host
> bridge (one with name "pciXXXX:YY"), if that dma_mask is set.
>
> This is the least destructive approach: currently dma_mask of that device
> object is not used anyhow, thus all existing setups will work as before,
> and modification is required only in actually affected components -
> driver of particular PCI host bridge, and dma_map_ops of particular
> platform.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  arch/arm64/mm/dma-mapping.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 290a84f..49645277 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
[...]
> @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>
>  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  {
> +#ifdef CONFIG_PCI
> +	if (dev_is_pci(hwdev)) {
> +		struct pci_dev *pdev = to_pci_dev(hwdev);
> +		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
> +
> +		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
> +				(mask & (*br->dev.dma_mask)) != mask)

    Hum, inner parens not necessary?

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1548521 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

On 12/30/2016 12:46 PM, Sergei Shtylyov wrote:

>> It is possible that PCI device supports 64-bit DMA addressing, and thus
>> it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host
>
>    Its.
>
>> bridge has limitations on inbound transactions addressing. Example of
>> such setup is NVME
>
>    Isn't it called NVMe?
>
>> SSD device connected to RCAR PCIe controller.
>
>    R=Car.

    Sorry, R-Car. :-)

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1550090 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

On Thu, Dec 29, 2016 at 11:45:03PM +0300, Nikita Yushchenko wrote:
> It is possible that PCI device supports 64-bit DMA addressing, and thus
> it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host
> bridge has limitations on inbound transactions addressing. Example of
> such setup is NVME SSD device connected to RCAR PCIe controller.
> 
> Previously there was attempt to handle this via bus notifier: after
> driver is attached to PCI device, bridge driver gets notifier callback,
> and resets dma_mask from there. However, this is racy: PCI device driver
> could already allocate buffers and/or start i/o in probe routine.
> In NVME case, i/o is started in workqueue context, and this race gives
> "sometimes works, sometimes not" effect.
> 
> Proper solution should make driver's dma_set_mask() call to fail if host
> bridge can't support mask being set.
> 
> This patch makes __swiotlb_dma_supported() to check mask being set for
> PCI device against dma_mask of struct device corresponding to PCI host
> bridge (one with name "pciXXXX:YY"), if that dma_mask is set.
> 
> This is the least destructive approach: currently dma_mask of that device
> object is not used anyhow, thus all existing setups will work as before,
> and modification is required only in actually affected components -
> driver of particular PCI host bridge, and dma_map_ops of particular
> platform.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  arch/arm64/mm/dma-mapping.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 290a84f..49645277 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -28,6 +28,7 @@
>  #include <linux/dma-contiguous.h>
>  #include <linux/vmalloc.h>
>  #include <linux/swiotlb.h>
> +#include <linux/pci.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>  
>  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  {
> +#ifdef CONFIG_PCI
> +	if (dev_is_pci(hwdev)) {
> +		struct pci_dev *pdev = to_pci_dev(hwdev);
> +		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
> +
> +		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
> +				(mask & (*br->dev.dma_mask)) != mask)
> +			return 0;
> +	}
> +#endif

Hmm, but this makes it look like the problem is both arm64 and swiotlb
specific, when in reality it's not. Perhaps another hack you could try
would be to register a PCI bus notifier in the host bridge looking for
BUS_NOTIFY_BIND_DRIVER, then you could proxy the DMA ops for each child
device before the driver has probed, but adding a dma_set_mask callback
to limit the mask to what you need?

I agree that it would be better if dma_set_mask handled all of this
transparently, but it's all based on the underlying ops rather than the
bus type.

Will

[toc] | [prev] | [next] | [standalone]

#1550108 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

>> It is possible that PCI device supports 64-bit DMA addressing, and thus
>> it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host
>> bridge has limitations on inbound transactions addressing. Example of
>> such setup is NVME SSD device connected to RCAR PCIe controller.
>>
>> Previously there was attempt to handle this via bus notifier: after
>> driver is attached to PCI device, bridge driver gets notifier callback,
>> and resets dma_mask from there. However, this is racy: PCI device driver
>> could already allocate buffers and/or start i/o in probe routine.
>> In NVME case, i/o is started in workqueue context, and this race gives
>> "sometimes works, sometimes not" effect.
>>
>> Proper solution should make driver's dma_set_mask() call to fail if host
>> bridge can't support mask being set.
>>
>> This patch makes __swiotlb_dma_supported() to check mask being set for
>> PCI device against dma_mask of struct device corresponding to PCI host
>> bridge (one with name "pciXXXX:YY"), if that dma_mask is set.
>>
>> This is the least destructive approach: currently dma_mask of that device
>> object is not used anyhow, thus all existing setups will work as before,
>> and modification is required only in actually affected components -
>> driver of particular PCI host bridge, and dma_map_ops of particular
>> platform.
>>
>> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> ---
>>  arch/arm64/mm/dma-mapping.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index 290a84f..49645277 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/dma-contiguous.h>
>>  #include <linux/vmalloc.h>
>>  #include <linux/swiotlb.h>
>> +#include <linux/pci.h>
>>  
>>  #include <asm/cacheflush.h>
>>  
>> @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>>  
>>  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>>  {
>> +#ifdef CONFIG_PCI
>> +	if (dev_is_pci(hwdev)) {
>> +		struct pci_dev *pdev = to_pci_dev(hwdev);
>> +		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
>> +
>> +		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
>> +				(mask & (*br->dev.dma_mask)) != mask)
>> +			return 0;
>> +	}
>> +#endif
> 
> Hmm, but this makes it look like the problem is both arm64 and swiotlb
> specific, when in reality it's not. Perhaps another hack you could try
> would be to register a PCI bus notifier in the host bridge looking for
> BUS_NOTIFY_BIND_DRIVER, then you could proxy the DMA ops for each child
> device before the driver has probed, but adding a dma_set_mask callback
> to limit the mask to what you need?

This is what Renesas BSP tries to do and it does not work.

BUS_NOTIFY_BIND_DRIVER arrives after driver's probe routine exits, but
i/o can be started before that.

[toc] | [prev] | [next] | [standalone]

#1550151 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

On 01/03/2017 01:01 PM, Nikita Yushchenko wrote:
>>> It is possible that PCI device supports 64-bit DMA addressing, and thus
>>> it's driver sets device's dma_mask to DMA_BIT_MASK(64), however PCI host
>>> bridge has limitations on inbound transactions addressing. Example of
>>> such setup is NVME SSD device connected to RCAR PCIe controller.
>>>
>>> Previously there was attempt to handle this via bus notifier: after
>>> driver is attached to PCI device, bridge driver gets notifier callback,
>>> and resets dma_mask from there. However, this is racy: PCI device driver
>>> could already allocate buffers and/or start i/o in probe routine.
>>> In NVME case, i/o is started in workqueue context, and this race gives
>>> "sometimes works, sometimes not" effect.
>>>
>>> Proper solution should make driver's dma_set_mask() call to fail if host
>>> bridge can't support mask being set.
>>>
>>> This patch makes __swiotlb_dma_supported() to check mask being set for
>>> PCI device against dma_mask of struct device corresponding to PCI host
>>> bridge (one with name "pciXXXX:YY"), if that dma_mask is set.
>>>
>>> This is the least destructive approach: currently dma_mask of that device
>>> object is not used anyhow, thus all existing setups will work as before,
>>> and modification is required only in actually affected components -
>>> driver of particular PCI host bridge, and dma_map_ops of particular
>>> platform.
>>>
>>> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>>> ---
>>>  arch/arm64/mm/dma-mapping.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>> index 290a84f..49645277 100644
>>> --- a/arch/arm64/mm/dma-mapping.c
>>> +++ b/arch/arm64/mm/dma-mapping.c
>>> @@ -28,6 +28,7 @@
>>>  #include <linux/dma-contiguous.h>
>>>  #include <linux/vmalloc.h>
>>>  #include <linux/swiotlb.h>
>>> +#include <linux/pci.h>
>>>  
>>>  #include <asm/cacheflush.h>
>>>  
>>> @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>>>  
>>>  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>>>  {
>>> +#ifdef CONFIG_PCI
>>> +	if (dev_is_pci(hwdev)) {
>>> +		struct pci_dev *pdev = to_pci_dev(hwdev);
>>> +		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
>>> +
>>> +		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
>>> +				(mask & (*br->dev.dma_mask)) != mask)
>>> +			return 0;
>>> +	}
>>> +#endif
>>
>> Hmm, but this makes it look like the problem is both arm64 and swiotlb
>> specific, when in reality it's not. Perhaps another hack you could try
>> would be to register a PCI bus notifier in the host bridge looking for
>> BUS_NOTIFY_BIND_DRIVER, then you could proxy the DMA ops for each child
>> device before the driver has probed, but adding a dma_set_mask callback
>> to limit the mask to what you need?
> 
> This is what Renesas BSP tries to do and it does not work.
> 
> BUS_NOTIFY_BIND_DRIVER arrives after driver's probe routine exits, but
> i/o can be started before that.

Hm. This is strange statement:
 really_probe
 |->driver_sysfs_add
    |-> blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_BIND_DRIVER, dev);
...
 |- ret = drv->probe(dev);
...
 |- driver_bound(dev);
    |- blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
					     BUS_NOTIFY_BOUND_DRIVER, dev);

Am I missing smth?

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1550161 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

>>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>>> index 290a84f..49645277 100644
>>>> --- a/arch/arm64/mm/dma-mapping.c
>>>> +++ b/arch/arm64/mm/dma-mapping.c
>>>> @@ -28,6 +28,7 @@
>>>>  #include <linux/dma-contiguous.h>
>>>>  #include <linux/vmalloc.h>
>>>>  #include <linux/swiotlb.h>
>>>> +#include <linux/pci.h>
>>>>  
>>>>  #include <asm/cacheflush.h>
>>>>  
>>>> @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>>>>  
>>>>  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>>>>  {
>>>> +#ifdef CONFIG_PCI
>>>> +	if (dev_is_pci(hwdev)) {
>>>> +		struct pci_dev *pdev = to_pci_dev(hwdev);
>>>> +		struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
>>>> +
>>>> +		if (br->dev.dma_mask && (*br->dev.dma_mask) &&
>>>> +				(mask & (*br->dev.dma_mask)) != mask)
>>>> +			return 0;
>>>> +	}
>>>> +#endif
>>>
>>> Hmm, but this makes it look like the problem is both arm64 and swiotlb
>>> specific, when in reality it's not. Perhaps another hack you could try
>>> would be to register a PCI bus notifier in the host bridge looking for
>>> BUS_NOTIFY_BIND_DRIVER, then you could proxy the DMA ops for each child
>>> device before the driver has probed, but adding a dma_set_mask callback
>>> to limit the mask to what you need?
>>
>> This is what Renesas BSP tries to do and it does not work.
>>
>> BUS_NOTIFY_BIND_DRIVER arrives after driver's probe routine exits, but
>> i/o can be started before that.
> 
> Hm. This is strange statement:
>  really_probe
>  |->driver_sysfs_add
>     |-> blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>  					     BUS_NOTIFY_BIND_DRIVER, dev);
> ...
>  |- ret = drv->probe(dev);
> ...
>  |- driver_bound(dev);
>     |- blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> 					     BUS_NOTIFY_BOUND_DRIVER, dev);
> 
> Am I missing smth?

I misinterpreted your message, sorry.

BSP attaches to BUS_NOTIFY_BOUND_DRIVER, not to BUS_NOTIFY_BIND_DRIVER,
and simply overwrites device's dma_mask there.  You are suggesting
something completely different.

I'll check if your approach is practical.


Currently powerpc architecture has one more approach implemented, they
use pci_controller structure provided by host bridge driver, and that
has a set_dma_mask() hook. Maybe extending this beyond powerpc could be
a good idea. However, that will require changing quite a few host bridge
drivers, without any gain for most of those...

[toc] | [prev] | [next] | [standalone]

#1550283

On Tuesday, January 3, 2017 6:44:44 PM CET Will Deacon wrote:
> > @@ -347,6 +348,16 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> >  
> >  static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
> >  {
> > +#ifdef CONFIG_PCI
> > +     if (dev_is_pci(hwdev)) {
> > +             struct pci_dev *pdev = to_pci_dev(hwdev);
> > +             struct pci_host_bridge *br = pci_find_host_bridge(pdev->bus);
> > +
> > +             if (br->dev.dma_mask && (*br->dev.dma_mask) &&
> > +                             (mask & (*br->dev.dma_mask)) != mask)
> > +                     return 0;
> > +     }
> > +#endif
> 
> Hmm, but this makes it look like the problem is both arm64 and swiotlb
> specific, when in reality it's not. Perhaps another hack you could try
> would be to register a PCI bus notifier in the host bridge looking for
> BUS_NOTIFY_BIND_DRIVER, then you could proxy the DMA ops for each child
> device before the driver has probed, but adding a dma_set_mask callback
> to limit the mask to what you need?
> 
> I agree that it would be better if dma_set_mask handled all of this
> transparently, but it's all based on the underlying ops rather than the
> bus type.

This is what I prototyped a long time ago when this first came up.
I still think this needs to be solved properly for all of arm64, not
with a PCI specific hack, and in particular not using notifiers.

	Arnd

commit 9a57d58d116800a535510053136c6dd7a9c26e25
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue Nov 17 14:06:55 2015 +0100

    [EXPERIMENTAL] ARM64: check implement dma_set_mask
    
    Needs work for coherent mask
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef256b8c9..a57e7bb10e71 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f3351f..aa65875c611b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,31 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -367,6 +392,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
 static int __init atomic_pool_init(void)
@@ -957,6 +983,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }

[toc] | [prev] | [next] | [standalone]

#1550462 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

> commit 9a57d58d116800a535510053136c6dd7a9c26e25
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Tue Nov 17 14:06:55 2015 +0100
> 
>     [EXPERIMENTAL] ARM64: check implement dma_set_mask
>     
>     Needs work for coherent mask
>     
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Unfortunately this is far incomplete

> @@ -957,6 +983,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	if (!dev->archdata.dma_ops)
>  		dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +	/*
> +	 * we don't yet support buses that have a non-zero mapping.
> +	 *  Let's hope we won't need it
> +	 */
> +	WARN_ON(dma_base != 0);
> +
> +	/*
> +	 * Whatever the parent bus can set. A device must not set
> +	 * a DMA mask larger than this.
> +	 */
> +	dev->archdata.parent_dma_mask = size;
> +

... because size/mask passed here for PCI devices are meaningless.

For OF platforms, this is called via of_dma_configure(), that checks
dma-ranges of node that is *parent* for host bridge. Host bridge
currently does not control this at all.

In current device trees no dma-ranges is defined for nodes that are
parents to pci host bridges. This will make of_dma_configure() to fall
back to 32-bit size for all devices on all current platforms.  Thus
applying this patch will immediately break 64-bit dma masks on all
hardware that supports it.


Also related: dma-ranges property used by several pci host bridges is
*not* compatible with "legacy" dma-ranges parsed by of_get_dma_range() -
former uses additional flags word at beginning.

[toc] | [prev] | [next] | [standalone]

#1550771

On Wednesday, January 4, 2017 9:24:09 AM CET Nikita Yushchenko wrote:
> > commit 9a57d58d116800a535510053136c6dd7a9c26e25
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Tue Nov 17 14:06:55 2015 +0100
> > 
> >     [EXPERIMENTAL] ARM64: check implement dma_set_mask
> >     
> >     Needs work for coherent mask
> >     
> >     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Unfortunately this is far incomplete
> 
> > @@ -957,6 +983,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >  	if (!dev->archdata.dma_ops)
> >  		dev->archdata.dma_ops = &swiotlb_dma_ops;
> >  
> > +	/*
> > +	 * we don't yet support buses that have a non-zero mapping.
> > +	 *  Let's hope we won't need it
> > +	 */
> > +	WARN_ON(dma_base != 0);
> > +
> > +	/*
> > +	 * Whatever the parent bus can set. A device must not set
> > +	 * a DMA mask larger than this.
> > +	 */
> > +	dev->archdata.parent_dma_mask = size;
> > +
> 
> ... because size/mask passed here for PCI devices are meaningless.
> 
> For OF platforms, this is called via of_dma_configure(), that checks
> dma-ranges of node that is *parent* for host bridge. Host bridge
> currently does not control this at all.

We need to think about this a bit. Is it actually the PCI host
bridge that limits the ranges here, or the bus that it is connected
to. In the latter case, the caller needs to be adapted to handle
both.

> In current device trees no dma-ranges is defined for nodes that are
> parents to pci host bridges. This will make of_dma_configure() to fall
> back to 32-bit size for all devices on all current platforms.  Thus
> applying this patch will immediately break 64-bit dma masks on all
> hardware that supports it.

No, it won't break it, it will just fall back to swiotlb for all the
ones that are lacking the dma-ranges property. I think this is correct
behavior.

> Also related: dma-ranges property used by several pci host bridges is
> *not* compatible with "legacy" dma-ranges parsed by of_get_dma_range() -
> former uses additional flags word at beginning.

Can you elaborate? Do we have PCI host bridges that use wrongly formatted
dma-ranges properties?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1550871 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

>> For OF platforms, this is called via of_dma_configure(), that checks
>> dma-ranges of node that is *parent* for host bridge. Host bridge
>> currently does not control this at all.
> 
> We need to think about this a bit. Is it actually the PCI host
> bridge that limits the ranges here, or the bus that it is connected
> to. In the latter case, the caller needs to be adapted to handle
> both.

In r-car case, I'm not sure what is the source of limitation at physical
level.

pcie-rcar driver configures ranges for PCIe inbound transactions based
on dma-ranges property in it's device tree node. In the current device
tree for this platform, that only contains one range and it is in lower
memory.

NVMe driver tries i/o to kmalloc()ed area. That returns 0x5xxxxxxxx
addresses here. As a quick experiment, I tried to add second range to
pcie-rcar's dma-ranges to cover 0x5xxxxxxxx area - but that did not make
DMA to high addresses working.

My current understanding is that host bridge hardware module can't
handle inbound transactions to PCI addresses above 4G - and this
limitations comes from host bridge itself.

I've read somewhere in the lists that pcie-rcar hardware is "32-bit" -
but I don't remember where, and don't know lowlevel details. Maybe
somebody from linux-renesas can elaborate?


>> In current device trees no dma-ranges is defined for nodes that are
>> parents to pci host bridges. This will make of_dma_configure() to fall
>> back to 32-bit size for all devices on all current platforms.  Thus
>> applying this patch will immediately break 64-bit dma masks on all
>> hardware that supports it.
> 
> No, it won't break it, it will just fall back to swiotlb for all the
> ones that are lacking the dma-ranges property. I think this is correct
> behavior.

I'd say - for all ones that have parents without dma-ranges property.

As of 4.10-rc2, I see only two definitions of wide parent dma-ranges
under arch/arm64/boot/dts/ - in amd/amd-seattle-soc.dtsi and
apm/apm-storm.dtsi

Are these the only arm64 platforms that can to DMA to high addresses?
I'm not arm64 expert but I'd be surprised if that's the case.


>> Also related: dma-ranges property used by several pci host bridges is
>> *not* compatible with "legacy" dma-ranges parsed by of_get_dma_range() -
>> former uses additional flags word at beginning.
> 
> Can you elaborate? Do we have PCI host bridges that use wrongly formatted
> dma-ranges properties?

of_dma_get_range() expects <dma_addr cpu_addr size> format.

pcie-rcar.c, pci-rcar-gen2.c, pci-xgene.c and pcie-iproc.c from
drivers/pci/host/ all parse dma-ranges using of_pci_range_parser that
uses <flags pci-addr cpu-addr size> format - i.e. something different
from what of_dma_get_range() uses.


Nikita

[toc] | [prev] | [next] | [standalone]

#1550875

On Wednesday, January 4, 2017 5:30:19 PM CET Nikita Yushchenko wrote:
> >> For OF platforms, this is called via of_dma_configure(), that checks
> >> dma-ranges of node that is *parent* for host bridge. Host bridge
> >> currently does not control this at all.
> > 
> > We need to think about this a bit. Is it actually the PCI host
> > bridge that limits the ranges here, or the bus that it is connected
> > to. In the latter case, the caller needs to be adapted to handle
> > both.
> 
> In r-car case, I'm not sure what is the source of limitation at physical
> level.
> 
> pcie-rcar driver configures ranges for PCIe inbound transactions based
> on dma-ranges property in it's device tree node. In the current device
> tree for this platform, that only contains one range and it is in lower
> memory.
> 
> NVMe driver tries i/o to kmalloc()ed area. That returns 0x5xxxxxxxx
> addresses here. As a quick experiment, I tried to add second range to
> pcie-rcar's dma-ranges to cover 0x5xxxxxxxx area - but that did not make
> DMA to high addresses working.
> 
> My current understanding is that host bridge hardware module can't
> handle inbound transactions to PCI addresses above 4G - and this
> limitations comes from host bridge itself.
> 
> I've read somewhere in the lists that pcie-rcar hardware is "32-bit" -
> but I don't remember where, and don't know lowlevel details. Maybe
> somebody from linux-renesas can elaborate?

Just a guess, but if the inbound translation windows in the host
bridge are wider than 32-bit, the reason for setting up a single
32-bit window is probably because that is what the parent bus supports.

> >> In current device trees no dma-ranges is defined for nodes that are
> >> parents to pci host bridges. This will make of_dma_configure() to fall
> >> back to 32-bit size for all devices on all current platforms.  Thus
> >> applying this patch will immediately break 64-bit dma masks on all
> >> hardware that supports it.
> > 
> > No, it won't break it, it will just fall back to swiotlb for all the
> > ones that are lacking the dma-ranges property. I think this is correct
> > behavior.
> 
> I'd say - for all ones that have parents without dma-ranges property.
> 
> As of 4.10-rc2, I see only two definitions of wide parent dma-ranges
> under arch/arm64/boot/dts/ - in amd/amd-seattle-soc.dtsi and
> apm/apm-storm.dtsi
> 
> Are these the only arm64 platforms that can to DMA to high addresses?
> I'm not arm64 expert but I'd be surprised if that's the case.

It's likely that a few others also do high DMA, but a lot of arm64
chips are actually derived from earlier 32-bit chips and don't
even support any RAM above 4GB, as well as having a lot of 32-bit
DMA masters.

> >> Also related: dma-ranges property used by several pci host bridges is
> >> *not* compatible with "legacy" dma-ranges parsed by of_get_dma_range() -
> >> former uses additional flags word at beginning.
> > 
> > Can you elaborate? Do we have PCI host bridges that use wrongly formatted
> > dma-ranges properties?
> 
> of_dma_get_range() expects <dma_addr cpu_addr size> format.
> 
> pcie-rcar.c, pci-rcar-gen2.c, pci-xgene.c and pcie-iproc.c from
> drivers/pci/host/ all parse dma-ranges using of_pci_range_parser that
> uses <flags pci-addr cpu-addr size> format - i.e. something different
> from what of_dma_get_range() uses.

The "dma_addr" here is expressed in terms of #address-cells of the
bus it is in, and that is "3" in case of PCI, where the first 32-bit
word is a bit pattern containing various things, and the other two
cells are a 64-bit address. I think this is correct, but we may
need to add some special handling for parsing PCI host bridges
in of_dma_get_range, to ensure we actually look at translations for
the memory space.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1550932 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

>>>> For OF platforms, this is called via of_dma_configure(), that checks
>>>> dma-ranges of node that is *parent* for host bridge. Host bridge
>>>> currently does not control this at all.
>>>
>>> We need to think about this a bit. Is it actually the PCI host
>>> bridge that limits the ranges here, or the bus that it is connected
>>> to. In the latter case, the caller needs to be adapted to handle
>>> both.
>>
>> In r-car case, I'm not sure what is the source of limitation at physical
>> level.
>>
>> pcie-rcar driver configures ranges for PCIe inbound transactions based
>> on dma-ranges property in it's device tree node. In the current device
>> tree for this platform, that only contains one range and it is in lower
>> memory.
>>
>> NVMe driver tries i/o to kmalloc()ed area. That returns 0x5xxxxxxxx
>> addresses here. As a quick experiment, I tried to add second range to
>> pcie-rcar's dma-ranges to cover 0x5xxxxxxxx area - but that did not make
>> DMA to high addresses working.
>>
>> My current understanding is that host bridge hardware module can't
>> handle inbound transactions to PCI addresses above 4G - and this
>> limitations comes from host bridge itself.
>>
>> I've read somewhere in the lists that pcie-rcar hardware is "32-bit" -
>> but I don't remember where, and don't know lowlevel details. Maybe
>> somebody from linux-renesas can elaborate?
> 
> Just a guess, but if the inbound translation windows in the host
> bridge are wider than 32-bit, the reason for setting up a single
> 32-bit window is probably because that is what the parent bus supports.

Well anyway applying patch similar to your's will fix pcie-rcar + nvme
case - thus I don't object :)   But it can break other cases ...

But why do you hook at set_dma_mask() and overwrite mask inside, instead
of hooking at dma_supported() and rejecting unsupported mask?

I think later is better, because it lets drivers to handle unsupported
high-dma case, like documented in DMA-API_HOWTO.

[toc] | [prev] | [next] | [standalone]

#1552703

On Wednesday, January 4, 2017 6:29:39 PM CET Nikita Yushchenko wrote:

> > Just a guess, but if the inbound translation windows in the host
> > bridge are wider than 32-bit, the reason for setting up a single
> > 32-bit window is probably because that is what the parent bus supports.
> 
> Well anyway applying patch similar to your's will fix pcie-rcar + nvme
> case - thus I don't object :)   But it can break other cases ...
> 
> But why do you hook at set_dma_mask() and overwrite mask inside, instead
> of hooking at dma_supported() and rejecting unsupported mask?
> 
> I think later is better, because it lets drivers to handle unsupported
> high-dma case, like documented in DMA-API_HOWTO.

I think the behavior I put in there is required for swiotlb to make
sense, otherwise you would rely on the driver to handle dma_set_mask()
failure gracefully with its own bounce buffers (as network and
scsi drivers do but others don't).

Having swiotlb or iommu enabled should result in dma_set_mask() always
succeeding unless the mask is too small to cover the swiotlb
bounce buffer area or the iommu virtual address space. This behavior
is particularly important in case the bus address space is narrower
than 32-bit, as we have to guarantee that the fallback to 32-bit
DMA always succeeds. There are also a lot of drivers that try to
set a 64-bit mask but don't implement bounce buffers for streaming
mappings if that fails, and swiotlb is what we use to make those
drivers work.

And yes, the API is a horrible mess.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1552797 — Re: [PATCH 1/2] arm64: dma_mapping: allow PCI host driver to limit DMA mask

>>> Just a guess, but if the inbound translation windows in the host
>>> bridge are wider than 32-bit, the reason for setting up a single
>>> 32-bit window is probably because that is what the parent bus supports.

I've re-checked rcar-pcie hardware documentation.

It indeed mentions that AXI bus it sits on is 32-bit.


>> Well anyway applying patch similar to your's will fix pcie-rcar + nvme
>> case - thus I don't object :)   But it can break other cases ...
>>
>> But why do you hook at set_dma_mask() and overwrite mask inside, instead
>> of hooking at dma_supported() and rejecting unsupported mask?
>>
>> I think later is better, because it lets drivers to handle unsupported
>> high-dma case, like documented in DMA-API_HOWTO.
> 
> I think the behavior I put in there is required for swiotlb to make
> sense, otherwise you would rely on the driver to handle dma_set_mask()
> failure gracefully with its own bounce buffers (as network and
> scsi drivers do but others don't).
> 
> Having swiotlb or iommu enabled should result in dma_set_mask() always
> succeeding unless the mask is too small to cover the swiotlb
> bounce buffer area or the iommu virtual address space. This behavior
> is particularly important in case the bus address space is narrower
> than 32-bit, as we have to guarantee that the fallback to 32-bit
> DMA always succeeds. There are also a lot of drivers that try to
> set a 64-bit mask but don't implement bounce buffers for streaming
> mappings if that fails, and swiotlb is what we use to make those
> drivers work.
> 
> And yes, the API is a horrible mess.

With my patch applied and thus 32bit dma_mask set for NVMe device, I do
see high addresses passed to dma_map_*() routines and handled by
swiotlb. Thus your statement that behavior "succeed 64bit dma_set_mask()
operation but silently replace mask behind the scene" is required for
swiotlb to be used, does not match reality.

It can be interpreted as a breakage elsewhere, but it's hard to point
particular "root cause". The entire infrastructure to allocate and use
DMA memory is messy.

Still current code does not work, thus fix is needed.

Perhaps need to introduce some generic API to "allocate memory best
suited for DMA to particular device", and fix allocation points (in
drivers, filesystems, etc) to use it. Such an API could try to allocate
area that can be DMAed by hardware, and fallback to other memory that
can be used via swiotlb or other bounce buffer implementation.

But for now, have to stay with dma masks. Will follow-up with a patch
based on your but with coherent mask handling added.

Nikita

[toc] | [prev] | [next] | [standalone]

#1552843 — [PATCH] arm64: do not set dma masks that device connection can't handle

It is possible that device is capable of 64-bit DMA addresses, and
device driver tries to set wide DMA mask, but bridge or bus used to
connect device to the system can't handle wide addresses.

With swiotlb, memory above 4G still can be used by drivers for streaming
DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
that hardware handles physically.

This patch enforces that. Based on original version by
Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/Kconfig              |  3 +++
 arch/arm64/include/asm/device.h |  1 +
 arch/arm64/mm/dma-mapping.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..be3632e 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,31 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -367,8 +392,23 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	if (get_dma_ops(dev) == &swiotlb_dma_ops &&
+	    mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static int __init atomic_pool_init(void)
 {
 	pgprot_t prot = __pgprot(PROT_NORMAL_NC);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1552861 — [PATCH] arm64: do not set dma masks that device connection can't handle

It is possible that device is capable of 64-bit DMA addresses, and
device driver tries to set wide DMA mask, but bridge or bus used to
connect device to the system can't handle wide addresses.

With swiotlb, memory above 4G still can be used by drivers for streaming
DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
that hardware handles physically.

This patch enforces that. Based on original version by
Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
---

... now with initially missed change in arch_setup_dma_ops() ...

 arch/arm64/Kconfig              |  3 +++
 arch/arm64/include/asm/device.h |  1 +
 arch/arm64/mm/dma-mapping.c     | 52 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..09c7900 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,31 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -367,8 +392,23 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	if (get_dma_ops(dev) == &swiotlb_dma_ops &&
+	    mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static int __init atomic_pool_init(void)
 {
 	pgprot_t prot = __pgprot(PROT_NORMAL_NC);
@@ -957,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1553798 — Re: [PATCH] arm64: do not set dma masks that device connection can't handle

Hello!

On 1/6/2017 5:45 PM, Nikita Yushchenko wrote:

> It is possible that device is capable of 64-bit DMA addresses, and
> device driver tries to set wide DMA mask, but bridge or bus used to
> connect device to the system can't handle wide addresses.
>
> With swiotlb, memory above 4G still can be used by drivers for streaming
> DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
> that hardware handles physically.
>
> This patch enforces that. Based on original version by
> Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> CC: Arnd Bergmann <arnd@arndb.de>
[...]
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 290a84f..09c7900 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,31 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  	return 1;
>  }
>
> +static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
> +{
> +	/* device is not DMA capable */
> +	if (!dev->dma_mask)
> +		return -EIO;
> +
> +	/* mask is below swiotlb bounce buffer, so fail */
> +	if (!swiotlb_dma_supported(dev, mask))
> +		return -EIO;
> +
> +	/*
> +	 * because of the swiotlb, we can return success for
> +	 * larger masks, but need to ensure that bounce buffers
> +	 * are used above parent_dma_mask, so set that as
> +	 * the effective mask.
> +	 */
> +	if (mask > dev->archdata.parent_dma_mask)
> +		mask = dev->archdata.parent_dma_mask;
> +
> +

    One empty line is enough...

> +	*dev->dma_mask = mask;
> +
> +	return 0;
> +}
> +
>  static struct dma_map_ops swiotlb_dma_ops = {
>  	.alloc = __dma_alloc,
>  	.free = __dma_free,
[...]
> @@ -957,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	if (!dev->archdata.dma_ops)
>  		dev->archdata.dma_ops = &swiotlb_dma_ops;
>
> +	/*
> +	 * we don't yet support buses that have a non-zero mapping.
> +	 *  Let's hope we won't need it
> +	 */
> +	WARN_ON(dma_base != 0);
> +
> +	/*
> +	 * Whatever the parent bus can set. A device must not set
> +	 * a DMA mask larger than this.
> +	 */
> +	dev->archdata.parent_dma_mask = size;

    Not 'size - 1'?

> +
>  	dev->archdata.dma_coherent = coherent;
>  	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
>  }

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1554114 — Re: [PATCH] arm64: do not set dma masks that device connection can't handle

>> +    if (mask > dev->archdata.parent_dma_mask)
>> +        mask = dev->archdata.parent_dma_mask;
>> +
>> +
> 
>    One empty line is enough...

Ok

>> +    /*
>> +     * Whatever the parent bus can set. A device must not set
>> +     * a DMA mask larger than this.
>> +     */
>> +    dev->archdata.parent_dma_mask = size;
> 
>    Not 'size - 1'?

Good question.

Indeed of_dma_configure() calls arch_setup_dma_ops() with size, not
mask. Which implies '-1' is needed here. Although better fix may be to
change caller side - to make DMA_BIT_MASK(64) case cleaner.

Will repost path.

Nikita

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web