Date: Thu, 28 Feb 2008 00:13:58 +0200 From: Andriy Gapon <avg@icyb.net.ua> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-acpi@freebsd.org Subject: Re: acpi_throttle: quirk based on pci info Message-ID: <47C5E0A6.5030102@icyb.net.ua> In-Reply-To: <200802271126.30132.jhb@freebsd.org> References: <47B96989.6070008@icyb.net.ua> <47C32D3C.2020205@icyb.net.ua> <47C5173E.40207@icyb.net.ua> <200802271126.30132.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
on 27/02/2008 18:26 John Baldwin said the following:
> On Wednesday 27 February 2008 02:54:38 am Andriy Gapon wrote:
[snip]
>> So what I am getting at - it is certainly not hard to make
>> acpi_pcib_acpi (and consecutively pci) be attached earlier than
>> acpi_cpu. All is needed is to add another special case in
>> acpi_probe_order after the existing ones, I actually did it as an
>> experiment:
>> else if (acpi_MatchHid(handle, "PNP0A03"))
>> *order = 4;
>> This worked for me as expected and without any side effects.
>>
>> But I have a big concern about whether we have any devices/drivers that
>> rely on a current typical order. That is, drivers that expect cpu
>> device/bus to be already probed and attached at the time when
>> probe/attach is called for something on pci bus.
>> I guess one example here is famous ichss that adds a child to cpu bus in
>> its pci probe routine, and calls device_probe_and_attach for it in the
>> same place too. I guess that while BUS_ADD_CHILD should work at that
>> place, device_probe_and_attach may not because cpu itself is not probed
>> and attached yet.
>>
>> On the other hand, as I said in the beginning, I think that even now
>> there are no strict guarantees about the order, it just happens to work.
>>
>> Anyway, I am not sure if the discussed change in ordering has any merit.
>> It seems that whatever the order it would be convenient in some places
>> and inconvenient in others, so some jumping through the hoops is inevitable.
>>
>> I wonder if it would be possible to create some mechanism for attaching
>> "important" devices first and then the rest later. E.g. cpu and pcib+pci
>> are attached first (in whatever order), then their children are attached
>> later (in whatever order). What is important is that all those "late"
>> children can expect both cpu and pci to be fully available.
>
> I actually have had patches for a while to make CPU devices attach in a
> certain order with respect to Host-PCI bridges precisely to help address
> this. What I did was to force CPUs to probe before PCI devices so then the
> ichss drivers current method (adding devices to CPUs) always works as the
> CPUs are around when ichss goes to attach devices to them.
>
> This is the patch I've had for a while. It also attaches Host-PCI bridges in
> a more deterministic order:
>
> --- //depot/vendor/freebsd/src/sys/dev/acpica/acpi.c 2007/10/09 07:52:34
> +++ //depot/user/jhb/acpipci/dev/acpica/acpi.c 2008/01/22 23:22:19
> @@ -1533,18 +1534,32 @@
> static int
> acpi_probe_order(ACPI_HANDLE handle, int *order)
> {
> + ACPI_OBJECT_TYPE type;
> + u_int addr;
>
> /*
> * 1. I/O port and memory system resource holders
> * 2. Embedded controllers (to handle early accesses)
> - * 3. PCI Link Devices
> + * 3. CPUs
> + * 4. PCI Link Devices
> + * 11 - 266. Host-PCI bridges sorted by _ADR
> */
> + AcpiGetType(handle, &type);
> if (acpi_MatchHid(handle, "PNP0C01") || acpi_MatchHid(handle, "PNP0C02"))
> *order = 1;
> else if (acpi_MatchHid(handle, "PNP0C09"))
> *order = 2;
> + else if (type == ACPI_TYPE_PROCESSOR)
> + *order = 3;
> else if (acpi_MatchHid(handle, "PNP0C0F"))
> - *order = 3;
> + *order = 4;
> + else if (acpi_MatchHid(handle, "PNP0A03")) {
> + if (ACPI_SUCCESS(acpi_GetInteger(handle, "_ADR", &addr)))
> + *order = 11 + ACPI_ADR_PCI_SLOT(addr) * (PCI_FUNCMAX + 1) +
> + ACPI_ADR_PCI_FUNC(addr);
> + else
> + *order = 11;
> + }
> return (0);
> }
>
> @@ -1591,14 +1606,16 @@
> break;
>
> /*
> - * Create a placeholder device for this node. Sort the placeholder
> - * so that the probe/attach passes will run breadth-first. Orders
> - * less than ACPI_DEV_BASE_ORDER are reserved for special objects
> - * (i.e., system resources). Larger values are used for all other
> - * devices.
> + * Create a placeholder device for this node. Sort the
> + * placeholder so that the probe/attach passes will run
> + * breadth-first. Orders less than ACPI_DEV_BASE_ORDER
> + * are reserved for special objects (i.e., system
> + * resources). Values between ACPI_DEV_BASE_ORDER and 300
> + * are reserved for Host-PCI bridges. Larger values are
> + * used for all other devices.
> */
> ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "scanning '%s'\n", handle_str));
> - order = (level + 1) * ACPI_DEV_BASE_ORDER;
> + order = level * 10 + 300;
> acpi_probe_order(handle, &order);
> child = BUS_ADD_CHILD(bus, order, NULL, -1);
> if (child == NULL)
>
> With this patch PCI drivers that want to work with CPU devices would use the
> same approach as ichss. I'm not sure that is the best approach though. The
> fact that smist0 wants to check if ichss0 is attached probably wouldn't work
> as with this patch smist0 would attach first before ichss0 got a chance to
> probe via PCI.
>
> We could flip this around to have PCI bridges first before CPUs. As you
> suggest. However, that will break ichss in its current form as cpu0 won't
> exist yet (the device wouldn't be probed, so it would be called cpu0 yet).
>
> This is begging for multi-pass. :( (But that's another topic).
>
> In practice, I think you could change ichss to not use a PCI probe, but to
> just read config registers directly and hardcode the address of the ICH. You
> could even walk PCI bus 0 looking for a PCI-ISA bridge instead of hardcoding
> the address I think. Then ichss would just be a CPU device and not depend on
> the PCI bus at all. I'm not sure if that is workable for the other case you
> are worried about.
>
> BTW, once an order is decided upon of CPUs relative to Host-PCI bridges we can
> fix that order for the non-ACPI case easily enough in legacy.c.
>
John,
thank you for this detailed reply!
I looked through the code and I think that ichss is the only device that
explicitly requires cpu and pci buses to be "configured". acpi_throttle
is another one that implicitly requires that.
My personal preference is to probe/attach pci first and then go with
cpu. This is mostly because pci can provide a lot of useful information
and resources to various devices. On the other hand, cpu mostly exists
so that others could attach to it (it does provide a little bit, but
it's a very little bit). So, in my opinion, it is more likely that a
child of cpu would need something from pci than vice versa.
If we agree on this order and implement it, then I agree with you that
it would be quite easy to modify ichss to be a "normal" child of cpu and
use pci_find_device to find a proper pci device. And the rest of the
code that uses pci_read_config, bus_set_resource and
bus_alloc_resource_any would remain practically the same.
I'd even say that this would be a trivial change.
And I'd even say that this would be a change in right direction, because
ichss would lose most of its 'specialness'.
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47C5E0A6.5030102>