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

[Tvrtko Ursulin]

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