| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37547189.Yunjf1Ri5r@aspire.rjw.lan>
Date: Sat, 22 Jul 2017 00:15:42 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc: linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, Robin Murphy <robin.murphy@....com>,
Will Deacon <will.deacon@....com>,
Robert Moore <robert.moore@...el.com>,
Hanjun Guo <hanjun.guo@...aro.org>, Feng Kan <fkan@....com>,
Jon Masters <jcm@...hat.com>, Zhang Rui <rui.zhang@...el.com>,
Nate Watterson <nwatters@...eaurora.org>
Subject: Re: [PATCH 3/4] ACPI: Introduce DMA ranges parsing
On Thursday, July 20, 2017 03:45:15 PM Lorenzo Pieralisi wrote:
> Some devices have limited addressing capabilities and cannot
> reference the whole memory address space while carrying out DMA
> operations (eg some devices with bus address bits range smaller than
> system bus - which prevents them from using bus addresses that are
> otherwise valid for the system).
>
> The ACPI _DMA object allows bus devices to define the DMA window that is
> actually addressable by devices that sit upstream the bus, therefore
> providing a means to parse and initialize the devices DMA masks and
> addressable DMA range size.
>
> By relying on the generic ACPI kernel layer to retrieve and parse
> resources, introduce ACPI core code to parse the _DMA object.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> ---
> drivers/acpi/resource.c | 35 +++++++++++++++++++++
> drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 2 ++
> include/linux/acpi.h | 8 +++++
> 4 files changed, 128 insertions(+)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 2b20a09..9602248 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -636,6 +636,41 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> }
> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>
> +static int is_memory(struct acpi_resource *ares, void *not_used)
> +{
> + struct resource_win win;
> + struct resource *res = &win.res;
> +
> + memset(&win, 0, sizeof(win));
> +
> + return !(acpi_dev_resource_memory(ares, res)
> + || acpi_dev_resource_address_space(ares, &win)
> + || acpi_dev_resource_ext_address_space(ares, &win));
> +}
> +
> +/**
> + * acpi_dev_get_dma_resources - Get current DMA resources of a device.
> + * @adev: ACPI device node to get the resources for.
> + * @list: Head of the resultant list of resources (must be empty).
> + *
> + * Evaluate the _DMA method for the given device node and process its
> + * output.
> + *
> + * The resultant struct resource objects are put on the list pointed to
> + * by @list, that must be empty initially, as members of struct
> + * resource_entry objects. Callers of this routine should use
> + * %acpi_dev_free_resource_list() to free that list.
> + *
> + * The number of resources in the output list is returned on success,
> + * an error code reflecting the error condition is returned otherwise.
> + */
> +int acpi_dev_get_dma_resources(struct acpi_device *adev, struct list_head *list)
> +{
> + return acpi_dev_get_resources_method(adev, list, is_memory, NULL,
> + METHOD_NAME__DMA);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_get_dma_resources);
> +
> /**
> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> * types
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 3389729..eb493c2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1360,6 +1360,89 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
> }
>
> /**
> + * acpi_dma_get_range() - Get device DMA parameters.
> + *
> + * @dev: device to configure
> + * @dma_addr: pointer device DMA address result
> + * @offset: pointer to the DMA offset result
> + * @size: pointer to DMA range size result
> + *
> + * Evaluate DMA regions and return respectively DMA region start, offset
> + * and size in dma_addr, offset and size on parsing success; it does not
> + * update the passed in values on failure.
> + *
> + * Return 0 on success, < 0 on failure.
> + */
> +int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
> + u64 *size)
> +{
> + struct acpi_device *adev;
> + LIST_HEAD(list);
> + struct resource_entry *rentry;
> + int ret;
> + struct device *dma_dev = dev;
> + struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> +
> + /*
> + * Walk the device tree chasing an ACPI companion with a _DMA
> + * object while we go. Stop if we find a device with an ACPI
> + * companion containing a _DMA method.
> + */
> + do {
> + if (has_acpi_companion(dma_dev)) {
> + adev = ACPI_COMPANION(dma_dev);
> +
> + if (acpi_has_method(adev->handle, METHOD_NAME__DMA))
> + break;
Why don't you do
adev = ACPI_COMPANION(dma_dev);
if (adev && acpi_has_method(adev->handle, METHOD_NAME__DMA))
break;
instead?
> + }
> + } while ((dma_dev = dma_dev->parent));
We had a rule to avoid things like this once and it wasn't a bad one. :-)
Why don't you just do
dma_dev = dma_dev->parent;
} while (dma_dev);
?
> +
> + if (!dma_dev)
> + return -ENODEV;
> +
> + if (!acpi_has_method(adev->handle, METHOD_NAME__CRS)) {
> + acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &name_buffer);
> + pr_warn(FW_BUG "%s: _DMA object only valid in object with valid _CRS\n",
> + (char *)name_buffer.pointer);
> + kfree(name_buffer.pointer);
We have acpi_handle_warn() and friends for stuff like that ...
> + return -EINVAL;
> + }
> +
> + ret = acpi_dev_get_dma_resources(adev, &list);
> + if (ret > 0) {
> + list_for_each_entry(rentry, &list, node) {
> + if (dma_offset && rentry->offset != dma_offset) {
> + ret = -EINVAL;
> + pr_warn("Can't handle multiple windows with different offsets\n");
> + goto out;
> + }
> + dma_offset = rentry->offset;
> +
> + /* Take lower and upper limits */
> + if (rentry->res->start < dma_start)
> + dma_start = rentry->res->start;
> + if (rentry->res->end > dma_end)
> + dma_end = rentry->res->end;
> + }
> +
> + if (dma_start >= dma_end) {
> + ret = -EINVAL;
> + pr_warn("Invalid DMA regions configuration\n");
dev_warn()?
And why _warn() and not _info()?
> + goto out;
> + }
> +
> + *dma_addr = dma_start - dma_offset;
> + *size = dma_end - dma_start + 1;
> + *offset = dma_offset;
> + }
> + out:
> + acpi_dev_free_resource_list(&list);
> +
> + return ret >= 0 ? 0 : ret;
> +}
Thanks,
Rafael
Powered by blists - more mailing lists