Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds

Rafael J. Wysocki
2024-10-22T10:02:01Z

Hi Lukasz,

On Tue, Oct 22, 2024 at 11:39 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel,
>
> On 10/22/24 08:09, Daniel Lezcano wrote:
> > On 22/10/2024 00:02, Lukasz Luba wrote:
> >>
> >>
> >> On 10/14/24 10:43, Daniel Lezcano wrote:
> >>> The thresholds exist but there is no notification neither action code
> >>> related to them yet.
> >>>
> >>> These changes implement the netlink for the notifications when the
> >>> thresholds are crossed, added, deleted or flushed as well as the
> >>> commands which allows to get the list of the thresholds, flush them,
> >>> add and delete.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> ---
> >
> > [ ... ]
> >
> >>> +static inline int thermal_notify_threshold_up(const struct
> >>> thermal_zone_device *tz)
> >>> +{
> >>> + return 0;
> >>> +}
> >>
> >> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
> >> not defined. Although, since we don't check the output of those
> >> functions - we are OK. We just have to remember about these zeros,
> >> one day when we would like to add the check of the return.
> >
> > The return error really depends on the context of the call site. There
> > are other subsystems returning 0 when the service is not enabled (eg.
> > cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
> >
> >
>
> Fair enough. As I said, we would just keep them in mind if we one
> day decide to add the checks of the returns.
>
> I'm waiting for your next version with the new locking scheme that
> Rafael asked and I will add my review tags.

My plan was to take this patch and replace the open-coded locking with
guards when applying it (that would be a mechanical change AFAICS).

I guess it's OK to add a R-by from you in that case?