Received-SPF: pass (google.com: domain of linux-kernel+bounces-203593-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <cf24f2193c16ed070e5ec3b2f601650eb5b867ed.camel@linux.intel.com>
Subject: Re: [PATCH v1] thermal: core: Do not fail cdev registration because
 of invalid initial state
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>, Linux PM
 <linux-pm@vger.kernel.org>
Cc: Linux ACPI <linux-acpi@vger.kernel.org>, LKML
 <linux-kernel@vger.kernel.org>,  Lukasz Luba <lukasz.luba@arm.com>, Zhang
 Rui <rui.zhang@intel.com>, Daniel Lezcano <daniel.lezcano@linaro.org>,
 Laura Nao <laura.nao@collabora.com>
Date: Wed, 05 Jun 2024 20:41:22 -0700
In-Reply-To: <4569763.LvFx2qVVIh@kreacher>
References: <4569763.LvFx2qVVIh@kreacher>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.0-1 
Precedence: bulk
MIME-Version: 1.0

On Wed, 2024-06-05 at 21:17 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>=20
> It is reported that commit 31a0fa0019b0 ("thermal/debugfs: Pass
> cooling
> device state to thermal_debug_cdev_add()") causes the ACPI fan driver
> to fail probing on some systems which turns out to be due to the _FST
> control method returning an invalid value until _FSL is first
> evaluated
> for the given fan.=C2=A0 If this happens, the .get_cur_state() cooling
> device
> callback returns an error and __thermal_cooling_device_register()
> fails
> as uses that callback after commit 31a0fa0019b0.
>=20
> Arguably, _FST should not return an inavlid
s/inavlid/invalid

Thanks,
Srinivas

>  value even if it is
> evaluated before _FSL, so this may be regarded as a platform firmware
> issue, but at the same time it is not a good enough reason for
> failing
> the cooling device registration where the initial cooling device
> state
> is only needed to initialize a thermal debug facility.
>=20
> Accordingly, modify __thermal_cooling_device_register() to pass a
> negative state value to thermal_debug_cdev_add() instead of failing
> if the initial .get_cur_state() callback invocation fails and adjust
> the thermal debug code to ignore negative cooling device state
> values.
>=20
> Fixes: 31a0fa0019b0 ("thermal/debugfs: Pass cooling device state to
> thermal_debug_cdev_add()")
> Closes:
> https://lore.kernel.org/linux-acpi/20240530153727.843378-1-laura.nao@coll=
abora.com
> Reported-by: Laura Nao <laura.nao@collabora.com>
> Tested-by: Laura Nao <laura.nao@collabora.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> =C2=A0drivers/thermal/thermal_core.c=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 11 +=
++++++----
> =C2=A0drivers/thermal/thermal_debugfs.c |=C2=A0=C2=A0=C2=A0 7 ++++++-
> =C2=A02 files changed, 13 insertions(+), 5 deletions(-)
>=20
> Index: linux-pm/drivers/thermal/thermal_core.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -964,7 +964,8 @@ __thermal_cooling_device_register(struct
> =C2=A0{
> =C2=A0	struct thermal_cooling_device *cdev;
> =C2=A0	struct thermal_zone_device *pos =3D NULL;
> -	unsigned long current_state;
> +	unsigned long val;
> +	int current_state;
> =C2=A0	int id, ret;
> =C2=A0
> =C2=A0	if (!ops || !ops->get_max_state || !ops->get_cur_state ||
> @@ -1002,9 +1003,11 @@ __thermal_cooling_device_register(struct
> =C2=A0	if (ret)
> =C2=A0		goto out_cdev_type;
> =C2=A0
> -	ret =3D cdev->ops->get_cur_state(cdev, &current_state);
> -	if (ret)
> -		goto out_cdev_type;
> +	ret =3D cdev->ops->get_cur_state(cdev, &val);
> +	if (!ret && val >=3D 0 && val <=3D INT_MAX)
> +		current_state =3D val;
> +	else
> +		current_state =3D -1;
> =C2=A0
> =C2=A0	thermal_cooling_device_setup_sysfs(cdev);
> =C2=A0
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -421,6 +421,8 @@ void thermal_debug_cdev_state_update(con
> =C2=A0	cdev_dbg =3D &thermal_dbg->cdev_dbg;
> =C2=A0
> =C2=A0	old_state =3D cdev_dbg->current_state;
> +	if (old_state < 0)
> +		goto unlock;
> =C2=A0
> =C2=A0	/*
> =C2=A0	 * Get the old state information in the durations list. If
> @@ -463,6 +465,7 @@ void thermal_debug_cdev_state_update(con
> =C2=A0
> =C2=A0	cdev_dbg->total++;
> =C2=A0
> +unlock:
> =C2=A0	mutex_unlock(&thermal_dbg->lock);
> =C2=A0}
> =C2=A0
> @@ -499,7 +502,9 @@ void thermal_debug_cdev_add(struct therm
> =C2=A0	 * duration will be printed by cdev_dt_seq_show() as
> expected if it
> =C2=A0	 * runs before the first state transition.
> =C2=A0	 */
> -	thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg-
> >durations, state);
> +	if (state >=3D 0)
> +		thermal_debugfs_cdev_record_get(thermal_dbg,
> cdev_dbg->durations,
> +						state);
> =C2=A0
> =C2=A0	debugfs_create_file("trans_table", 0400, thermal_dbg->d_top,
> =C2=A0			=C2=A0=C2=A0=C2=A0 thermal_dbg, &tt_fops);
>=20
>=20
>=20
>=20