linux-kernel - Re: [PATCH] pwm: mc33xs2410: add support for temperature sensors

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] [day] [month] [year] [list]

Message-ID: <20250512165252.GA11091@legfed1>
Date: Mon, 12 May 2025 18:52:52 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: dimitri.fedrau@...bherr.com,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Jean Delvare <jdelvare@...e.com>, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH] pwm: mc33xs2410: add support for temperature sensors

Am Mon, May 12, 2025 at 06:53:21AM -0700 schrieb Guenter Roeck:
> On 5/12/25 06:31, Dimitri Fedrau wrote:
> > Hi Guenter,
> > 
> > Am Mon, May 12, 2025 at 06:04:33AM -0700 schrieb Guenter Roeck:
> > > On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote:
> > > > From: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > > > 
> > > > The MC33XS2410 provides temperature sensors for the central die temperature
> > > > and the four outputs. Additionally a common temperature warning threshold
> > > > can be configured for the outputs. Add hwmon support for the sensors.
> > > > 
> > > > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > > > ---
> > > 
> > > > +
> > > > +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi,
> > > > +					    int channel, u16 *val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	/* Bits latches high */
> > > > +	return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val);
> > > 
> > > Is that double read of the same register needed ? If so, you'll probably
> > > need a lock to prevent it from being executed from multiple threads at the
> > > same time.
> > > 
> > > The comment "Bit latches high" doesn't really mean anything to me and doesn't
> > > explain why the register needs to be read twice.
> > > 
> > > 
> > 
> > All bits of the output status registers are latched high. In case there
> > was overtemperature detected, the bit stays set until read once and cleared
> > afterwards. So I need a second read to get the "realtime" status.
> > Otherwise I might end up returning an false positive overtemperature
> > warning. I don't think a lock is really necessary, since I'm only
> > interested in the "realtime" status but not if there was a warning in
> > the past. What do you think ?
> > 
> 
> Hardware monitoring is _expected_ to report the last latched status and clear
> it afterwards, to ensure that historic alarms are reported at least once.
> This isn't about "false positive", it is about "report at least once if
> possible".
>
Didn't know that, thanks for the explanation.

> Given that, the second read is unnecessary from hwmon ABI perspective. If you
> don't want to do that, you should explicitly document that latched (historic)
> over-temperature alarms are not reported.
> 
I would stick to hwmon ABI, just didn't know better.

Best regards,
Dimitri Fedrau