MIME-Version: 1.0
References: <20230221180710.2781027-1-daniel.lezcano@linaro.org>
 <20230221180710.2781027-17-daniel.lezcano@linaro.org> <CAJZ5v0jfZ2_LyPC9ArnSfHpYgr379HBhk5ODyESoewL_ee1wYw@mail.gmail.com>
 <66a6fdeb-d8e8-0041-769e-84abba7367c8@linaro.org>
In-Reply-To: <66a6fdeb-d8e8-0041-769e-84abba7367c8@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 22 Feb 2023 21:07:58 +0100
Message-ID: <CAJZ5v0g9Me0D3=ev_qQyRu6P7mH=-8OGPtk=oAkZpEM6PDkNpg@mail.gmail.com>
Subject: Re: [PATCH v2 16/16] thermal/traces: Replace the thermal zone
 structure parameter with the field value
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Lukasz Luba <lukasz.luba@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "open list:TRACING" <linux-trace-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 22, 2023 at 9:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 22/02/2023 20:51, Rafael J. Wysocki wrote:
> > On Tue, Feb 21, 2023 at 7:08 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> In the work of the thermal zone device self-encapsulation, let's pass
> >> the field value instead of dereferencing them in the traces which
> >> force us to export publicly the thermal_zone_device structure.
> >>
> >> No fonctionnal change intended.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>   drivers/thermal/gov_fair_share.c              |  2 +-
> >>   drivers/thermal/gov_power_allocator.c         |  4 ++--
> >>   drivers/thermal/gov_step_wise.c               |  2 +-
> >>   drivers/thermal/thermal_core.c                |  5 ++--
> >>   include/trace/events/thermal.h                | 24 +++++++++----------
> >>   .../trace/events/thermal_power_allocator.h    | 12 +++++-----
> >>   6 files changed, 25 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
> >> index aad7d5fe3a14..e6c21abaaa80 100644
> >> --- a/drivers/thermal/gov_fair_share.c
> >> +++ b/drivers/thermal/gov_fair_share.c
> >> @@ -35,7 +35,7 @@ static int get_trip_level(struct thermal_zone_device *tz)
> >>           * point, in which case, trip_point = count - 1
> >>           */
> >>          if (count > 0)
> >> -               trace_thermal_zone_trip(tz, count - 1, trip.type);
> >> +               trace_thermal_zone_trip(tz->type, tz->id, count - 1, trip.type);
> >
> > Haven't you introduced an accessor for tz->id in this series?  Why not
> > use it here?
> >
> > And there can be an analogous accessor for tz->type.
> >
> > If there are accessors like that, they should be used consistently
> > everywhere as applicable IMO.
>
> governors are part of the thermal core code, so they are authorized to
> access the thermal structure internals, that is why they are not passing
> through the accessors.

I'm not talking about "authorization", but about consistency.

If accessors are used consistently, it is sufficient to grep for an
accessor to find all places where the given field is accessed, for
example.