linux-kernel - Re: [PATCH v2] PCI: Add D3 support for PCI bridges in DT based platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 14 Feb 2024 11:28:37 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konrad.dybcio@...aro.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczy??ski <kw@...ux.com>, Rob Herring <robh@...nel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	quic_krichai@...cinc.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] PCI: Add D3 support for PCI bridges in DT based
 platforms

On Wed, Feb 14, 2024 at 02:18:31PM +0530, Manivannan Sadhasivam wrote:
> +/**
> + * of_pci_bridge_d3 - Check if the bridge is supporting D3 states or not
> + *
> + * @node: device tree node of the bridge
> + *
> + * Return: True if the bridge is supporting D3 states, False otherwise.

A lot of kernel-doc uses %true and %false.


> +bool of_pci_bridge_d3(struct device_node *node)
> +{
> +	return of_property_read_bool(node, "supports-d3");
> +}

What's the difference between of_property_read_bool() and
of_property_present()?  When should one use which?
The former has 691 occurrences in the tree, the latter 120.
The latter would seem more "literary" / readable here,
but maybe that's just me.


> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1142,6 +1142,9 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>  	if (pci_use_mid_pm())
>  		return false;
>  
> +	if (dev->dev.of_node)
> +		return of_pci_bridge_d3(dev->dev.of_node);
> +
>  	return acpi_pci_bridge_d3(dev);
>  }

This will result in an unnecessary test on non-DT platforms (e.g. ACPI)
whether dev->dev.of_node is set.

Please use dev_of_node() instead of "dev->dev.of_node" so that the
code added here can be optimized away by the compiler on non-DT
platforms (due to the IS_ENABLED(CONFIG_OF)).

Thanks,

Lukas