Adding a "-1" into https://reviews.freebsd.org/D25219 's code looks to make uefi/ACPI handle USB3 SSD reliably
Mark Millard
marklmi at yahoo.com
Mon Oct 12 17:45:37 UTC 2020
On 2020-Oct-7, at 00:26, Mark Millard <marklmi at yahoo.com> wrote:
> https://reviews.freebsd.org/D25219 has (in part)
>
>> + if (bus_dma_tag_create(NULL, 1, 0,
>> + limits.lowaddr, BUS_SPACE_MAXADDR, NULL, NULL,
>
> Based on sysctl hw.busdma output and testing a change for the u-boot/DTB/fdt code context that worked in testing so far, I've tried using limits.lowaddr-1 in the above ACPI handling code instead.
>
> It has worked so far.
>
> So both existing implementations have the same basic
> problem as far as I can tell: limits.lowaddr too large
> by one, so identifying the wrong page.
>
>
>
> What sysctl showed me was the likes of (before
> changes that lead to lack of zone2 for u-boot/dtb/fdt):
>
> . . .
> hw.busdma.zone2.lowaddr: 0x3c000fff
> . . .
> hw.busdma.zone1.lowaddr: 0x3fffffff
> . . .
> hw.busdma.zone0.lowaddr: 0xffffffff
> . . .
>
> So I've guessed that lowaddr should identify the
> end page of the possibly-use-it-directly region,
> not the first do-not-use-it-directly page. If I've
> guessed wrong, at most it would bounce one page
> that it could avoid bouncing. But, if I guessed
> correct, it might bounce a page that it should
> instead of not doing so. Thus the "-1" addition.
>
> For reference, after the first duplicate-and-diff test
> for uefi/ACPI:
>
> # sysctl hw.busdma
> hw.busdma.zone0.alignment: 4096
> hw.busdma.zone0.lowaddr: 0xbfffffff
> hw.busdma.zone0.total_deferred: 0
> hw.busdma.zone0.total_bounced: 762568
> hw.busdma.zone0.active_bpages: 12
> hw.busdma.zone0.reserved_bpages: 0
> hw.busdma.zone0.free_bpages: 824
> hw.busdma.zone0.total_bpages: 836
> hw.busdma.total_bpages: 836
>
> I'll note that "total_bounced" need not be the
> a page count: it is incremented by 1 after
> the loop for a bounce, not inside the loop.
> Lots of pages of data were bounced.
For reference, the patch that I'm running is below.
It has a comment and the addition of the "-1" compared
to what is in https://reviews.freebsd.org/D25219 .
I've not observed any problems so far in my use.
(E-mail may make whitespace odd below.)
# svnlite diff /usr/src/sys/dev/acpica/acpi.c
Index: /usr/src/sys/dev/acpica/acpi.c
===================================================================
--- /usr/src/sys/dev/acpica/acpi.c (revision 365932)
+++ /usr/src/sys/dev/acpica/acpi.c (working copy)
@@ -184,6 +184,7 @@
static void acpi_hint_device_unit(device_t acdev, device_t child,
const char *name, int *unitp);
static void acpi_reset_interfaces(device_t dev);
+static bus_dma_tag_t acpi_get_dma_tag(device_t dev, device_t child);
static device_method_t acpi_methods[] = {
/* Device interface */
@@ -218,6 +219,7 @@
DEVMETHOD(bus_hint_device_unit, acpi_hint_device_unit),
DEVMETHOD(bus_get_cpus, acpi_get_cpus),
DEVMETHOD(bus_get_domain, acpi_get_domain),
+ DEVMETHOD(bus_get_dma_tag, acpi_get_dma_tag),
/* ACPI bus */
DEVMETHOD(acpi_id_probe, acpi_device_id_probe),
@@ -431,6 +433,123 @@
return (0);
}
+struct dma_limits {
+ bus_addr_t lowaddr;
+};
+
+static ACPI_STATUS
+dma_on_resource(ACPI_RESOURCE *res, void *arg)
+{
+ struct dma_limits *limits = arg;
+ bus_addr_t min, len;
+
+ /*
+ * The minimum and maximum are device-side. To get the CPU-side minimum,
+ * we add the translation offset. This can overflow to signify lower addresses
+ * on the CPU than the device, e.g. "Bus 0xC0000000 -> CPU 0x00000000"
+ * on the RPi4 is represented as 0xC0000000 min + 0xFFFFFFFF40000000 offset.
+ */
+
+ switch (res->Type) {
+ case ACPI_RESOURCE_TYPE_ADDRESS16:
+ min = (uint16_t)(res->Data.Address16.Address.Minimum +
+ res->Data.Address16.Address.TranslationOffset);
+ len = res->Data.Address16.Address.AddressLength;
+ break;
+ case ACPI_RESOURCE_TYPE_ADDRESS32:
+ min = (uint32_t)(res->Data.Address32.Address.Minimum +
+ res->Data.Address32.Address.TranslationOffset);
+ len = res->Data.Address32.Address.AddressLength;
+ break;
+ case ACPI_RESOURCE_TYPE_ADDRESS64:
+ min = (uint64_t)(res->Data.Address64.Address.Minimum +
+ res->Data.Address64.Address.TranslationOffset);
+ len = res->Data.Address64.Address.AddressLength;
+ break;
+ case ACPI_RESOURCE_TYPE_END_TAG:
+ return (AE_OK);
+ default:
+ printf("ACPI: warning: DMA limit with unsupported resource type %d\n",
+ res->Type);
+ return (AE_OK);
+ }
+
+ if (min != 0)
+ printf("ACPI: warning: DMA limit with non-zero minimum address"
+ " not supported yet\n");
+
+ limits->lowaddr = MIN(limits->lowaddr, min + len);
+
+ return (AE_OK);
+}
+
+static int
+get_dma_tag(ACPI_HANDLE handle, bus_dma_tag_t *result)
+{
+ ACPI_HANDLE parent;
+ unsigned int coherent;
+ struct dma_limits limits = {
+ .lowaddr = BUS_SPACE_MAXADDR,
+ };
+
+ if (ACPI_FAILURE(AcpiWalkResources(handle, "_DMA",
+ dma_on_resource, (void *)&limits))) {
+ /* Inherit resources from parent handle if we don't have our own */
+ if (ACPI_SUCCESS(AcpiGetParent(handle, &parent)))
+ return (get_dma_tag(parent, result));
+
+ /* The root (which has no parent) has no restrictions */
+ *result = NULL;
+ return (0);
+ }
+
+ if (ACPI_FAILURE(acpi_GetInteger(handle, "_CCA", &coherent)))
+ coherent = 0;
+
+ /*
+ * First off, a note about lowaddr values. What sysctl showed
+ * me was (during investigation of u-boot/dtb/fdt oddities
+ * that had the same style of solution that I am trying here):
+ *
+ * . . .
+ * hw.busdma.zone2.lowaddr: 0x3c000fff
+ * . . .
+ * hw.busdma.zone1.lowaddr: 0x3fffffff
+ * . . .
+ * hw.busdma.zone0.lowaddr: 0xffffffff
+ * . . .
+ *
+ * So I've guessed that lowaddr should identify the
+ * end page of the possibly-use-it region, not the
+ * first do-not-use-it page. If I've guessed wrong,
+ * at most it would bounce one page that it could
+ * avoid bouncing. But, if I guessed correct, it
+ * might bounce a page that it should instead of
+ * not doing so. Thus the "-1" below.
+ */
+ if (bus_dma_tag_create(NULL, 1, 0,
+ limits.lowaddr-1, BUS_SPACE_MAXADDR, NULL, NULL,
+ BUS_SPACE_MAXSIZE, BUS_SPACE_UNRESTRICTED, BUS_SPACE_MAXSIZE,
+ coherent ? BUS_DMA_COHERENT : 0, NULL, NULL,
+ result) != 0)
+ return (ENOMEM);
+
+ return (0);
+}
+
+static bus_dma_tag_t
+acpi_get_dma_tag(device_t dev, device_t child)
+{
+ bus_dma_tag_t result;
+
+ if (get_dma_tag(acpi_get_handle(child), &result) != 0) {
+ device_printf(child, "could not get ACPI DMA limits\n");
+ return (NULL);
+ }
+
+ return (result);
+}
+
/*
* Fetch some descriptive data from ACPI to put in our attach message.
*/
I later found the code that confirms the need to do the "-1" . . .
sys/arm64/arm64/busdma_machdep.c 's common_bus_dma_tag_create has:
common->lowaddr = trunc_page((vm_paddr_t)lowaddr) + (PAGE_SIZE - 1);
common->highaddr = trunc_page((vm_paddr_t)highaddr) + (PAGE_SIZE - 1);
and so forces reference to the last byte of the page identified by lowaddr
(and highaddr). Thus lowaddr needs to identify the last page that can avoid
bouncing (or earlier).
===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
More information about the freebsd-arm
mailing list