[PATCH] drm/xe/pcode: Initialize data0 for pcode read routine
Cavitt, Jonathan
jonathan.cavitt at intel.com
Fri Aug 8 16:38:46 UTC 2025
-----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
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