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

[Jani Nikula <jani.nikula@linux.intel.com>]

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());

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


BR,
Jani.


-- 
Jani Nikula, Intel