Re: [PATCH v1] drm/xe/vf: Fail migration recovery if fixups needed but platform not supported

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 13.05.2025 01:06, Tomasz Lis wrote:
> The post-migration recovery needs to be fully implemented for a
> specific platform in order to make continuation of workloads
> possible.
> 
> New platforms introduce changes which affect the recovery procedure,
> and without a clear verification of support this leads to errors
> with no straight forward error message explaining the cause.
> 
> This patch fixes that issue - it introduces a message to be logged
> when the current driver is known to not support the current platform.
> 
> Wedging the driver immediately also decreases the amount of
> additional errors which would come afterwards if the driver continued
> operation.
> 
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_sriov_vf.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf.c b/drivers/gpu/drm/xe/xe_sriov_vf.c
> index 2674fa948fda..f21f98f5d25f 100644
> --- a/drivers/gpu/drm/xe/xe_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_sriov_vf.c
> @@ -224,6 +224,11 @@ static void vf_post_migration_notify_resfix_done(struct xe_device *xe)
>  	drm_dbg(&xe->drm, "another recovery imminent, skipping notifications\n");
>  }
>  
> +static bool fixups_supported(struct xe_device *xe)
> +{

can we have some TODO comment here explaining what conditions we expect
to be added here? or maybe we can start with CONFIG_XE_DEBUG to indicate
early development phase?

> +	return false;
> +}
> +
>  static void vf_post_migration_recovery(struct xe_device *xe)
>  {
>  	bool need_fixups;
> @@ -243,6 +248,11 @@ static void vf_post_migration_recovery(struct xe_device *xe)
>  		vf_post_migration_fixup_ctb(xe);
>  
>  	vf_post_migration_notify_resfix_done(xe);
> +	if (need_fixups && !fixups_supported(xe)) {
> +		drm_err(&xe->drm, "migration recovery not supported by this module version\n");

we already have drm_err in the fail: section, do we need this extra one?
if yes, can we make the message more specific (and maybe the reason
should be printed in fixups_supported() as for now it's all magic)

also, since support likely will not change between one migration and the
other, maybe it should be just a single drm_info() message printed
during a VF boot that any later migration will fail, without waiting
until the first migration happen to surprise the user

> +		err = -ENOTRECOVERABLE;
> +		goto fail;
> +	}

hmm, and this whole chunk seems to be placed in a wrong place - if
fixups are not supported, why did we attempt to fixup CTB few lines
above and claim that fixups are done? can you please explain

>  	xe_pm_runtime_put(xe);
>  	drm_notice(&xe->drm, "migration recovery ended\n");
>  	return;