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_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.
 */
"

> > +	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.

> > +	/*
> > +	 * We are interested at knowing from where the interface
> > +	 * has been called, whether it's called from gt/ or from
> > +	 * the parent directory.
> > +	 * From the interface position it depends also the value of
> > +	 * the private data.
> > +	 * If the interface is called from gt/ then private data is
> > +	 * of the "struct intel_gt *" type, otherwise it's * a
> > +	 * "struct drm_i915_private *" type.
> > +	 */
> > +	if (strcmp(dev->kobj.name, "gt")) {
> > +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > +		pr_warn_ratelimited(DEPRECATED
> > +			"(%s, %d) trying to access deprecated interface, "
> > +			"use the corresponding interface in gt/\n",
> 
> Saying interface two times sounds a bit suboptimal but I leave this to
> native speakers to improve.
> 
> Can you get to the name of the sysfs control here?

sure.

> "(%s, %d) is trying to access deprecated '%s' sysfs control. Please use
> 'gt/%s' instead.". Something like that?

yes... it's not always easy to write logs when you have to stay
within the 80 characters

Thanks for the review!
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx