[PATCH] drm/xe/pcode: Initialize data0 for pcode read routine
Summers, Stuart
stuart.summers at intel.com
Fri Aug 8 17:01:14 UTC 2025
On Fri, 2025-08-08 at 16:38 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> stuartsummers
> Sent: Friday, August 8, 2025 8:37 AM
> Cc: intel-xe at lists.freedesktop.org; Jadav, Raag
> <raag.jadav at intel.com>; Summers, Stuart <stuart.summers at intel.com>
> Subject: [PATCH] drm/xe/pcode: Initialize data0 for pcode read
> routine
> >
> > There are two registered filled in when reading data from
> > pcode besides the mailbox itself. Currently we allow a NULL
> > value for the second of these two (data1) and assume the first
> > is defined. However many of the routines that are calling
> > this function assume that pcode will ignore the value being
> > passed in and so leave that first value (data0) defined but
> > uninitialized. To be safe, make sure this value is always
> > initialized to something (0 generally) in the event pcode
> > behavior changes and starts using this value.
> >
> > Signed-off-by: stuartsummers <stuart.summers at intel.com>
>
> s/registered/registers
>
> Debatably, you should also add commas after 'Currently' and
> 'However', but that's just a semantic nit-pick, and is basically
> just splitting hairs at that point. We're writing patches, after
> all, not college papers...
>
> ---
>
> Aside:
> It looks like this patch is initializing some previously
> uninitialized variables to zero
> as a guard against them not being set during xe_pcode_read. This is
> a necessary
> fix in the event we have the xe info skip_pcode flag set (as that
> causes the pcode
> reading function to return a success without actually doing
> anything). But in every
> other case, I'm failing to see how the guards we currently have in
> place (namely,
> checking the return value and aborting the function if the read
> fails) aren't sufficient
> to prevent us from using an uninitialized variable in the case of a
> read failure. And
Thanks for the comment here Jonathan and the review!
You're refering to the pcode status check that returns one of the list
of errors if that shows up after the read/write? I guess what I'm
thinking there is having a known value is better than an unknown value
being sent down. It could also be that 0 (or any other number) is a
valid value and we get a successful status. At least with 0 (or some
other well-known value) we can have something consistent there.
Thanks,
Stuart
> if it's to guard against a future where the read function needs extra
> data, I think I'd
> rather we just create a new variable to store that extra data, rather
> than coopting
> the data0 write location for that purpose.
>
> Of course, the skip_pcode case is sufficient enough to warrant this
> change by itself,
> so I'm not going to block this. The skip_pcode flag is set to TRUE
> by default for SRIOV
> VF platforms, so I'm surprised breakages haven't been detected sooner
> given the
> uninitialized variables are quite liable to contain garbage data
> under some (or most)
> common kernel compiler configurations.
>
> An explanation would be nice, but it's not necessary. I mostly give
> these asides
> as proof that I actually read the patch, rather than as blocking
> queries, so you
> can take my reviewed-by without any strings attached. The grammar
> fixes can
> be applied at merge time.
>
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> -Jonathan Cavitt
>
> > ---
> > drivers/gpu/drm/xe/xe_device_sysfs.c | 8 ++++----
> > drivers/gpu/drm/xe/xe_hwmon.c | 8 ++++----
> > drivers/gpu/drm/xe/xe_vram_freq.c | 4 ++--
> > 3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > index bd9015761aa0..6ee422594b56 100644
> > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > @@ -76,7 +76,7 @@ lb_fan_control_version_show(struct device *dev,
> > struct device_attribute *attr, c
> > {
> > struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> > struct xe_tile *root = xe_device_get_root_tile(xe);
> > - u32 cap, ver_low = FAN_TABLE, ver_high = FAN_TABLE;
> > + u32 cap = 0, ver_low = FAN_TABLE, ver_high = FAN_TABLE;
> > u16 major = 0, minor = 0, hotfix = 0, build = 0;
> > int ret;
> >
> > @@ -115,7 +115,7 @@ lb_voltage_regulator_version_show(struct device
> > *dev, struct device_attribute *a
> > {
> > struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> > struct xe_tile *root = xe_device_get_root_tile(xe);
> > - u32 cap, ver_low = VR_CONFIG, ver_high = VR_CONFIG;
> > + u32 cap = 0, ver_low = VR_CONFIG, ver_high = VR_CONFIG;
> > u16 major = 0, minor = 0, hotfix = 0, build = 0;
> > int ret;
> >
> > @@ -153,7 +153,7 @@ static int late_bind_create_files(struct device
> > *dev)
> > {
> > struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> > struct xe_tile *root = xe_device_get_root_tile(xe);
> > - u32 cap;
> > + u32 cap = 0;
> > int ret;
> >
> > xe_pm_runtime_get(xe);
> > @@ -186,7 +186,7 @@ static void late_bind_remove_files(struct
> > device *dev)
> > {
> > struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> > struct xe_tile *root = xe_device_get_root_tile(xe);
> > - u32 cap;
> > + u32 cap = 0;
> > int ret;
> >
> > xe_pm_runtime_get(xe);
> > diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
> > b/drivers/gpu/drm/xe/xe_hwmon.c
> > index f08fc4377d25..17a324e9de82 100644
> > --- a/drivers/gpu/drm/xe/xe_hwmon.c
> > +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> > @@ -179,7 +179,7 @@ static int xe_hwmon_pcode_rmw_power_limit(const
> > struct xe_hwmon *hwmon, u32 attr
> > u32 clr, u32 set)
> > {
> > struct xe_tile *root_tile = xe_device_get_root_tile(hwmon-
> > >xe);
> > - u32 val0, val1;
> > + u32 val0 = 0, val1 = 0;
> > int ret = 0;
> >
> > ret = xe_pcode_read(root_tile,
> > PCODE_MBOX(PCODE_POWER_SETUP,
> > @@ -719,7 +719,7 @@ static int xe_hwmon_power_curr_crit_read(struct
> > xe_hwmon *hwmon, int channel,
> > long *value, u32
> > scale_factor)
> > {
> > int ret;
> > - u32 uval;
> > + u32 uval = 0;
> >
> > mutex_lock(&hwmon->hwmon_lock);
> >
> > @@ -889,7 +889,7 @@ xe_hwmon_power_write(struct xe_hwmon *hwmon,
> > u32 attr, int channel, long val)
> > static umode_t
> > xe_hwmon_curr_is_visible(const struct xe_hwmon *hwmon, u32 attr,
> > int channel)
> > {
> > - u32 uval;
> > + u32 uval = 0;
> >
> > /* hwmon sysfs attribute of current available only for
> > package */
> > if (channel != CHANNEL_PKG)
> > @@ -991,7 +991,7 @@ xe_hwmon_energy_read(struct xe_hwmon *hwmon,
> > u32 attr, int channel, long *val)
> > static umode_t
> > xe_hwmon_fan_is_visible(struct xe_hwmon *hwmon, u32 attr, int
> > channel)
> > {
> > - u32 uval;
> > + u32 uval = 0;
> >
> > if (!hwmon->xe->info.has_fan_control)
> > return 0;
> > diff --git a/drivers/gpu/drm/xe/xe_vram_freq.c
> > b/drivers/gpu/drm/xe/xe_vram_freq.c
> > index b26e26d73dae..17bc84da4cdc 100644
> > --- a/drivers/gpu/drm/xe/xe_vram_freq.c
> > +++ b/drivers/gpu/drm/xe/xe_vram_freq.c
> > @@ -34,7 +34,7 @@ static ssize_t max_freq_show(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > struct xe_tile *tile = dev_to_tile(dev);
> > - u32 val, mbox;
> > + u32 val = 0, mbox;
> > int err;
> >
> > mbox = REG_FIELD_PREP(PCODE_MB_COMMAND,
> > PCODE_FREQUENCY_CONFIG)
> > @@ -56,7 +56,7 @@ static ssize_t min_freq_show(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > struct xe_tile *tile = dev_to_tile(dev);
> > - u32 val, mbox;
> > + u32 val = 0, mbox;
> > int err;
> >
> > mbox = REG_FIELD_PREP(PCODE_MB_COMMAND,
> > PCODE_FREQUENCY_CONFIG)
> > --
> > 2.34.1
> >
> >
More information about the Intel-xe
mailing list