Re: [PATCH Review 1/1] drm/amdgpu: handle denied inject error into critical regions

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 1/12/2022 7:12 AM, Stanley.Yang wrote:
> Signed-off-by: Stanley.Yang <Stanley.Yang@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 10 +++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/ta_ras_if.h  |  3 ++-
>   3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index c742d1aacf5a..8e0ea582b9c7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -1309,6 +1309,12 @@ static void psp_ras_ta_check_status(struct psp_context *psp)
>   		break;
>   	case TA_RAS_STATUS__SUCCESS:
>   		break;
> +	case TA_RAS_STATUS__TEE_ERROR_ACCESS_DENIED:
> +		if (ras_cmd->cmd_id == TA_RAS_COMMAND__TRIGGER_ERROR) {
> +			dev_info(psp->adev->dev,
> +					"RAS INFO: Inject error to critical region is not allowed\n");
> +		}

Instead of doing this, why not print this in psp_ras_trigger_error(). 
i.e. caller interprets the error code and prints the appropriate 
message. I guess that is the single entry point to send TRIGGER_ERROR 
command.

Thanks,
Lijo

> +		break;
>   	default:
>   		dev_warn(psp->adev->dev,
>   				"RAS WARNING: ras status = 0x%X\n", ras_cmd->ras_status);
> @@ -1521,7 +1527,9 @@ int psp_ras_trigger_error(struct psp_context *psp,
>   	if (amdgpu_ras_intr_triggered())
>   		return 0;
>   
> -	if (ras_cmd->ras_status)
> +	if (ras_cmd->ras_status == TA_RAS_STATUS__TEE_ERROR_ACCESS_DENIED)
> +		return -EACCES;
> +	else if (ras_cmd->ras_status)
>   		return -EINVAL;
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index e674dbed3615..8bdc2e85cb20 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -449,7 +449,7 @@ static ssize_t amdgpu_ras_debugfs_ctrl_write(struct file *f,
>   	}
>   
>   	if (ret)
> -		return -EINVAL;
> +		return ret;
>   
>   	return size;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/ta_ras_if.h b/drivers/gpu/drm/amd/amdgpu/ta_ras_if.h
> index 5093826a43d1..509d8a1945eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/ta_ras_if.h
> +++ b/drivers/gpu/drm/amd/amdgpu/ta_ras_if.h
> @@ -64,7 +64,8 @@ enum ta_ras_status {
>   	TA_RAS_STATUS__ERROR_PCS_STATE_ERROR            = 0xA016,
>   	TA_RAS_STATUS__ERROR_PCS_STATE_HANG             = 0xA017,
>   	TA_RAS_STATUS__ERROR_PCS_STATE_UNKNOWN          = 0xA018,
> -	TA_RAS_STATUS__ERROR_UNSUPPORTED_ERROR_INJ      = 0xA019
> +	TA_RAS_STATUS__ERROR_UNSUPPORTED_ERROR_INJ      = 0xA019,
> +	TA_RAS_STATUS__TEE_ERROR_ACCESS_DENIED          = 0xA01A
>   };
>   
>   enum ta_ras_block {