Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Fri, 15 Sep 2023 14:19:52 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     David Ober <dober6023@gmail.com>
Cc:     linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, jdelvare@suse.com, corbet@lwn.net,
        dober@lenovo.com, mpearson@lenovo.com
Subject: Re: [PATCH] hwmon:Add MEC172x Micro Chip driver for Lenovo
 motherboards
Message-ID: <8a566102-5ea6-4449-9083-8feebe711065@roeck-us.net>
References: <20230915150340.301067-1-dober6023@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230915150340.301067-1-dober6023@gmail.com>
Precedence: bulk

On Fri, Sep 15, 2023 at 11:03:40AM -0400, David Ober wrote:
> This addition adds in the ability for the system to scan the
> MEC172x EC chip in Lenovo ThinkStation systems to get the
> current fan RPM speeds and the Maximum speed value for each
> fan also provides the current CPU and DIMM thermal status
> 
> Signed-off-by: David Ober <dober6023@gmail.com>
> 
> Written by David Ober from Lenovo using this gmail address since
> my corporate email address does not comply with git email

FWIW, this needs to be after '---'

Anyway, thinking about this submission makes me even more concerned.

This isn't really a driver for MEC172x; it is simply a driver
accessing an EC on a set of PCs and/or laptops from Lenovo
which uses a vertain API for communication between EC and main
CPU.

Such ECs are typically accessed through ACPI. Yet, in this driver
there is no mention of ACPI, much less any protection against
parallel use by ACPI code (that access lock in get_ec_reg() doesn't
even protect against parallel access from userspace, much less
against parallel access from other drivers or ACPI, for example
by using request_region() to reserve the used memory ranges).

There needs to be explanations and clarifications 
- Why this driver will only be used for communication with MEC172X
  based chips, and why the exact EC chip is relevant in the first place
  to be mentioned as much as it is.
- How it is guaranteed that the EC is not and will never be accessed
  through ACPI.
- How it is guaranteed that there will never be any other kernel drivers
  accessing the chip.

> ---
>  drivers/hwmon/Kconfig             |  10 +
>  drivers/hwmon/Makefile            |   1 +
>  drivers/hwmon/lenovo-ec-sensors.c | 471 ++++++++++++++++++++++++++++++

Documentation missing.

Guenter