[Intel-gfx] [PATCH] drm/i915/huc: Add more errors for I915_PARAM_HUC_STATUS

[Michal Wajdeczko]

On 30.03.2020 14:28, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2020-03-30 12:33:02)
>> There might be many reasons why we failed to successfully
>> load and authenticate HuC firmware, but today we only use
>> single error in case of no HuC hardware. Add some more
>> error codes for most common cases (disabled, not installed,
>> corrupted or mismatched firmware).
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Tony Ye <tony.ye at intel.com>
>> Cc: Robert M. Fosha <robert.m.fosha at intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_huc.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> index d6097b46600c..1e8073ec343f 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> @@ -200,9 +200,13 @@ int intel_huc_auth(struct intel_huc *huc)
>>   * This function reads status register to verify if HuC
>>   * firmware was successfully loaded.
>>   *
>> - * Returns: 1 if HuC firmware is loaded and verified,
>> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
>> - * is not present on this platform.
>> + * Returns:
>> + *  * -ENODEV if HuC is not present on this platform,
>> + *  * -EOPNOTSUPP if HuC firmware is disabled,
>> + *  * -ENOPKG if HuC firmware was not installed,
>> + *  * -ENOEXEC if HuC firmware is invalid or mismatched,
>> + *  * 0 if HuC firmware is not running,
>> + *  * 1 if HuC firmware is authenticated and running.
>>   */
>>  int intel_huc_check_status(struct intel_huc *huc)
>>  {
>> @@ -210,8 +214,18 @@ int intel_huc_check_status(struct intel_huc *huc)
>>         intel_wakeref_t wakeref;
>>         u32 status = 0;
>>  
>> -       if (!intel_huc_is_supported(huc))
>> +       switch (__intel_uc_fw_status(&huc->fw)) {
>> +       case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
>>                 return -ENODEV;
> 
> No HW support.
> 
>> +       case INTEL_UC_FIRMWARE_DISABLED:
>> +               return -EOPNOTSUPP;
> 
> Override by user [sysadmin]
> 
>> +       case INTEL_UC_FIRMWARE_MISSING:
>> +               return -ENOPKG;
> 
> FILENOTFOUND.
> 
>> +       case INTEL_UC_FIRMWARE_ERROR:
>> +               return -ENOEXEC;
> 
> File corruption.
> 
> There's nothing else between us loading the fw and the huc rejecting
> it?
> 
> FIRMWARE_FAIL? That's set as the opposite of FIRMWARE_TRANSFERRED in
> that we failed to upload the image to the HW. The firmware itself hasn't
> had a chance to run.
> 
> case INTEL_UC_FIRMWARE_FAIL:
> 	return -ENXIO;
> 
> Or is that being overridden to FIRMWARE_ERROR?

No, it's not overridden by FIRMWARE_ERROR (since we use FIRMWARE_ERROR
as final state, while with FIRMWARE_FAIL there is a chance for recovery
during reset)

Also note that FIRMWARE_FAIL case is covered by the register check that
we have below, which provides HuC runtime status.

And if we decide to use FIRMWARE_FAIL to report -ENXIO, then it is
unlikely that we will ever report 0 again for any other fw error that
could prevent fw from successful load (now recall your and Joonas
position that this param shall stay as reflection of register read).

Michal

ps. on other hand, if we trust our uc_fw_status() then we can drop that
register read, finally decouple GET_PARAM from MMIO_READ and fully rely
on cached status:

case INTEL_UC_FIRMWARE_RUNNING:
	return 1;
default:
	return 0;

see [1] for my earlier attempt, before uc_fw.status was added

[1] https://patchwork.freedesktop.org/patch/306179/?series=60928&rev=1

> 
> Other than the question of whether there's one more step before the fw
> is being run [and then able to set HUC_STATUS as it determines for
> itself],
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris
>