On Wed, Sep 18, 2024 at 08:35:48PM +0300, Jani Nikula wrote:
> For intel_wakeref_t, opaque is reasonable, but disguising the underlying
> struct ref_tracker * as an unsigned long is not so great. Update the
> typedef to remove one level of disguise.
>
> Although the kernel coding style strongly discourages pointer typedefs,
> it's a better alternative, and an incremental improvement on the status
> quo. It provides much better type safety than an unsigned long could,
> and prevents passing magic -1 instead of INTEL_WAKEREF_DEF. Moreover, it
> provides a gradual path for replacing intel_wakeref_t with struct
> ref_tracker * if desired.
>
> As an extra safety measure, check for error pointers in
> intel_ref_tracker_free() before passing them on to ref_tracker_free(),
> to catch any mistakes with mock gt special wakeref value.
After this would be easy to get a coccinelle or sed
to use ref_tracker directly and kill the typedef?
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_pm.h | 2 +-
> drivers/gpu/drm/i915/intel_wakeref.h | 16 +++++++++-------
> .../drm/xe/compat-i915-headers/intel_wakeref.h | 4 ++--
> 3 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index fef8d5d288f8..dcbfc09194b7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -105,7 +105,7 @@ int intel_gt_runtime_resume(struct intel_gt *gt);
>
> ktime_t intel_gt_get_awake_time(const struct intel_gt *gt);
>
> -#define INTEL_WAKEREF_MOCK_GT ((intel_wakeref_t)-ENODEV)
> +#define INTEL_WAKEREF_MOCK_GT ERR_PTR(-ENODEV)
>
> static inline bool is_mock_gt(const struct intel_gt *gt)
> {
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h
> index 3944587a5e78..48836ef52d40 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.h
> +++ b/drivers/gpu/drm/i915/intel_wakeref.h
> @@ -21,7 +21,7 @@
> #include <linux/timer.h>
> #include <linux/workqueue.h>
>
> -typedef unsigned long intel_wakeref_t;
> +typedef struct ref_tracker *intel_wakeref_t;
>
> #define INTEL_REFTRACK_DEAD_COUNT 16
> #define INTEL_REFTRACK_PRINT_LIMIT 16
> @@ -273,7 +273,7 @@ __intel_wakeref_defer_park(struct intel_wakeref *wf)
> */
> int intel_wakeref_wait_for_idle(struct intel_wakeref *wf);
>
> -#define INTEL_WAKEREF_DEF ((intel_wakeref_t)(-1))
> +#define INTEL_WAKEREF_DEF ERR_PTR(-ENOENT)
>
> static inline intel_wakeref_t intel_ref_tracker_alloc(struct ref_tracker_dir *dir)
> {
> @@ -281,17 +281,19 @@ static inline intel_wakeref_t intel_ref_tracker_alloc(struct ref_tracker_dir *di
>
> ref_tracker_alloc(dir, &user, GFP_NOWAIT);
>
> - return (intel_wakeref_t)user ?: INTEL_WAKEREF_DEF;
> + return user ?: INTEL_WAKEREF_DEF;
> }
>
> static inline void intel_ref_tracker_free(struct ref_tracker_dir *dir,
> - intel_wakeref_t handle)
> + intel_wakeref_t wakeref)
> {
> - struct ref_tracker *user;
> + if (wakeref == INTEL_WAKEREF_DEF)
> + wakeref = NULL;
>
> - user = (handle == INTEL_WAKEREF_DEF) ? NULL : (void *)handle;
> + if (WARN_ON(IS_ERR(wakeref)))
> + return;
>
> - ref_tracker_free(dir, &user);
> + ref_tracker_free(dir, &wakeref);
> }
>
> void intel_ref_tracker_show(struct ref_tracker_dir *dir,
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_wakeref.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_wakeref.h
> index 5c139ba144a6..2a32faea9db5 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_wakeref.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_wakeref.h
> @@ -5,6 +5,6 @@
>
> #include <linux/types.h>
>
> -typedef unsigned long intel_wakeref_t;
> +typedef struct ref_tracker *intel_wakeref_t;
>
> -#define INTEL_WAKEREF_DEF ((intel_wakeref_t)(-1))
> +#define INTEL_WAKEREF_DEF ERR_PTR(-ENOENT)
> --
> 2.39.2
>
