Re: [Intel-gfx] [PATCH v5] drm/i915/gt: make a gt sysfs group and move power management files

[Andi Shyti <andi.shyti@intel.com>]

> > > > +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> > > > +{
> > > > +	struct kobject *parent = gt_get_parent_obj(gt);
> > > > +
> > > > +	/*
> > > > +	 * the name gt tells us wether sysfs_root
> > > > +	 * object was initialized properly
> > > > +	 */
> > > > +	if (!strcmp(gt->sysfs_root.name, "gt"))
> > > > +		kobject_put(&gt->sysfs_root);
> > > 
> > > Slightly nicer would be looking at  kobj->state_initialized for this check I
> > > think. Or even kref_get_unless_zero on kobj->kref? Ugliness there is double
> > > put on sucess which makes me ask whether holding a reference on parent is
> > > even needed? It can't go away so perhaps it isn't.
> > 
> > I'd rather use the state_initialized, even though I don't trust
> > its value if the kobject has failed to initialise earlier, I
> > trust it only if it's '1', maybe I'm paranoic.
> 
> But is the reference even needed?

yes, because I _get it here (i.e. above, during initialization):

> > > > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > > > +{
> > > > +	struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
> > > > +	int ret;
> > > > +

and if I need to call kobject_put at the end. If for some reason
the files have failed to be initialized, I would have an
unbalanced put and a warning would be printed.

I'll summarize in pseudo code:

intel_gt_sysfs_register()
{
	kobject_init_and_add(sysfs_root...); /* which calls kobject_get() inside */
	if (fails)
		kobject_put(sysfs_root); /* reference goes to '0' */
}

intel_gt_sysfs_unregister()
{
	option1: I don't call kobject_put(), I have an unbalanced
                 situation as you reviewed in patch 1.

        option2: I call kobject_put(), if it did fail during init
                 there is an unbalanced situation, which is
                 handled but an annoying WARN() is issued.

	option3: I check if "state_initialized" which I suppose
                 has been properly initialised during declaration
                 (maybe too paranoic?) and call _put()
                 accordingly
}

Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx