| Message ID | cca2b0631f816ad90461aa1bf4fe3f80c0e13464.1726680898.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: wakeref fixes and improvements | expand |
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@intel.com> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > 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 >
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)
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. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- 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(-)