> > +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(>->sysfs_root,
> > + &sysfs_gt_ktype,
> > + parent, "gt");
> > + if (ret) {
> > + drm_err(>->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(>->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(>->sysfs_root, &dev_attr_gt_info.attr);
> > + if (ret)
> > + drm_err(>->i915->drm, "failed to create sysfs gt info files\n");
> > +
> > + intel_gt_sysfs_pm_init(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(>->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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
