| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Jun 2023 10:48:31 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Nícolas F. R. A. Prado
<nfraprado@...labora.com>
Cc: kernel@...labora.com, Alexandre Mergnat <amergnat@...libre.com>,
Balsam CHIHI <bchihi@...libre.com>,
Chen-Yu Tsai <wenst@...omium.org>,
Alexandre Bailon <abailon@...libre.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Amit Kucheria <amitk@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Zhang Rui <rui.zhang@...el.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 2/6] thermal/drivers/mediatek/lvts_thermal: Honor
sensors in immediate mode
On 04/05/2023 02:48, Nícolas F. R. A. Prado wrote:
> Each controller can be configured to operate on immediate or filtered
> mode. On filtered mode, the sensors are enabled by setting the
> corresponding bits in MONCTL0, while on immediate mode, by setting
> MSRCTL1.
>
> Previously, the code would set MSRCTL1 for all four sensors when
> configured to immediate mode, but given that the controller might not
> have all four sensors connected, this would cause interrupts to trigger
> for non-existent sensors. Fix this by handling the MSRCTL1 register
> analogously to the MONCTL0: only enable the sensors that were declared.
>
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> Tested-by: Chen-Yu Tsai <wenst@...omium.org>
> ---
>
> (no changes since v1)
>
> drivers/thermal/mediatek/lvts_thermal.c | 50 +++++++++++++------------
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index 2988f201633a..f7d998a45ea0 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -922,24 +922,6 @@ static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> LVTS_HW_FILTER << 3 | LVTS_HW_FILTER;
> writel(value, LVTS_MSRCTL0(lvts_ctrl->base));
>
> - /*
> - * LVTS_MSRCTL1 : Measurement control
> - *
> - * Bits:
> - *
> - * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
> - * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
> - * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
> - * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
> - *
> - * That configuration will ignore the filtering and the delays
> - * introduced below in MONCTL1 and MONCTL2
> - */
> - if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
> - value = BIT(9) | BIT(6) | BIT(5) | BIT(4);
> - writel(value, LVTS_MSRCTL1(lvts_ctrl->base));
> - }
> -
> /*
> * LVTS_MONCTL1 : Period unit and group interval configuration
> *
> @@ -1004,6 +986,8 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> struct lvts_sensor *lvts_sensors = lvts_ctrl->sensors;
> struct thermal_zone_device *tz;
> u32 sensor_map = 0;
> + u32 sensor_map_bits[][4] = {{BIT(4), BIT(5), BIT(6), BIT(9)},
> + {BIT(0), BIT(1), BIT(2), BIT(3)}};
Even correct, this initialization sounds prone to error and a bit
obfuscating (eg. it assumes LVTS_MSR_IMMEDIATE_MODE is 1).
What about?
/*
* A description
*/
u32 sensor_imm_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
u32 sensor_filt_bitmap[] = { BIT(4), BIT(5), BIT(6), BIT(9) };
u32 *sensor_bitmap = lvts_ctrl->mode ? LVTS_MSR_IMMEDIATE_MODE :
sensor_imm_bitmap : sensor_filt_bitmap;
> int i;
>
> for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) {
> @@ -1040,20 +1024,38 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> * map, so we can enable the temperature monitoring in
> * the hardware thermal controller.
> */
> - sensor_map |= BIT(i);
> + sensor_map |= sensor_map_bits[lvts_ctrl->mode][i];
sensor_map |= sensor_bitmap[i];
> }
>
> /*
> - * Bits:
> - * 9: Single point access flow
> - * 0-3: Enable sensing point 0-3
> - *
> * The initialization of the thermal zones give us
> * which sensor point to enable. If any thermal zone
> * was not described in the device tree, it won't be
> * enabled here in the sensor map.
> */
> - writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> + if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
> + /*
> + * LVTS_MSRCTL1 : Measurement control
> + *
> + * Bits:
> + *
> + * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
> + * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
> + * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
> + * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
> + *
> + * That configuration will ignore the filtering and the delays
> + * introduced in MONCTL1 and MONCTL2
> + */
> + writel(sensor_map, LVTS_MSRCTL1(lvts_ctrl->base));
> + } else {
> + /*
> + * Bits:
> + * 9: Single point access flow
> + * 0-3: Enable sensing point 0-3
> + */
> + writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> + }
>
> return 0;
> }
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Powered by blists - more mailing lists