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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 24/02/2020 16:30, Andi Shyti wrote:
> > > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > > +{
> > > +	struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
> > > +	int ret;
> > > +
> > > +	ret = kobject_init_and_add(&gt->sysfs_root,
> > > +				   &sysfs_gt_ktype,
> > > +				   parent, "gt");
> > > +	if (ret) {
> > > +		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
> >
> > I'd perhaps pin point the failure more by s/file/GT sysfs root/.
>
> OK
>
> > > +		kobject_put(&gt->sysfs_root);
> >
> > Is the reference needed for the registration steps? I am thinking if you
> > could kobject_get only once everything worked to simplify.
>
> I haven't really understood what you mean here. Are you saying
> that kobject_put not needed? in the lib/kobject.c it says as
> comment to kobject_init_and_add():
>
> "
>   * If this function returns an error, kobject_put() must be called to
>   * properly clean up the memory associated with the object.  This is the
>   * same type of error handling after a call to kobject_add() and kobject
>   * lifetime rules are the same here.
>   */
> "

My mistake, I confused the two objects.

> > > +	ret = sysfs_create_file(&gt->sysfs_root, &dev_attr_gt_info.attr);
> > > +	if (ret)
> > > +		drm_err(&gt->i915->drm, "failed to create sysfs gt info files\n");
> > > +
> > > +	intel_gt_sysfs_pm_init(gt, &gt->sysfs_root);
> >
> > If you put this first you can avoid the goto I think which makes the
> > function smaller.
>
> True!
>
> > > +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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx