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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.