linux-kernel - Re: [PATCH] hwmon: Add MSI PSU HID monitoring 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]

Date: Fri, 2 Feb 2024 11:24:39 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Jack Doan <me@...kdoan.com>
Cc: jdelvare@...e.com, corbet@....net, linux-hwmon@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: Add MSI PSU HID monitoring driver

On Mon, Jan 08, 2024 at 11:56:04AM -0700, Jack Doan wrote:
> This driver provides a sysfs interface for MSI power supplies with a
> USB-HID monitoring interface.
> 
> Measurements for the output voltage and current for each rail are provided,
> as well as total output power, temperature, and fan control.
> 
> This patch adds:
> - hwmon driver msi-psu
> - hwmon documentation
> - updates MAINTAINERS
> 
> Signed-off-by: Jack Doan <me@...kdoan.com>
> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/msi-psu.rst |  64 +++
>  MAINTAINERS                     |   7 +
>  drivers/hwmon/Kconfig           |  12 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/msi-psu.c         | 801 ++++++++++++++++++++++++++++++++
>  6 files changed, 886 insertions(+)
>  create mode 100644 Documentation/hwmon/msi-psu.rst
>  create mode 100644 drivers/hwmon/msi-psu.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 72f4e6065bae..34e4bc086bdb 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -159,6 +159,7 @@ Hardware Monitoring Kernel Drivers
>     mp2888
>     mp2975
>     mp5023
> +   msi-psu
>     nct6683
>     nct6775
>     nct7802
> diff --git a/Documentation/hwmon/msi-psu.rst b/Documentation/hwmon/msi-psu.rst
> new file mode 100644
> index 000000000000..3dda7190a627
> --- /dev/null
> +++ b/Documentation/hwmon/msi-psu.rst
> @@ -0,0 +1,64 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver msi-psu
> +=========================
> +
> +Supported devices:
> +
> +* MSI MEG Ai1300P
> +
> +* MSI MEG Ai1000P
> +
> +Author: Jack Doan
> +
> +Description
> +-----------
> +
> +This driver provides a sysfs interface for MSI PSUs with a HID monitoring
> +interface.
> +
> +Measurements for the output voltage and current for each rail are provided,
> +as well as total output power, temperature, and fan control.
> +
> +Additional properties are available in debugfs, such as an efficiency
> +measurement, and switching to/from 12V multi-rail mode
> +
> +Sysfs entries
> +-------------
> +
> +============    ===============================================================
> +curr1_input     Current on the 12v psu rail
> +curr2_input     Current on the 5v psu rail
> +curr3_input     Current on the 3.3v psu rail
> +fan1_input      RPM of psu fan
> +in0_input       Voltage of the psu ac input
> +in1_input       Voltage of the 12v psu rail
> +in2_input       Voltage of the 5v psu rail
> +in3_input       Voltage of the 3.3v psu rail
> +power1_input    Total power usage
> +pwm1            PWM value for fan1. Writes to this file will switch set
> +                pwm1_enable to manual control mode.

No, that is unexpected and not supposed to happen.

> +pwm1_enable     PWM mode for fan1. (1) means "auto", and uses the built-in fan
> +                curve. (3) means manual control

The ABI (Documentation/ABI/testing/sysfs-class-hwmon) says:

                - 0: no fan speed control (i.e. fan at full speed)
                - 1: manual fan speed control enabled (using `pwmY`)
                - 2+: automatic fan speed control enabled

I really do not see the point of declaring that 1 shall mean automatic,
or to skip 2.

..

> +#define REPLY_SIZE 40 /* max length of a reply to a single command */

#define<space>NAME<tab>value

for all defines, please.

Guenter