Re: [PATCH] drm/xe: Check return values of functions in xe_gt_shutdown()

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 26-09-2024 16:49, Ghimiray, Himal Prasad wrote:
> 
> 
> On 26-09-2024 16:41, Maarten Lankhorst wrote:
>> Hey,
>>
>> Den 2024-09-26 kl. 13:00, skrev apoorva.singh@intel.com:
>>> From: Apoorva Singh <apoorva.singh@intel.com>
>>>
>>> Check the return values of the functions xe_force_wake_get()
>>> and xe_force_wake_put() to prevent mistakenly treating them as
>>> void returns.
>>>
>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> Signed-off-by: Apoorva Singh <apoorva.singh@intel.com>
>>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_gt.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>> index 274737417b0f..317640554310 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>> @@ -890,9 +890,12 @@ int xe_gt_suspend(struct xe_gt *gt)
>>>   void xe_gt_shutdown(struct xe_gt *gt)
>>>   {
>>> -    xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> +    int err;

Nit. Add blank line before merge.

>>> +    err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> +    xe_gt_WARN(gt, err, "Acknowledgment for domain awake timedout");
>>>       do_gt_reset(gt);
>>> -    xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> +    err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> +    xe_gt_WARN(gt, err, "Acknowledgment for domain sleep timedout");
>>>   }
>>
>> There's absolutely nothing we can do with xe_force_wake_put, the only 
>> thing you can do here is:
>>
>> int ret = xe_force_wake_get(..);
>> do_gt_reset;
>> if (!ret)
>> xe_force_wake_put
> 
> This is wrong in current scenario. force_wake_get() fails but it still 
> increases refcount. So xe_force_wake_put needs to be called here to 
> decrease the refcount. I am already working on solving the problem in 
> entire driver.
> 
> https://lore.kernel.org/intel-xe/20240924121641.1045763-1- 
> himal.prasad.ghimiray@intel.com/T/#t
> 
>>
>> Also no point in complaining, the hardware may already be hosed. :)
>>
>> I believe that xe_force_wake_put should return void, there's nothing 
>> we can do except log the error here.
> 
> Agreed. Above series covers same too.
> 
>>
>> Cheers,
>> Maarten
>