Re: [PATCH v1 1/4] drm/xe/pcode: Introduce xe_pcode_read_probe()

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Nov 20, 2025 at 09:02:29AM -0600, Lucas De Marchi wrote:
> On Tue, Nov 18, 2025 at 04:38:46PM +0100, Raag Jadav wrote:
> > > > +int xe_pcode_read_probe(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1);
> > > 
> > > Is "probe" the right condition? I mean, the right name for the exported function?
> > > 
> > > The caller is deciding to downgrade the Illegal command from error to debug, but
> > > is it because it is in the probe? Or because we know that most of FW out there
> > > might not have this command yet and driver knows that and will handle the
> > > lack of backward compatibility properly... in a way that this is not an error.
> > > 
> > > But is this 'probe' ?!
> > 
> > This is following Ville's original comment[1] as in 'probing' for something
> > that may or may not exist.
> > 
> > [1] https://lore.kernel.org/intel-xe/aQ3xItyGMVnKdzoi@intel.com/
> > 
> > > I'm bad with naming as well, so asking help from AI:
> > > 
> > > Alternative naming ideas:
> > > xe_pcode_read_optional
> > > (Indicates the command is optional and failure is acceptable.)
> > > xe_pcode_read_safe
> > > (Suggests a safe read that won't break if unsupported.)
> > > xe_pcode_read_tolerant
> > > (Highlights tolerance for missing command.)
> > > xe_pcode_try_read
> > > (Common pattern for non-fatal attempts.)
> > > xe_pcode_read_if_supported
> > > (Explicit about conditional support.)
> > 
> > I don't even begin to qualify here so it's upto you all.
> 
> xe_pcode_read_if_supported() would be ok IMO, documenting it to mask
> not-supported errors.
> 
> But the the way this is implemented with the extra flag seems weird.
> By "having the caller check" I think it's about handling
> the return code from this function and treating it as a fatal or normal
> case depending on the command being sent, if there's a fallback etc.
> This patch seems to add a function and not used it, but I may be missing
> something.

Forgot to doc. I had an impression that -ENXIO could be used for the
fallback since we already have it here but ...

> I'd rather have this:
> 
> 1) Caller should handle errors and treat it as fatal or normal,
>    depending on having a fallback or not. Emit an err there if
>    appropriate rather than here. It seems we are already emitting
>    additional dbgs in the caller for when pcode_read fails
> 
> 2) What is the command/subcommand triggering this error? We could have a
>    helper like xe_pcode_strerr() that users could call if needed (but
>    then we'd need to return the undecoded error), or we could change
>    this specific return code to -ENOTSUPP.

... converting to -ENOTSUPP makes much more sense, considering the undecoded
return will be inconsistent with other pcode helpers.

>    Is the error we are seeing the latebind from drivers/gpu/drm/xe/xe_late_bind_fw.c?

The errors are from all over but the one currently on fire is the hwmon.

Raag