Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20230829002346.2104251-1-srinivas.pandruvada@linux.intel.com>
In-Reply-To: <20230829002346.2104251-1-srinivas.pandruvada@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 12 Sep 2023 16:09:10 +0200
Message-ID: <CAJZ5v0jrKSUOGG72w-EdzhQqC==CA7zYUofNSEW1wV-58TOnLw@mail.gmail.com>
Subject: Re: [PATCH v3 0/7] thermal: processor_thermal: Suport workload hint
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     daniel.lezcano@linaro.org, rafael@kernel.org, rui.zhang@intel.com,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Aug 29, 2023 at 2:23 AM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Add support for Meteor Lake workload hints. Before adding this support,
> some reorganization and clean up is required.
> First four changes are for clean up and to reorganize code to add
> support for workload hint. The last patch adds a test program as part
> of self tests.
>
> v3:
> Changes in the commit log
> Rename of files for using WT instead of WLT
> Address comments from Rafael on v2
>
> v2:
> Changes in comments and commit log
> Self test program is improved to disable workloadtype notification
> on exit
>
> Srinivas Pandruvada (7):
>   thermal: int340x: processor_thermal: Move mailbox code to common
>     module
>   thermal: int340x: processor_thermal: Add interrupt configuration
>   thermal: int340x: processor_thermal: Use non MSI interrupts by default
>   thermal/drivers/int340x: Remove PROC_THERMAL_FEATURE_WLT_REQ for
>     Meteor Lake
>   thermal: int340x: processor_thermal: Add workload type hint interface
>   thermal/drivers/int340x: Support workload hint interrupts
>   selftests/thermel/intel: Add test to read workload hint
>
>  .../driver-api/thermal/intel_dptf.rst         |  51 ++++
>  .../thermal/intel/int340x_thermal/Makefile    |   2 +
>  .../processor_thermal_device.c                |  17 +-
>  .../processor_thermal_device.h                |  21 +-
>  .../processor_thermal_device_pci.c            |  79 ++++--
>  .../processor_thermal_device_pci_legacy.c     |   3 +-
>  .../int340x_thermal/processor_thermal_mbox.c  | 179 ++++---------
>  .../processor_thermal_wt_hint.c               | 252 ++++++++++++++++++
>  .../processor_thermal_wt_req.c                | 136 ++++++++++
>  tools/testing/selftests/Makefile              |   1 +
>  .../thermal/intel/workload_hint/Makefile      |  12 +
>  .../intel/workload_hint/workload_hint_test.c  | 157 +++++++++++
>  12 files changed, 752 insertions(+), 158 deletions(-)
>  create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_hint.c
>  create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
>  create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/Makefile
>  create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/workload_hint_test.c
>
> --

There is a slight issue with the patch ordering in this series,
because the interface to enable the interrupt should only be provided
after implementing the interrupt handlers.  I don't think that anyone
will apply the series partially and try to enable the feature, though.

Also, I'm not actually sure if proc_thermal_wt_intr_callback() can run
safely against the work item scheduled in proc_thermal_irq_handler()
in case the workload hint one triggers along with a thermal threshold
one.  I think that the access to MMIO is cached, so what if they both
try to update the same cache line at the same time?  Or are they
guaranteed to be different cache lines?

Anyway, tentatively applied as 6.7 material, but I've changed the
second patch somewhat, because I couldn't convince myself that the
implicit type conversions in processor_thermal_mbox_interrupt_config()
would always do the right thing regardless of the numbers involved, so
please check the result in my bleeding-edge branch.

Thanks!