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;
}
