Re: [PATCH v2] drm/xe: Log unreliable MMIO reads during forcewake

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 17, 2024 at 03:40:26AM +0000, Shuicheng Lin wrote:
> In some cases, when the driver attempts to read an MMIO register,
> the hardware may return 0xFFFFFFFF. The current force wake path
> code treats this as a valid response, as it only checks the BIT.
> However, 0xFFFFFFFF should be considered an invalid value, indicating
> a potential issue. To address this, we should add a log entry to
> highlight this condition.
> 
> v2 (Matt Brost):
>   - set ret value (-EIO) to kick the error to upper layers
> 
> Suggested-by: Alex Zuo <alex.zuo@intel.com>
> Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_force_wake.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> index a64c14757c84..49ceb16e82e4 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.c
> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> @@ -115,10 +115,15 @@ static int __domain_wait(struct xe_gt *gt, struct xe_force_wake_domain *domain,
>  			     XE_FORCE_WAKE_ACK_TIMEOUT_MS * USEC_PER_MSEC,
>  			     &value, true);
>  	if (ret)
> -		xe_gt_notice(gt, "Force wake domain %d failed to ack %s (%pe) reg[%#x] = %#x\n",
> -			     domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> -			     domain->reg_ack.addr, value);
> -
> +		xe_gt_err(gt, "Force wake domain %d failed to ack %s (%pe) reg[%#x] = %#x\n",

Nothing in the commit message talks about this promotion from notice to error.
Please do that in the commit message. After all an error is indeed returned below anyway
when we are here.
Or do this in a separate patch.

> +			  domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> +			  domain->reg_ack.addr, value);
> +	if (value == ~0) {
> +		xe_gt_err(gt,
> +			  "Force wake domain %d: %s. MMIO unreliable (forcewake register returns 0xFFFFFFFF)!\n",
> +			  domain->id, str_wake_sleep(wake));
> +		ret = -EIO;
> +	}
>  	return ret;
>  }
>  
> -- 
> 2.25.1
>