Re: [PATCH] drm/xe: Always check force_wake_get return code

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 3/13/2024 7:56 AM, Jani Nikula wrote:
> On Wed, 13 Mar 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
>> On 3/13/2024 1:31 AM, Jani Nikula wrote:
>>> On Tue, 12 Mar 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
>>>> diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
>>>> index d9aa815a5bc2..902c52d95a8a 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gsc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gsc.c
>>>> @@ -287,7 +287,7 @@ static void gsc_work(struct work_struct *work)
>>>>    	spin_unlock_irq(&gsc->lock);
>>>>    
>>>>    	xe_pm_runtime_get(xe);
>>>> -	xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
>>>> +	XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC));
>>> Up to the xe maintainers to decide, but I'm really not a fan of hiding
>>> functionality inside warn ons. My approach usually is, would it work if
>>> all the warns were removed? If yes, it's good. If not, maybe reconsider.
>> The code works even without the warns, they're only there so we know
>> that there was a forcewake issue if/when some other error crops up down
>> the line (which will be handled appropriately). There is nothing we can
>> do to actually handle the forcewake failure as it can only happen if the
>> HW is in a bad state.
> My point is, I personally prefer:
>
> 	ret = do_stuff():
>
> 	WARN_ON(ret);
>
> over:
>
> 	WARN_ON(do_stuff());
>
> because in the former do_stuff() stands out as something we actually
> want to do functionally, while in the latter the fact that we do
> anything at all is hidden inside the WARN_ON().
>
> I prefer WARN_ON()'s to only have stuff inside them that have no
> side-effects:
>
> 	WARN_ON(check_stuff_but_dont_do_stuff());

Got it, I do agree that it's generally cleaner that way.

>
> Again, not my call to make here, just musing on style. ;)

I actually copied what was done elsewhere in Xe with forcewake failures 
in unabortable paths and other similar checks. For consistency with 
what's already there, I'd prefer to keep it like it is.

Daniele

>
>
> BR,
> Jani.
>
>