From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Karthik Poosa <karthik.poosa@intel.com>
Cc: <intel-xe@lists.freedesktop.org>, <anshuman.gupta@intel.com>,
<badal.nilawar@intel.com>, <raag.jadav@intel.com>,
<riana.tauro@intel.com>, <sk.anirban@intel.com>,
<mallesh.koujalagi@intel.com>, <soham.purkait@intel.com>
Subject: Re: [PATCH] drm/xe/hwmon: Propagate all hardware read errors
Date: Mon, 12 Oct 2026 20:59:55 -0400 [thread overview]
Message-ID: <as2Ci8Vy9iTAN9r4@intel.com> (raw)
In-Reply-To: <20261012110648.253748-1-karthik.poosa@intel.com>
On Mon, Oct 12, 2026 at 04:36:48PM +0530, Karthik Poosa wrote:
> Propagate all failures from pcode and mailbox accesses and fix missed
> return-value checks found by Sashiko in
> https://patchwork.freedesktop.org/patch/758843/?series=174898&rev=6.
>
> Fixes: 7596d839f622 ("drm/xe/hwmon: Add support to manage power limits though mailbox")
> Fixes: 0c5405d3aa4ad ("drm/xe/hwmon: Read energy status from PMT")
> Fixes: 25e963a09e059 ("drm/xe/hwmon: Move card reactive critical power under channel card")
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
Please add this as part of your re-submission of the other v10 series for the CI
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/xe/xe_hwmon.c | 47 +++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 768566648bc8..fbcc04341e29 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -381,17 +381,20 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
> * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
> * clamped values when read.
> */
> -static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *value)
> +static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *value)
> {
> u32 reg_val = 0;
> struct xe_device *xe = hwmon->xe;
> struct xe_reg rapl_limit, pkg_power_sku;
> struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> + int ret = 0;
>
> mutex_lock(&hwmon->hwmon_lock);
>
> if (hwmon->xe->info.has_mbx_power_limits) {
> - xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
> + ret = xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
> + if (ret)
> + goto unlock;
> } else {
> rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
> @@ -423,6 +426,7 @@ static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, u32 attr, int channe
> }
> unlock:
> mutex_unlock(&hwmon->hwmon_lock);
> + return ret;
> }
>
> static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channel, long value)
> @@ -445,7 +449,9 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channe
> ret = xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> if (ret)
> goto unlock;
> - xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
> + ret = xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
> + if (ret)
> + goto unlock;
> } else {
> reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN, 0);
> reg_val = xe_mmio_read32(mmio, rapl_limit);
> @@ -502,15 +508,18 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channe
> return ret;
> }
>
> -static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, u32 attr, int channel,
> - long *value)
> +static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, u32 attr, int channel,
> + long *value)
> {
> struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
> u32 reg_val;
> + int ret = 0;
>
> if (hwmon->xe->info.has_mbx_power_limits) {
> /* PL1 is rated max if supported. */
> - xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, channel, ®_val);
> + ret = xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, channel, ®_val);
> + if (ret)
> + return ret;
> } else {
> /*
> * This sysfs file won't be visible if REG_PKG_POWER_SKU is invalid, so valid check
> @@ -524,6 +533,7 @@ static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, u32 attr, int
>
> reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
> *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> + return ret;
> }
>
> /*
> @@ -546,7 +556,7 @@ static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, u32 attr, int
> * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
> * energyN_input overflows. This at 1000 W is an overflow duration of 278 years.
> */
> -static void
> +static int
> xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
> {
> struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
> @@ -557,7 +567,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
> /* Energy is supported only for card and pkg */
> if (channel > CHANNEL_PKG) {
> *energy = 0;
> - return;
> + return 0;
> }
>
> if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
> @@ -569,7 +579,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
> if (ret != sizeof(pmt_val)) {
> drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
> *energy = 0;
> - return;
> + return ret < 0 ? ret : -EIO;
> }
>
> if (channel == CHANNEL_PKG)
> @@ -586,6 +596,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
>
> *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
> hwmon->scl_shift_energy);
> + return 0;
> }
>
> static ssize_t
> @@ -611,7 +622,8 @@ xe_hwmon_power_max_interval_show(struct device *dev, struct device_attribute *at
> drm_err(&hwmon->xe->drm,
> "power interval read fail, ch %d, attr %d, val 0x%08x, ret %d\n",
> channel, power_attr, reg_val, ret);
> - reg_val = 0;
> + mutex_unlock(&hwmon->hwmon_lock);
> + return ret;
> }
> } else {
> reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
> @@ -1391,7 +1403,8 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
> }
> case hwmon_power_crit:
> if (channel == CHANNEL_CARD) {
> - xe_hwmon_pcode_read_i1(hwmon, &uval);
> + if (xe_hwmon_pcode_read_i1(hwmon, &uval))
> + return 0;
> return (uval & POWER_SETUP_I1_WATTS) ? 0644 : 0;
> }
> break;
> @@ -1410,7 +1423,8 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
> }
> }
> if ((!(uval & PWR_LIM_EN)) && channel == CHANNEL_CARD) {
> - xe_hwmon_pcode_read_i1(hwmon, &uval);
> + if (xe_hwmon_pcode_read_i1(hwmon, &uval))
> + return 0;
> return (uval & POWER_SETUP_I1_WATTS) ? 0444 : 0;
> }
> return (uval) ? 0444 : 0;
> @@ -1426,11 +1440,9 @@ xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
> switch (attr) {
> case hwmon_power_max:
> case hwmon_power_cap:
> - xe_hwmon_power_max_read(hwmon, attr, channel, val);
> - return 0;
> + return xe_hwmon_power_max_read(hwmon, attr, channel, val);
> case hwmon_power_rated_max:
> - xe_hwmon_power_rated_max_read(hwmon, attr, channel, val);
> - return 0;
> + return xe_hwmon_power_rated_max_read(hwmon, attr, channel, val);
> case hwmon_power_crit:
> return xe_hwmon_power_curr_crit_read(hwmon, channel, val, SF_POWER);
> default:
> @@ -1547,8 +1559,7 @@ xe_hwmon_energy_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
> {
> switch (attr) {
> case hwmon_energy_input:
> - xe_hwmon_energy_get(hwmon, channel, val);
> - return 0;
> + return xe_hwmon_energy_get(hwmon, channel, val);
> default:
> return -EOPNOTSUPP;
> }
> --
> 2.25.1
>
next prev parent reply other threads:[~2026-10-13 1:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-12 11:06 [PATCH] drm/xe/hwmon: Propagate all hardware read errors Karthik Poosa
2026-10-13 0:59 ` Rodrigo Vivi [this message]
2026-10-13 2:20 ` Poosa, Karthik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=as2Ci8Vy9iTAN9r4@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=badal.nilawar@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=karthik.poosa@intel.com \
--cc=mallesh.koujalagi@intel.com \
--cc=raag.jadav@intel.com \
--cc=riana.tauro@intel.com \
--cc=sk.anirban@intel.com \
--cc=soham.purkait@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox