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

[Gupta, Anshuman]

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Wednesday, November 30, 2022 2:14 PM
> To: igt-dev at lists.freedesktop.org
> Cc: Gupta, Anshuman <anshuman.gupta at intel.com>; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>; kamil.konieczny at linux.intel.com; Tangudu, Tilak
> <tilak.tangudu at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> Iddamsetty, Aravind <aravind.iddamsetty at intel.com>
> Subject: Re: [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd
> 
> 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 at intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at 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);
Thanks for pointing it out.
I will fix it.
BR,
Anshuman.
> 
> Regards,
> Kamil
> 
> >
> > +	close(firmware_node_fd);
> > +	close(fd);
> >  	return true;
> >  }
> >
> > --
> > 2.25.1
> >