From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> To: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Robin Murphy <robin.murphy@arm.com>, Will Deacon <will.deacon@arm.com>, Robert Moore <robert.moore@intel.com>, Hanjun Guo <hanjun.guo@linaro.org>, Feng Kan <fkan@apm.com>, Jon Masters <jcm@redhat.com>, Zhang Rui <rui.zhang@intel.com>, Nate Watterson <nwatters@codeaurora.org> Subject: Re: [PATCH 3/4] ACPI: Introduce DMA ranges parsing Date: Mon, 24 Jul 2017 11:40:48 +0100 [thread overview] Message-ID: <20170724104048.GD13145@red-moon> (raw) In-Reply-To: <37547189.Yunjf1Ri5r@aspire.rjw.lan> On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote: [...] > > + * 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? Yes, it is better. > > + } > > + } 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); > > ? Yes I should have done that in the first place, will update. > > + > > + 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 ... I will update to it. > > + 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()? Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think this specific error condition is overkill TBH, the ACPI resource validation code should catch it before we even get here) not sure about downgrading it to _info() though, I would leave it at this loglevel - in particular in the offset check above: if (dma_offset && rentry->offset != dma_offset) { ret = -EINVAL; pr_warn("Can't handle multiple windows with different offsets\n"); goto out; } Thanks, Lorenzo
WARNING: multiple messages have this Message-ID (diff)
From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 3/4] ACPI: Introduce DMA ranges parsing Date: Mon, 24 Jul 2017 11:40:48 +0100 [thread overview] Message-ID: <20170724104048.GD13145@red-moon> (raw) In-Reply-To: <37547189.Yunjf1Ri5r@aspire.rjw.lan> On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote: [...] > > + * 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? Yes, it is better. > > + } > > + } 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); > > ? Yes I should have done that in the first place, will update. > > + > > + 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 ... I will update to it. > > + 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()? Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think this specific error condition is overkill TBH, the ACPI resource validation code should catch it before we even get here) not sure about downgrading it to _info() though, I would leave it at this loglevel - in particular in the offset check above: if (dma_offset && rentry->offset != dma_offset) { ret = -EINVAL; pr_warn("Can't handle multiple windows with different offsets\n"); goto out; } Thanks, Lorenzo
next prev parent reply other threads:[~2017-07-24 10:38 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-07-20 14:45 [PATCH 0/4] ACPI: DMA ranges management Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-20 14:45 ` [PATCH 1/4] ACPI: Allow _DMA method in walk resources Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-20 15:48 ` Moore, Robert 2017-07-20 15:48 ` Moore, Robert 2017-07-20 15:48 ` Moore, Robert 2017-07-20 15:50 ` Moore, Robert 2017-07-20 15:50 ` Moore, Robert 2017-07-20 15:50 ` Moore, Robert 2017-07-21 10:20 ` Lorenzo Pieralisi 2017-07-21 10:20 ` Lorenzo Pieralisi 2017-07-21 10:20 ` Lorenzo Pieralisi 2017-07-20 14:45 ` [PATCH 2/4] ACPI: Make acpi_dev_get_resources() method agnostic Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-21 22:05 ` Rafael J. Wysocki 2017-07-21 22:05 ` Rafael J. Wysocki 2017-07-24 9:22 ` Lorenzo Pieralisi 2017-07-24 9:22 ` Lorenzo Pieralisi 2017-07-25 9:15 ` Lorenzo Pieralisi 2017-07-25 9:15 ` Lorenzo Pieralisi 2017-07-26 0:23 ` Rafael J. Wysocki 2017-07-26 0:23 ` Rafael J. Wysocki 2017-07-20 14:45 ` [PATCH 3/4] ACPI: Introduce DMA ranges parsing Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-21 22:15 ` Rafael J. Wysocki 2017-07-21 22:15 ` Rafael J. Wysocki 2017-07-24 10:40 ` Lorenzo Pieralisi [this message] 2017-07-24 10:40 ` Lorenzo Pieralisi 2017-07-24 18:42 ` Rafael J. Wysocki 2017-07-24 18:42 ` Rafael J. Wysocki 2017-07-24 18:42 ` Rafael J. Wysocki 2017-07-25 9:06 ` Lorenzo Pieralisi 2017-07-25 9:06 ` Lorenzo Pieralisi 2017-07-25 9:06 ` Lorenzo Pieralisi 2017-07-26 0:27 ` Rafael J. Wysocki 2017-07-26 0:27 ` Rafael J. Wysocki 2017-07-26 0:27 ` Rafael J. Wysocki 2017-07-20 14:45 ` [PATCH 4/4] ACPI: Make acpi_dma_configure() DMA regions aware Lorenzo Pieralisi 2017-07-20 14:45 ` Lorenzo Pieralisi 2017-07-26 14:46 ` [PATCH 0/4] ACPI: DMA ranges management Nate Watterson 2017-07-26 14:46 ` Nate Watterson 2017-07-26 15:05 ` Robin Murphy 2017-07-26 15:05 ` Robin Murphy 2017-07-26 15:35 ` Lorenzo Pieralisi 2017-07-26 15:35 ` Lorenzo Pieralisi 2017-07-26 16:39 ` Nate Watterson 2017-07-26 16:39 ` Nate Watterson 2017-07-28 13:08 ` Robin Murphy 2017-07-28 13:08 ` Robin Murphy 2017-07-28 14:09 ` Lorenzo Pieralisi 2017-07-28 14:09 ` Lorenzo Pieralisi 2017-07-28 15:55 ` Robin Murphy 2017-07-28 15:55 ` Robin Murphy 2017-07-31 8:56 ` Lorenzo Pieralisi 2017-07-31 8:56 ` Lorenzo Pieralisi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170724104048.GD13145@red-moon \ --to=lorenzo.pieralisi@arm.com \ --cc=fkan@apm.com \ --cc=hanjun.guo@linaro.org \ --cc=jcm@redhat.com \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nwatters@codeaurora.org \ --cc=rjw@rjwysocki.net \ --cc=robert.moore@intel.com \ --cc=robin.murphy@arm.com \ --cc=rui.zhang@intel.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.