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

[Tvrtko Ursulin]

On 25/02/2020 01:35, Andi Shyti wrote:
>>>>> +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
> }

Yes you are right, I confused the two parents again. :I

Okay then, is the extra kobject_get/put on the parent 
(kobject_get(gt_get_parent_obj(gt) - this one) needed?

Regards,

Tvrtko