| Message ID | 20200404073047.17898-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 7fb89e1d44cb6aec342e5cca6ed6371d818a428c |
| Headers | show |
| Series | arm64: iort: take _DMA methods into account for named components | expand |
On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > Where IORT nodes for named components can describe simple DMA limits > expressed as the number of address bits a device can driver, _DMA methods > in AML can express more complex topologies, involving DMA translation in > particular. > > Currently, we only take this _DMA method into account if it appears on a > ACPI device node describing a PCIe root complex, but it is perfectly > acceptable to attach them to named components as well, so let's ensure > we take them into account in those cases too. ACPI spec v6.3, 6.2.4 _DMA: "_DMA is only defined under devices that represent buses" This should probably be updated and _DMA usage clarified - we can't leave it open to interpretation. Thanks, Lorenzo > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > Cc: <stable@vger.kernel.org> # v4.14+ > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/acpi/arm64/iort.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index ed3d2d1a7ae9..07eb78baf198 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1146,13 +1146,10 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > else > size = 1ULL << 32; > > - if (dev_is_pci(dev)) { > - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > - if (ret == -ENODEV) > - ret = rc_dma_get_range(dev, &size); > - } else { > - ret = nc_dma_get_range(dev, &size); > - } > + ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > + if (ret == -ENODEV) > + ret = dev_is_pci(dev) ? rc_dma_get_range(dev, &size) > + : nc_dma_get_range(dev, &size); > > if (!ret) { > /* > -- > 2.17.1 >
On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > Where IORT nodes for named components can describe simple DMA limits > > expressed as the number of address bits a device can driver, _DMA methods > > in AML can express more complex topologies, involving DMA translation in > > particular. > > > > Currently, we only take this _DMA method into account if it appears on a > > ACPI device node describing a PCIe root complex, but it is perfectly > > acceptable to attach them to named components as well, so let's ensure > > we take them into account in those cases too. > > ACPI spec v6.3, 6.2.4 _DMA: > > "_DMA is only defined under devices that represent buses" > Sure. But ACPI0004 module devices are also bus nodes, so that statement does not exclude named components that are defined under such a module device. > This should probably be updated and _DMA usage clarified - we can't > leave it open to interpretation. > Clarification is always better.
On Mon, Apr 06, 2020 at 01:16:15PM +0200, Ard Biesheuvel wrote: > On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > Where IORT nodes for named components can describe simple DMA limits > > > expressed as the number of address bits a device can driver, _DMA methods > > > in AML can express more complex topologies, involving DMA translation in > > > particular. > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > acceptable to attach them to named components as well, so let's ensure > > > we take them into account in those cases too. > > > > ACPI spec v6.3, 6.2.4 _DMA: > > > > "_DMA is only defined under devices that represent buses" > > > > Sure. But ACPI0004 module devices are also bus nodes, so that > statement does not exclude named components that are defined under > such a module device. Yes. _DMA is valid only if a _CRS is present, ACPI0004 does not seem to _require_ a _CRS to be considered valid. It is an example. Better to make the _DMA definition more robust and linked to ACPI0004, for instance. > > This should probably be updated and _DMA usage clarified - we can't > > leave it open to interpretation. > > > > Clarification is always better. Yes, we should be able to get this in as an Errata, better to be clear given that it is something that will be used extensively across platforms. Thanks ! Lorenzo
On Mon, 6 Apr 2020 at 13:32, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Apr 06, 2020 at 01:16:15PM +0200, Ard Biesheuvel wrote: > > On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > Where IORT nodes for named components can describe simple DMA limits > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > in AML can express more complex topologies, involving DMA translation in > > > > particular. > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > acceptable to attach them to named components as well, so let's ensure > > > > we take them into account in those cases too. > > > > > > ACPI spec v6.3, 6.2.4 _DMA: > > > > > > "_DMA is only defined under devices that represent buses" > > > > > > > Sure. But ACPI0004 module devices are also bus nodes, so that > > statement does not exclude named components that are defined under > > such a module device. > > Yes. _DMA is valid only if a _CRS is present, ACPI0004 does not seem > to _require_ a _CRS to be considered valid. > How is that relevant? Any node that has a _DMA must have a _CRS as well. Some nodes that don't have a _DMA may not have a _CRS either. But that does not disqualify a ACPI0004 that *does* have both from being considered a bus node, no? Or is that not what you are saying? > It is an example. Better to make the _DMA definition more robust > and linked to ACPI0004, for instance. > If there is wording in the spec that says that only APCI0004 or PNP0A03/PNP0A08 should be considered to be bus nodes, then we should probably do that. But I wonder if that is really the intent, and whether vendors could denote bus nodes using their own HID namespace instead. > > > This should probably be updated and _DMA usage clarified - we can't > > > leave it open to interpretation. > > > > > > > Clarification is always better. > > Yes, we should be able to get this in as an Errata, better to be clear > given that it is something that will be used extensively across > platforms. >
On Mon, Apr 06, 2020 at 01:59:07PM +0200, Ard Biesheuvel wrote: > On Mon, 6 Apr 2020 at 13:32, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Mon, Apr 06, 2020 at 01:16:15PM +0200, Ard Biesheuvel wrote: > > > On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > > Where IORT nodes for named components can describe simple DMA limits > > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > > in AML can express more complex topologies, involving DMA translation in > > > > > particular. > > > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > > acceptable to attach them to named components as well, so let's ensure > > > > > we take them into account in those cases too. > > > > > > > > ACPI spec v6.3, 6.2.4 _DMA: > > > > > > > > "_DMA is only defined under devices that represent buses" > > > > > > > > > > Sure. But ACPI0004 module devices are also bus nodes, so that > > > statement does not exclude named components that are defined under > > > such a module device. > > > > Yes. _DMA is valid only if a _CRS is present, ACPI0004 does not seem > > to _require_ a _CRS to be considered valid. > > > > How is that relevant? Any node that has a _DMA must have a _CRS as > well. Some nodes that don't have a _DMA may not have a _CRS either. > But that does not disqualify a ACPI0004 that *does* have both from > being considered a bus node, no? Or is that not what you are saying? I am just trying to prevent firmware developers from adding ACPI0004 nodes with *just* a _DMA object (because the _CRS is optional) which will become unusable in OS context (where we do check the _CRS presence), that's all I am saying. Just trying to make specs foolproof :) Thanks, Lorenzo
On Mon, 6 Apr 2020 at 15:14, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Apr 06, 2020 at 01:59:07PM +0200, Ard Biesheuvel wrote: > > On Mon, 6 Apr 2020 at 13:32, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Mon, Apr 06, 2020 at 01:16:15PM +0200, Ard Biesheuvel wrote: > > > > On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi > > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > > > Where IORT nodes for named components can describe simple DMA limits > > > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > > > in AML can express more complex topologies, involving DMA translation in > > > > > > particular. > > > > > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > > > acceptable to attach them to named components as well, so let's ensure > > > > > > we take them into account in those cases too. > > > > > > > > > > ACPI spec v6.3, 6.2.4 _DMA: > > > > > > > > > > "_DMA is only defined under devices that represent buses" > > > > > > > > > > > > > Sure. But ACPI0004 module devices are also bus nodes, so that > > > > statement does not exclude named components that are defined under > > > > such a module device. > > > > > > Yes. _DMA is valid only if a _CRS is present, ACPI0004 does not seem > > > to _require_ a _CRS to be considered valid. > > > > > > > How is that relevant? Any node that has a _DMA must have a _CRS as > > well. Some nodes that don't have a _DMA may not have a _CRS either. > > But that does not disqualify a ACPI0004 that *does* have both from > > being considered a bus node, no? Or is that not what you are saying? > > I am just trying to prevent firmware developers from adding ACPI0004 > nodes with *just* a _DMA object (because the _CRS is optional) which > will become unusable in OS context (where we do check the _CRS > presence), that's all I am saying. > > Just trying to make specs foolproof :) > Ah ok, fair enough. Note that acpi_dma_get_range() already checks this, but on the firmware validation side, adding a rule like this would certainly help as well. I think the window for new ACPI material is closing atm - I'll check internally whether we can get someone to slip this in (i.e., a clarification added to '9.12 Module Device' that _DMA methods are permitted but only if _CRS is defined as well)
On Mon, 6 Apr 2020 at 15:19, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 6 Apr 2020 at 15:14, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Mon, Apr 06, 2020 at 01:59:07PM +0200, Ard Biesheuvel wrote: > > > On Mon, 6 Apr 2020 at 13:32, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > On Mon, Apr 06, 2020 at 01:16:15PM +0200, Ard Biesheuvel wrote: > > > > > On Mon, 6 Apr 2020 at 13:04, Lorenzo Pieralisi > > > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > > > > Where IORT nodes for named components can describe simple DMA limits > > > > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > > > > in AML can express more complex topologies, involving DMA translation in > > > > > > > particular. > > > > > > > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > > > > acceptable to attach them to named components as well, so let's ensure > > > > > > > we take them into account in those cases too. > > > > > > > > > > > > ACPI spec v6.3, 6.2.4 _DMA: > > > > > > > > > > > > "_DMA is only defined under devices that represent buses" > > > > > > > > > > > > > > > > Sure. But ACPI0004 module devices are also bus nodes, so that > > > > > statement does not exclude named components that are defined under > > > > > such a module device. > > > > > > > > Yes. _DMA is valid only if a _CRS is present, ACPI0004 does not seem > > > > to _require_ a _CRS to be considered valid. > > > > > > > > > > How is that relevant? Any node that has a _DMA must have a _CRS as > > > well. Some nodes that don't have a _DMA may not have a _CRS either. > > > But that does not disqualify a ACPI0004 that *does* have both from > > > being considered a bus node, no? Or is that not what you are saying? > > > > I am just trying to prevent firmware developers from adding ACPI0004 > > nodes with *just* a _DMA object (because the _CRS is optional) which > > will become unusable in OS context (where we do check the _CRS > > presence), that's all I am saying. > > > > Just trying to make specs foolproof :) > > > > Ah ok, fair enough. > > Note that acpi_dma_get_range() already checks this, but on the > firmware validation side, adding a rule like this would certainly help > as well. > > I think the window for new ACPI material is closing atm - I'll check > internally whether we can get someone to slip this in (i.e., a > clarification added to '9.12 Module Device' that _DMA methods are > permitted but only if _CRS is defined as well) So what is the verdict on this patch? We agree that the ACPI spec permits ACPI0004 containers with _CRS/_DMA method pairs, and child named components that rely on the described DMA translation to be honored by the OS. IOW, we should provide guidance to ensure that firmware providers don't get it wrong, but if they do get it right, we still need this change in order to take the translation into account.
On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > Where IORT nodes for named components can describe simple DMA limits > expressed as the number of address bits a device can driver, _DMA methods > in AML can express more complex topologies, involving DMA translation in > particular. > > Currently, we only take this _DMA method into account if it appears on a > ACPI device node describing a PCIe root complex, but it is perfectly > acceptable to attach them to named components as well, so let's ensure > we take them into account in those cases too. > > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > Cc: <stable@vger.kernel.org> # v4.14+ > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/acpi/arm64/iort.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) Only question is whether there is FW in the field with _DMA methods that now we would start parsing (and hopefully everything will still work) but for that the only choice is applying this patch and see, so: Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index ed3d2d1a7ae9..07eb78baf198 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1146,13 +1146,10 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > else > size = 1ULL << 32; > > - if (dev_is_pci(dev)) { > - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > - if (ret == -ENODEV) > - ret = rc_dma_get_range(dev, &size); > - } else { > - ret = nc_dma_get_range(dev, &size); > - } > + ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > + if (ret == -ENODEV) > + ret = dev_is_pci(dev) ? rc_dma_get_range(dev, &size) > + : nc_dma_get_range(dev, &size); > > if (!ret) { > /* > -- > 2.17.1 >
On Mon, 20 Apr 2020 at 10:41, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > Where IORT nodes for named components can describe simple DMA limits > > expressed as the number of address bits a device can driver, _DMA methods > > in AML can express more complex topologies, involving DMA translation in > > particular. > > > > Currently, we only take this _DMA method into account if it appears on a > > ACPI device node describing a PCIe root complex, but it is perfectly > > acceptable to attach them to named components as well, so let's ensure > > we take them into account in those cases too. > > > > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > > Cc: <stable@vger.kernel.org> # v4.14+ > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/acpi/arm64/iort.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > Only question is whether there is FW in the field with _DMA methods that > now we would start parsing (and hopefully everything will still work) > but for that the only choice is applying this patch and see, so: > Perhaps it would be better to call acpi_dma_get_range() on dev->parent then? > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index ed3d2d1a7ae9..07eb78baf198 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -1146,13 +1146,10 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > else > > size = 1ULL << 32; > > > > - if (dev_is_pci(dev)) { > > - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > > - if (ret == -ENODEV) > > - ret = rc_dma_get_range(dev, &size); > > - } else { > > - ret = nc_dma_get_range(dev, &size); > > - } > > + ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > > + if (ret == -ENODEV) > > + ret = dev_is_pci(dev) ? rc_dma_get_range(dev, &size) > > + : nc_dma_get_range(dev, &size); > > > > if (!ret) { > > /* > > -- > > 2.17.1 > >
On Mon, Apr 20, 2020 at 10:58:02AM +0200, Ard Biesheuvel wrote: > On Mon, 20 Apr 2020 at 10:41, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > Where IORT nodes for named components can describe simple DMA limits > > > expressed as the number of address bits a device can driver, _DMA methods > > > in AML can express more complex topologies, involving DMA translation in > > > particular. > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > acceptable to attach them to named components as well, so let's ensure > > > we take them into account in those cases too. > > > > > > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > > > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > drivers/acpi/arm64/iort.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > Only question is whether there is FW in the field with _DMA methods that > > now we would start parsing (and hopefully everything will still work) > > but for that the only choice is applying this patch and see, so: > > > > Perhaps it would be better to call acpi_dma_get_range() on dev->parent then? I think it is fine as it is - maybe we can hold off sending it all the way to stable kernels until we are confident it does not cause unintended breakage ? Anyway, thanks for putting it together. Minor nit: I'd make "arm64: iort:" in the subject "ACPI/IORT:" just to keep logs uniform. Thanks, Lorenzo > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > > index ed3d2d1a7ae9..07eb78baf198 100644 > > > --- a/drivers/acpi/arm64/iort.c > > > +++ b/drivers/acpi/arm64/iort.c > > > @@ -1146,13 +1146,10 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > > else > > > size = 1ULL << 32; > > > > > > - if (dev_is_pci(dev)) { > > > - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > > > - if (ret == -ENODEV) > > > - ret = rc_dma_get_range(dev, &size); > > > - } else { > > > - ret = nc_dma_get_range(dev, &size); > > > - } > > > + ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > > > + if (ret == -ENODEV) > > > + ret = dev_is_pci(dev) ? rc_dma_get_range(dev, &size) > > > + : nc_dma_get_range(dev, &size); > > > > > > if (!ret) { > > > /* > > > -- > > > 2.17.1 > > >
On Mon, 20 Apr 2020 at 11:13, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Apr 20, 2020 at 10:58:02AM +0200, Ard Biesheuvel wrote: > > On Mon, 20 Apr 2020 at 10:41, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > Where IORT nodes for named components can describe simple DMA limits > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > in AML can express more complex topologies, involving DMA translation in > > > > particular. > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > acceptable to attach them to named components as well, so let's ensure > > > > we take them into account in those cases too. > > > > > > > > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > > > > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > --- > > > > drivers/acpi/arm64/iort.c | 11 ++++------- > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > Only question is whether there is FW in the field with _DMA methods that > > > now we would start parsing (and hopefully everything will still work) > > > but for that the only choice is applying this patch and see, so: > > > > > > > Perhaps it would be better to call acpi_dma_get_range() on dev->parent then? > > I think it is fine as it is - maybe we can hold off sending it all > the way to stable kernels until we are confident it does not cause > unintended breakage ? > > Anyway, thanks for putting it together. > > Minor nit: I'd make "arm64: iort:" in the subject "ACPI/IORT:" > just to keep logs uniform. > OK, I'll respin and resend, with the ACPI folks on cc this time. Thanks
On Mon, Apr 20, 2020 at 11:14:50AM +0200, Ard Biesheuvel wrote: > On Mon, 20 Apr 2020 at 11:13, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Mon, Apr 20, 2020 at 10:58:02AM +0200, Ard Biesheuvel wrote: > > > On Mon, 20 Apr 2020 at 10:41, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > On Sat, Apr 04, 2020 at 09:30:47AM +0200, Ard Biesheuvel wrote: > > > > > Where IORT nodes for named components can describe simple DMA limits > > > > > expressed as the number of address bits a device can driver, _DMA methods > > > > > in AML can express more complex topologies, involving DMA translation in > > > > > particular. > > > > > > > > > > Currently, we only take this _DMA method into account if it appears on a > > > > > ACPI device node describing a PCIe root complex, but it is perfectly > > > > > acceptable to attach them to named components as well, so let's ensure > > > > > we take them into account in those cases too. > > > > > > > > > > Reported-by: Andrei Warkentin <awarkentin@vmware.com> > > > > > Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") > > > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > --- > > > > > drivers/acpi/arm64/iort.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > Only question is whether there is FW in the field with _DMA methods that > > > > now we would start parsing (and hopefully everything will still work) > > > > but for that the only choice is applying this patch and see, so: > > > > > > > > > > Perhaps it would be better to call acpi_dma_get_range() on dev->parent then? > > > > I think it is fine as it is - maybe we can hold off sending it all > > the way to stable kernels until we are confident it does not cause > > unintended breakage ? > > > > Anyway, thanks for putting it together. > > > > Minor nit: I'd make "arm64: iort:" in the subject "ACPI/IORT:" > > just to keep logs uniform. > > > > OK, I'll respin and resend, with the ACPI folks on cc this time. Thanks. I'm happy to queue this in the arm64 tree with the CC: stable dropped for now, so please keep me on cc for v2. Will
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index ed3d2d1a7ae9..07eb78baf198 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -1146,13 +1146,10 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) else size = 1ULL << 32; - if (dev_is_pci(dev)) { - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); - if (ret == -ENODEV) - ret = rc_dma_get_range(dev, &size); - } else { - ret = nc_dma_get_range(dev, &size); - } + ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); + if (ret == -ENODEV) + ret = dev_is_pci(dev) ? rc_dma_get_range(dev, &size) + : nc_dma_get_range(dev, &size); if (!ret) { /*
Where IORT nodes for named components can describe simple DMA limits expressed as the number of address bits a device can driver, _DMA methods in AML can express more complex topologies, involving DMA translation in particular. Currently, we only take this _DMA method into account if it appears on a ACPI device node describing a PCIe root complex, but it is perfectly acceptable to attach them to named components as well, so let's ensure we take them into account in those cases too. Reported-by: Andrei Warkentin <awarkentin@vmware.com> Fixes: 7ad4263980826e8b ("ACPI: Make acpi_dma_configure() DMA regions aware") Cc: <stable@vger.kernel.org> # v4.14+ Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/acpi/arm64/iort.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)