Re: [PATCH v15 7/9] PCI: Setup ACPI fwnode early and at the same time with OF

[Shanker R Donthineni]

Hi Bjorn,

On 8/13/21 11:10 PM, Bjorn Helgaas wrote:
>>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>>> index eaddbf701759..dae021322b3f 100644
>>>> --- a/drivers/pci/pci-acpi.c
>>>> +++ b/drivers/pci/pci-acpi.c
>>>> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
>>>>               return false;
>>>>
>>>>       /* Assume D3 support if the bridge is power-manageable by ACPI. */
>>>> -     pci_set_acpi_fwnode(dev);
>>>>       adev = ACPI_COMPANION(&dev->dev);
>>> I *think* the Root Port code farther down in this function is also now
>>> unnecessary:
>>>
>>>   acpi_pci_bridge_d3(...)
>>>   {
>>>     ...
>>>     root = pcie_find_root_port(dev);
>>>     adev = ACPI_COMPANION(&root->dev);
>>>     if (root == dev) {
>>>       /*
>>>        * It is possible that the ACPI companion is not yet bound
>>>        * for the root port so look it up manually here.
>>>        */
>>>       if (!adev && !pci_dev_is_added(root))
>>>         adev = acpi_pci_find_companion(&root->dev);
>>>     }
>>>
>>> Since we're now setting the ACPI_COMPANION for every pci_dev long
>>> before we get here, I think this could now be simplified to something
>>> like this:
>>>
>>>   acpi_pci_bridge_d3(...)
>>>   {
>>>     if (!dev->is_hotplug_bridge)
>>>       return false;
>>>
>>>     adev = ACPI_COMPANION(&dev->dev);
>>>     if (adev && acpi_device_power_manageable(adev))
>>>       return true;
>>>
>>>     root = pcie_find_root_port(dev);
>>>     if (!root)
>>>       return false;
>>>
>>>     adev = ACPI_COMPANION(&root->dev);
>>>     if (!adev)
>>>       return false;
>>>
>>>     rc = acpi_dev_get_property(dev, "HotPlugSupportInD3",
>>>                                ACPI_TYPE_INTEGER, &val);
>>>     if (rc < 0)
>>>       return false;
>>>
>>>     return val == 1;
>>>   }
>> Agree, thanks for your suggestion. Yes, it can be simplified too.
>> Can I do something like this using the unified device property API?
>>
>> static bool acpi_pci_bridge_d3(struct pci_dev *dev)
>> {
>>         struct acpi_device *adev;
>>         struct pci_dev *root;
>>         u8 val;
>>
>>         if (!dev->is_hotplug_bridge)
>>                 return false;
>>
>>         adev = ACPI_COMPANION(&dev->dev);
>>         if (adev && acpi_device_power_manageable(adev))
>>                 return true;
>>
>>         root = pcie_find_root_port(dev);
>>         if (!root)
>>                 return false;
>>
>>         if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val))
>>                 return false;
> I guess that might be OK.
>
> TBH I don't really like the device_property_read_u8() thing because
> (1) we know this is an ACPI property and I don't see a reason to use
> an "generic" interface that doesn't buy us anything, and (2) the
> connection to the source of the data (a _DSD method) is really, really
> hard to find.
>
> Admittedly, it's still pretty hard to connect acpi_dev_get_property()
> with "_DSD".  The only real clue is the comment about "Look for a
> special _DSD property ..."
>
Does it satisfy you if I change the comment and still use device_property API?

static bool acpi_pci_bridge_d3(struct pci_dev *dev)
{
        struct pci_dev *rpdev;
        u8 val;

        if (!dev->is_hotplug_bridge)
                return false;

        /* Assume D3 support if the bridge is power-manageable by ACPI. */
        if (acpi_pci_power_manageable(dev))
                return true;

        /*
         * Look for 'HotPlugSupportInD3' property for the root port and if
         * it is set we know the hierarchy behind it supports D3 just fine.
         */
        rpdev = pcie_find_root_port(dev);
        if (!rpdev)
                return false;

        if (device_property_read_u8(&rpdev->dev, "HotPlugSupportInD3", &val))
                return false;

        return val == 1;
}

If not, I'll do changes like this.

static bool acpi_pci_bridge_d3(struct pci_dev *dev)
{
        const union acpi_object *obj;
        struct acpi_device *adev;
        struct pci_dev *rpdev;


        if (!dev->is_hotplug_bridge)
                return false;

        /* Assume D3 support if the bridge is power-manageable by ACPI. */
        if (acpi_pci_power_manageable(dev))
                return true;

        /*
         * Look for 'HotPlugSupportInD3' property for the root port and if
         * it is set we know the hierarchy behind it supports D3 just fine.
         */
        rpdev = pcie_find_root_port(dev);
        if (!rpdev)
                return false;

        adev = ACPI_COMPANION(&rpdev->dev);
        if (!adev)
                return false;

       if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
                                   ACPI_TYPE_INTEGER, &obj) < 0)
                return false;

        return obj->integer.value == 1;
}