Re: [PATCH v2 3/5] lib/igt_gt: Add igt_open_forcewake_handle_for_pcidev()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 23, 2024 at 05:11:44PM -0500, Lucas De Marchi wrote:
> In several places we are getting the pci_dev for an fd and then passing
> both the pci_dev and the fd to intel_register_access_init(). Add a
> helper so intel_register_access_init() can operate on pci_dev only.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  lib/igt_gt.c | 37 +++++++++++++++++++++++++++++++++++++
>  lib/igt_gt.h |  3 +++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 0cc3ee131..c9fd9164f 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -30,6 +30,7 @@
>  #include <sys/stat.h>
>  #include <sys/ioctl.h>
>  #include <fcntl.h>
> +#include <pciaccess.h>
>  
>  #include "drmtest.h"
>  #include "i915/gem.h"
> @@ -514,6 +515,42 @@ int igt_open_forcewake_handle(int fd)
>  	return igt_debugfs_open(fd, fn, O_RDONLY);
>  }
>  
> +int igt_open_forcewake_handle_for_pcidev(const struct pci_device *pci_dev)
> +{
> +	static const char *FORCEWAKE_FILES[] = {

static const char * const FORCEWAKE_FILES[] = {

for full constness.

> +		"forcewake_all", "i915_forcewake_user",
> +		NULL
> +	};
> +	const char *debugfs_root = igt_debugfs_mount(), **fn;
> +	char path[PATH_MAX];
> +	int dirlen, ret;
> +
> +	if (!debugfs_root)
> +		return -ENOENT;
> +
> +	ret = snprintf(path, sizeof(path), "%s/dri/%04u:%02u:%02u.%u",

The u vs. x issue probably would have flown by me had 
you not mentioned it already.

> +		       debugfs_root,
> +		       pci_dev->domain, pci_dev->bus, pci_dev->dev,
> +		       pci_dev->func);
> +	if (ret < 0 || ret >= sizeof(path))
> +		return -EINVAL;
> +
> +	dirlen = ret;
> +	for (fn = FORCEWAKE_FILES; *fn; fn++) {
> +		ret = snprintf(path + dirlen, sizeof(path) - dirlen,
> +			       "/%s", *fn);
> +		if (ret < 0 || ret >= sizeof(path) - dirlen)
> +			return -EINVAL;
> +
> +		ret = open(path, O_RDONLY);
> +		if (ret >= 0 || errno != ENOENT)
> +			return ret;

For other failure cases you return -errno, so the
-1+errno here seems a bit inconsistent.

Otherwise looks nice
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	}
> +
> +	return -ENOENT;
> +
> +}
> +
>  #if defined(__x86_64__) || defined(__i386__)
>  static unsigned int clflush_size;
>  
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index d0c5510b1..4a67cd9f0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -31,6 +31,8 @@
>  #include "i915/i915_drm_local.h"
>  #include "i915_drm.h"
>  
> +struct pci_device;
> +
>  void igt_require_hang_ring(int fd, uint32_t ctx, int ring);
>  
>  typedef struct igt_hang {
> @@ -63,6 +65,7 @@ void igt_fork_hang_helper(void);
>  void igt_stop_hang_helper(void);
>  
>  int igt_open_forcewake_handle(int fd);
> +int igt_open_forcewake_handle_for_pcidev(const struct pci_device *pci_dev);
>  
>  int igt_setup_clflush(void);
>  void igt_clflush_range(void *addr, int size);
> -- 
> 2.46.1

-- 
Ville Syrjälä
Intel