Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Feedback-ID: ibe194615:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-745-g95dd7bea33-fm-20230905.001-g95dd7bea
Mime-Version: 1.0
Message-Id: <55f22980-b47a-4a22-8f19-2b0a5b4e6a1a@app.fastmail.com>
In-Reply-To: <TYZPR03MB59949F797738F5B1B8638278BDF6A@TYZPR03MB5994.apcprd03.prod.outlook.com>
References: <20230915150340.301067-1-dober6023@gmail.com>
 <8a566102-5ea6-4449-9083-8feebe711065@roeck-us.net>
 <TYZPR03MB59949F797738F5B1B8638278BDF6A@TYZPR03MB5994.apcprd03.prod.outlook.com>
Date:   Fri, 15 Sep 2023 18:43:02 -0400
From:   "Mark Pearson" <mpearson-lenovo@squebb.ca>
To:     "Guenter Roeck" <groeck7@gmail.com>
Cc:     "David Ober" <dober6023@gmail.com>, linux-hwmon@vger.kernel.org,
        linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, jdelvare@suse.com, corbet@lwn.net,
        "David Ober" <dober@lenovo.com>
Subject: Re: [PATCH] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards
Content-Type: text/plain
Precedence: bulk

Hi Guenter,

> From: Guenter Roeck <groeck7@gmail.com> on behalf of Guenter Roeck 
> 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.
>
I assume for this we just need confirmation from the BIOS team that this is how it will be handled and it's intentional by design?

Agreed this is normally done by ACPI, but my understanding is that it's not the case on these particular workstation platforms. FWIW Windows is also doing access by a separate driver. 
I'm not sure why the design is done this way but will confirm to make sure.

With regards to guaranteeing that no other kernel drivers access the chip - I'm not sure how we can ensure that. Or do you mean if another vendor is using this chip but with different platform IDs and want to use a similar driver? 
For this case we can make the driver generic (rename it mec172x.c) so others could add their platform support in the future (the platform IDs will be unique). Either that or I can confirm with Microchip if this particular chip is Lenovo specific.

Let me know if I'm misunderstanding or missing something obvious here. Ack on reserving the region.

>> ---
>>  drivers/hwmon/Kconfig             |  10 +
>>  drivers/hwmon/Makefile            |   1 +
>>  drivers/hwmon/lenovo-ec-sensors.c | 471 ++++++++++++++++++++++++++++++
>
> Documentation missing.
Ack. I assume under Documentation/hwmon

Thanks for the review
Mark