acpi_throttle: quirk based on pci info

[Andriy Gapon]

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