On Tue, Feb 04, 2020 at 06:58:00AM -0800, Srinivas Pandruvada wrote:
> > lib/kobject.c
> > 664 static void kobject_cleanup(struct kobject *kobj)
> > 665 {
> > 666 struct kobj_type *t = get_ktype(kobj);
> > 667 const char *name = kobj->name;
> > 668
> > 669 pr_debug("kobject: '%s' (%p): %s, parent %p\n",
> > 670 kobject_name(kobj), kobj, __func__, kobj-
> > >parent);
> > 671
> > 672 if (t && !t->release)
> > 673 pr_debug("kobject: '%s' (%p): does not have a
> > release() function, it is broken and must be fixed. See
> > Documentation/kobject.txt.\n",
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > 674 kobject_name(kobj), kobj);
> >
> There are several usages in kernel without release callback.
There are 19. They mostly have to do with sysfs? I'm still figuring
these things out so I don't know how it all fits together yet.
> Does the attach patch fixes the warning?
>
> Thanks,
> Srinivas
>
>
> > regards,
> > dan carpenter
> >
> From 959ee9fe784d623960df2560fd13cae215b464d4 Mon Sep 17 00:00:00 2001
> From: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Date: Tue, 4 Feb 2020 06:50:11 -0800
> Subject: [PATCH] platform/x86/intel-uncore-freq: Add release function
>
> When user mode is reference to the kobject, wait to free the memory
> during module unload by adding a release callback.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
> drivers/platform/x86/intel-uncore-frequency.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel-uncore-frequency.c b/drivers/platform/x86/intel-uncore-frequency.c
> index 2b1a0734c3f8..3eda66d0713b 100644
> --- a/drivers/platform/x86/intel-uncore-frequency.c
> +++ b/drivers/platform/x86/intel-uncore-frequency.c
> @@ -217,12 +217,21 @@ static struct attribute *uncore_attrs[] = {
> NULL
> };
>
> +
> +static void uncore_sysfs_entry_release(struct kobject *kobj)
> +{
> + if (!uncore_max_entries)
> + kfree(uncore_instances);
> +}
> +
> static struct kobj_type uncore_ktype = {
> + .release = uncore_sysfs_entry_release,
> .sysfs_ops = &kobj_sysfs_ops,
> .default_attrs = uncore_attrs,
> };
>
> static struct kobj_type uncore_root_ktype = {
> + .release = uncore_sysfs_entry_release,
> .sysfs_ops = &kobj_sysfs_ops,
> };
>
> @@ -281,9 +290,9 @@ static void uncore_remove_die_entry(int cpu)
> mutex_lock(&uncore_lock);
> data = uncore_get_instance(cpu);
> if (data) {
> - kobject_put(&data->kobj);
> data->control_cpu = -1;
> data->valid = false;
> + kobject_put(&data->kobj);
> }
> mutex_unlock(&uncore_lock);
> }
> @@ -424,12 +433,14 @@ static void __exit intel_uncore_exit(void)
>
> unregister_pm_notifier(&uncore_pm_nb);
> cpuhp_remove_state(uncore_hp_state);
> + mutex_lock(&uncore_lock);
It's not clear what this locking does. The actual uncore_sysfs_entry_release()
can be called from a different work queue depending on the .config. See
how kobject_release() is implemented.
regards,
dan carpenter
> for (i = 0; i < uncore_max_entries; ++i) {
> if (uncore_instances[i].valid)
> kobject_put(&uncore_instances[i].kobj);
> }
> + uncore_max_entries = 0;
> kobject_put(&uncore_root_kobj);
> - kfree(uncore_instances);
> + mutex_unlock(&uncore_lock);
> }
> module_exit(intel_uncore_exit)
