> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Tuesday, November 15, 2022 1:27 AM
> To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; kamil.konieczny@xxxxxxxxxxxxxxx;
> Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>; Nilawar, Badal
> <badal.nilawar@xxxxxxxxx>
> Subject: Re: [PATCH i-g-t v2 1/3] lib/igt_pm: Refactor get firmware_node fd
>
> On Mon, Nov 14, 2022 at 06:08:41PM +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@xxxxxxxxx>
> > ---
> > lib/igt_pm.c | 36 +++++++++++++++++++++++++-----------
>
> I believe we need a massive refactor in this lib... we have 2 different ways of
> using stuff, plus that ugly global restore...
>
> but anyway, this one here looks a good change regardless of the current
> mess in the lib.
>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Thanks Rodrigo for RB,
my bad I sent these patches on kernel mailing list instead of igt-dev.
I will keep your RB and re-float the patches to igt-dev.
Thanks,
Anshuman Gupta.
>
>
> > 1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 1e6e9ed3f..4f0cfbdd1
> > 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);
> >
> > + close(firmware_node_fd);
> > + close(fd);
> > return true;
> > }
> >
> > --
> > 2.25.1
> >
