Re: [PATCH v2] drm/xe: Don't fail probe on unsupported mailbox command

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

On Mon, Jul 14, 2025 at 06:02:55PM -0400, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Raag Jadav
> Sent: Monday, July 14, 2025 2:55 PM
> To: De Marchi, Lucas <lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Brost, Matthew <matthew.brost@intel.com>; Summers, Stuart <stuart.summers@intel.com>; Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>; Jadav, Raag <raag.jadav@intel.com>
> Subject: [PATCH v2] drm/xe: Don't fail probe on unsupported mailbox command
> > 
> > If the device is running older pcode firmware, it is possible that newer
> > mailbox commands are not supported by it. The sysfs attributes aren't
> > useful in that case, but we shouldn't fail driver probe because of it.
> > As of now, it is unknown if we can distinguish unsupported commands before
> > attempting them. But until we figure out a way to do that, fix the
> > regressions.
> > 
> > v2: Add debug message (Lucas)
> > 
> > Fixes: cdc36b66cd41 ("drm/xe: Expose fan control and voltage regulator version")
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > Tested-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_device_sysfs.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > index 8250aa4620ee..fd2ba0da02a2 100644
> > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > @@ -160,8 +160,13 @@ static int late_bind_create_files(struct device *dev)
> >  
> >  	ret = xe_pcode_read(root, PCODE_MBOX(PCODE_LATE_BINDING, GET_CAPABILITY_STATUS, 0),
> >  			    &cap, NULL);
> > -	if (ret)
> > +	if (ret) {
> > +		if (ret == -ENXIO) {
> > +			drm_dbg(&xe->drm, "Late binding not supported by firmware\n");
> > +			ret = 0;
> > +		}
> 
> If we're disarming the error value here, then do we need to 'goto out' after this,
> or can we reenter the standard execution path safely here?  I'm guessing we're
> branching unconditionally because we still detected a failure in this function that
> requires handling, and that we're just masking the report to prevent driver probe
> failures on boot as per the commit message description.
> 
> So, assuming there's a compelling reason we still need to branch here:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> -Jonathan Cavitt
> 
> >  		goto out;

                ^ HERE

This code is 'go(ing)to out' right after the error disarming.

For a moment this comment scared me... I had pushed the patch, then I removed with
a force push to be able to look it another time.

There's nothing wrong with this patch, so I pushed it again to drm-xe-next.

(Well, by nothing wrong I mean... I would still prefer something to check the PCODE version
before in a way that the mb command is never submitted to start with, but, let's
live with this fix)

Although it ended up missing the last drm-next pr it will be part of the
-next-fixes flow and still be fixing 6.17 as well.

Thanks,
Rodrigo.

> > +	}
> >  
> >  	if (REG_FIELD_GET(V1_FAN_SUPPORTED, cap)) {
> >  		ret = sysfs_create_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
> > -- 
> > 2.34.1
> > 
> >