Re: [PATCH 0/7] Create Intel PMC SSRAM Telemetry driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 9 Aug 2024, Xi Pardee wrote:

> This patch series removes the SSRAM support from Intel PMC Core driver
> and creates a separate PCI driver for SSRAM device. The new Intel PMC
> SSRAM driver provides the following functionalities:
>  
> 1. Search and store the PMC information in a structure, including PWRMBASE
> address and devid for each available PMC. Then Intel PMC Core driver
> achieves the PMC information using the API provided by the new driver.
>  
> 2. Search and register Intel Platform Monitoring Techology telemetry
> regions so they would by available for read through sysfs and Intel PMT
> API. Intel PMC Core driver can achieve Low Power Mode requirement
> information from a telemetry region registered by the new driver.
> The above functionalities was previously handled by Intel PMC Core
> driver. Intel PMC Core driver returns -EPROBE_DEFER when trying to read
> data from a telem region that is not available yet. This setup may
> result in an infinite loop of .probe() calls as Intel PMC Core driver
> creates child devices. Creating a separate PCI driver avoids the infinite
> loop possibility.
>  
> Xi Pardee (7):
>   platform/x86:intel/pmc: Remove SSRAM support from PMC Core
>   platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver
>   platform/x86:intel/pmc: Add support to get PMC information from SSRAM
>   platform/x86:intel/pmt: Get PMC from SSRAM for available platforms
>   platform/x86:intel/pmt: Create inline version for telemetry functions
>   platform/x86:intel/pmc: Add support to Retrieve LPM information
>   platform/x86:intel/pmc: Get LPM information for available platforms

Hi,

I don't see why the removal first, then re-add approach would be justified 
here. You're basically adding the same code back later in many cases with 
only very minimal changes, and some changes are entirely pointless such as 
pmc_idx -> pmc_index parameter rename. This is just a big pain to review.

I'd suggest you move functions in first patch into core.c. Try to 
avoid logic/code changes other than making making the necessary functions 
non-static and adding the prototypes for them into a header (temporarily).

Then rename the ssram file to its new name in the second change.

Then do the rework on top of that (and make things back static again).

Try to split the rework into sensible chunks, anything that can be taken 
away from the main rework change is less lines to review in that patch.
If you e.g. want to do pcidev -> pdev renames, put them into own separate 
change (and do it consistently then, not just for some of the cases like
currently :-/).

The move patches are nearly trivial to review and take large chunk of 
diff away from the actual rework itself which doesn't seem that 
complicated to review once the 1:1 move bits and trivial rename churn is
eliminated from the diff.

>  drivers/platform/x86/intel/pmc/Kconfig        |  13 +-
>  drivers/platform/x86/intel/pmc/Makefile       |   8 +-
>  drivers/platform/x86/intel/pmc/arl.c          |  36 +-
>  drivers/platform/x86/intel/pmc/core.c         | 216 +++++++++++-
>  drivers/platform/x86/intel/pmc/core.h         |  25 +-
>  drivers/platform/x86/intel/pmc/core_ssram.c   | 326 ------------------
>  drivers/platform/x86/intel/pmc/lnl.c          |  36 +-
>  drivers/platform/x86/intel/pmc/mtl.c          |  34 +-
>  .../platform/x86/intel/pmc/ssram_telemetry.c  | 184 ++++++++++
>  .../platform/x86/intel/pmc/ssram_telemetry.h  |  45 +++
>  drivers/platform/x86/intel/pmt/telemetry.h    |  19 +-
>  11 files changed, 550 insertions(+), 392 deletions(-)
>  delete mode 100644 drivers/platform/x86/intel/pmc/core_ssram.c
>  create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.c
>  create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
> 
> 

-- 
 i.