Re: [igt-dev] [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd

[Kamil Konieczny <kamil.konieczny@linux.intel.com>]

Hi Anshuman,

one small nit, see below.

On 2022-11-23 at 14:24:39 +0530, Anshuman Gupta wrote:
> Created igt_pm_open_pci_firmware_node() to refactor
> the retrieving the firmware_node fd code.
> 
> igt_pm_open_pci_firmware_node() will be used by other
> firmware_node consumers.
> 
> While doing that fixed the leaked fd as well.
> 
> v2:
> - return false instead of igt_require on no firmware_node_fd. [Kamil]
> - use igt_assert() when failed to open 'real_power_state' on error
>   other then ENONET.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  lib/igt_pm.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 1e6e9ed3ff..4f0cfbdd10 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -863,6 +863,20 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
>  	return strstr(buf, "LPSP: capable");
>  }
>  
> +static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
> +{
> +	char name[PATH_MAX];
> +	int dir;
> +
> +	snprintf(name, PATH_MAX,
> +		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> +		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +
> +	dir = open(name, O_RDONLY);
> +
> +	return dir;
> +}
> +
>  /**
>   * igt_pm_acpi_d3cold_supported:
>   * @pci_dev: root port pci_dev.
> @@ -873,23 +887,23 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
>   */
>  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)
>  {
> -	char name[PATH_MAX];
> -	int dir, fd;
> -
> -	snprintf(name, PATH_MAX,
> -		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> -		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +	int firmware_node_fd, fd;
>  
> -	dir = open(name, O_RDONLY);
> -	igt_require(dir > 0);
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +	if (firmware_node_fd < 0)
> +		return false;
>  
>  	/* BIOS need to enable ACPI D3Cold Support.*/
> -	fd = openat(dir, "real_power_state", O_RDONLY);
> -	if (fd < 0 && errno == ENOENT)
> +	fd = openat(firmware_node_fd, "real_power_state", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
>  		return false;
> +	}
>  
> -	igt_require(fd > 0);
> +	igt_assert(errno > 0);

You want here no error, so
	igt_assert(errno == 0);
or better:
	igt_assert_f(fd > 0, "failed to open real_power_state, errno=%d\n", errno);

Regards,
Kamil

>  
> +	close(firmware_node_fd);
> +	close(fd);
>  	return true;
>  }
>  
> -- 
> 2.25.1
>