| Message ID | 20221114123843.880902-2-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cold Reset IGT Test | expand |
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@intel.com> > --- > 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@intel.com> > 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 >
> -----Original Message----- > From: Vivi, Rodrigo <rodrigo.vivi@intel.com> > Sent: Tuesday, November 15, 2022 1:27 AM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: intel-gfx@lists.freedesktop.org; kamil.konieczny@linux.intel.com; > Tangudu, Tilak <tilak.tangudu@intel.com>; Nilawar, Badal > <badal.nilawar@intel.com> > 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@intel.com> > > --- > > 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@intel.com> 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 > >
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; }
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> --- lib/igt_pm.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)