linux-kernel - Re: [PATCH v3 6/9] drivers: thermal: tsens: Add watchdog support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200102195537.GE988120@minitux>
Date:   Thu, 2 Jan 2020 11:55:37 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Amit Kucheria <amit.kucheria@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        swboyd@...omium.org, sivaa@...eaurora.org,
        Andy Gross <agross@...nel.org>,
        Amit Kucheria <amit.kucheria@...durent.com>,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 6/9] drivers: thermal: tsens: Add watchdog support

On Thu 02 Jan 06:54 PST 2020, Amit Kucheria wrote:
[..]
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 4cf550766cf6..ecbc722eb348 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -377,6 +377,24 @@ irqreturn_t tsens_critical_irq_thread(int irq, void *data)
>  	struct tsens_irq_data d;
>  	unsigned long flags;
>  	int temp, ret, i;
> +	u32 wdog_status, wdog_count;
> +
> +	if (priv->feat->has_watchdog) {
> +		/* Watchdog is present only on v2.3+ */

Please omit this comment, you're carrying the motivation for this
decision when you set has_watchdog already.

> +		ret = regmap_field_read(priv->rf[WDOG_BARK_STATUS], &wdog_status);
> +		if (ret)
> +			return ret;
> +
> +		/* Clear WDOG interrupt */
> +		regmap_field_write(priv->rf[WDOG_BARK_CLEAR], 1);
> +		regmap_field_write(priv->rf[WDOG_BARK_CLEAR], 0);

Are you sure you need to zero the clear register?

> +
> +		ret = regmap_field_read(priv->rf[WDOG_BARK_COUNT], &wdog_count);
> +		if (ret)
> +			return ret;
> +		if (wdog_count)
> +			dev_dbg(priv->dev, "%s: watchdog count: %d\n", __func__, wdog_count);
> +	}
>  
[..]
> @@ -793,6 +815,22 @@ int __init init_common(struct tsens_priv *priv)
>  		}
>  	}
>  
> +	if (tsens_version(priv) > VER_1_X &&  ver_minor > 2) {
> +		/* Watchdog is present only on v2.3+ */
> +		priv->feat->has_watchdog = 1;
> +		for (i = WDOG_BARK_STATUS; i <= CC_MON_MASK; i++) {
> +			priv->rf[i] = devm_regmap_field_alloc(dev, priv->tm_map,
> +							      priv->fields[i]);
> +			if (IS_ERR(priv->rf[i])) {
> +				ret = PTR_ERR(priv->rf[i]);
> +				goto err_put_device;
> +			}
> +		}
> +		/* Enable WDOG and disable cycle completion monitoring */

Commit message says you're not enabling it. Should this say "WDOG is
already configured, unmask the bark" or something along those lines?

> +		regmap_field_write(priv->rf[WDOG_BARK_MASK], 0);
> +		regmap_field_write(priv->rf[CC_MON_MASK], 1);
> +	}
> +
>  	spin_lock_init(&priv->ul_lock);
>  	tsens_enable_irq(priv);
>  	tsens_debug_init(op);

Regards,
Bjorn