Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH v15 7/9] PCI: Setup ACPI fwnode early and at the same time
 with OF
To:     Bjorn Helgaas <helgaas@kernel.org>
CC:     Amey Narkhede <ameynarkhede03@gmail.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        <alex.williamson@redhat.com>,
        Raphael Norwitz <raphael.norwitz@nutanix.com>,
        <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kw@linux.com>, Sinan Kaya <okaya@kernel.org>,
        Len Brown <lenb@kernel.org>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Mika Westerberg <mika.westerberg@linux.intel.com>
References: <20210814041048.GA2765970@bjorn-Precision-5520>
From:   Shanker R Donthineni <sdonthineni@nvidia.com>
Message-ID: <63e0f64c-e51b-feb7-a193-c2999a280b80@nvidia.com>
Date:   Sat, 14 Aug 2021 11:16:11 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210814041048.GA2765970@bjorn-Precision-5520>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Aug 2021 16:16:14.5912
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 356ba3e2-35a8-4961-589c-08d95f3ed6dc
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT040.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB3733
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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