linux-kernel - Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

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: <20251023075050.254998-1-igor@reznichenko.net>
Date: Thu, 23 Oct 2025 00:50:50 -0700
From: Igor Reznichenko <igor@...nichenko.net>
To: linux@...ck-us.net
Cc: conor+dt@...nel.org,
	corbet@....net,
	david.hunter.linux@...il.com,
	devicetree@...r.kernel.org,
	igor@...nichenko.net,
	krzk+dt@...nel.org,
	linux-doc@...r.kernel.org,
	linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	robh@...nel.org,
	skhan@...uxfoundation.org
Subject: Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

Guenter,
Thanks for the detailed feedback. I will address it.

> Please send a register dump.

Here's register dump after init during run: 

tsc1641 1-0040: 0x00: 0x003f
tsc1641 1-0040: 0x01: 0x0253
tsc1641 1-0040: 0x02: 0x0dc0
tsc1641 1-0040: 0x03: 0x0053
tsc1641 1-0040: 0x04: 0x0250
tsc1641 1-0040: 0x05: 0x0033
tsc1641 1-0040: 0x06: 0x0000
tsc1641 1-0040: 0x07: 0x0000
tsc1641 1-0040: 0x08: 0x01f4
tsc1641 1-0040: 0x09: 0x0000
tsc1641 1-0040: 0x0a: 0x0000
tsc1641 1-0040: 0x0b: 0x0000
tsc1641 1-0040: 0x0c: 0x0000
tsc1641 1-0040: 0x0d: 0x0000
tsc1641 1-0040: 0x0e: 0x0000
tsc1641 1-0040: 0xfe: 0x0006
tsc1641 1-0040: 0xff: 0x1000

> > +
> > +	/*
> > +	 * Disable alert mask first, then write the value and enable alert mask
> Why ? 

The idea was to prevent potential previous alert from propagating when changing 
the value, plus to only enable alert when crit/lcrit value is non-zero. 
But given your response below this is not the right thing to do.

> Disabling alerts if the limit is 0 is wrong: The limit can be set
> to 0 on purpose. Only unmasking the limit if a limit is set is just as wrong.
> Either limits are enabled and reported, or they are disabled and the attributes
> must not be generated. Mis-using the ABI to declare "If the limit value is
> 0, mask the limit. Otherwise set the limit and unmask it" is unacceptable.

Thanks for clarification. So would you recommend then that all alerts should 
be always on/unmasked for this chip or to add custom sysfs attributes to control 
them, since it has this capability?

> Either report as standard voltage (in0_input) or drop entirely.
> The shunt voltage can be calculated from the shunt resisor value and
> the current. A non-standard attribute to report it does not add value.

I'll drop it since the shunt voltage resolution is 2.5uV and it won't give 
accurate information to report it in mV.

Thanks, Igor