| Message ID | 20240722210648.80892-6-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix i915 pmu on bind/unbind | expand |
On 22/07/2024 22:06, Lucas De Marchi wrote: > There's no need to free the resources during unbind. Since perf events > may still access them due to open events, it's safer to free them when > dropping the last i915 reference. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/i915_pmu.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index b5d14dd318e4..8708f905f4f4 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -5,6 +5,7 @@ > */ > > #include <linux/pm_runtime.h> > +#include <drm/drm_managed.h> > > #include "gt/intel_engine.h" > #include "gt/intel_engine_pm.h" > @@ -1152,6 +1153,17 @@ static void free_event_attributes(struct i915_pmu *pmu) > pmu->pmu_attr = NULL; > } > > +static void free_pmu(struct drm_device *dev, void *res) > +{ > + struct i915_pmu *pmu = res; > + struct drm_i915_private *i915 = pmu_to_i915(pmu); > + > + free_event_attributes(pmu); > + kfree(pmu->base.attr_groups); > + if (IS_DGFX(i915)) > + kfree(pmu->name); > +} > + > static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node *node) > { > struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node); > @@ -1302,6 +1314,9 @@ void i915_pmu_register(struct drm_i915_private *i915) > if (ret) > goto err_unreg; > > + if (drmm_add_action_or_reset(&i915->drm, free_pmu, pmu)) > + goto err_unreg; Is i915_pmu_unregister_cpuhp_state missing on this error path? Regards, Tvrtko > + > return; > > err_unreg: > @@ -1336,11 +1351,7 @@ void i915_pmu_unregister(struct drm_i915_private *i915) > hrtimer_cancel(&pmu->timer); > > i915_pmu_unregister_cpuhp_state(pmu); > - > perf_pmu_unregister(&pmu->base); > + > pmu->base.event_init = NULL; > - kfree(pmu->base.attr_groups); > - if (IS_DGFX(i915)) > - kfree(pmu->name); > - free_event_attributes(pmu); > }
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index b5d14dd318e4..8708f905f4f4 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -5,6 +5,7 @@ */ #include <linux/pm_runtime.h> +#include <drm/drm_managed.h> #include "gt/intel_engine.h" #include "gt/intel_engine_pm.h" @@ -1152,6 +1153,17 @@ static void free_event_attributes(struct i915_pmu *pmu) pmu->pmu_attr = NULL; } +static void free_pmu(struct drm_device *dev, void *res) +{ + struct i915_pmu *pmu = res; + struct drm_i915_private *i915 = pmu_to_i915(pmu); + + free_event_attributes(pmu); + kfree(pmu->base.attr_groups); + if (IS_DGFX(i915)) + kfree(pmu->name); +} + static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node *node) { struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node); @@ -1302,6 +1314,9 @@ void i915_pmu_register(struct drm_i915_private *i915) if (ret) goto err_unreg; + if (drmm_add_action_or_reset(&i915->drm, free_pmu, pmu)) + goto err_unreg; + return; err_unreg: @@ -1336,11 +1351,7 @@ void i915_pmu_unregister(struct drm_i915_private *i915) hrtimer_cancel(&pmu->timer); i915_pmu_unregister_cpuhp_state(pmu); - perf_pmu_unregister(&pmu->base); + pmu->base.event_init = NULL; - kfree(pmu->base.attr_groups); - if (IS_DGFX(i915)) - kfree(pmu->name); - free_event_attributes(pmu); }
There's no need to free the resources during unbind. Since perf events may still access them due to open events, it's safer to free them when dropping the last i915 reference. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_pmu.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)