Re: [PATCH v2] thermal: int340x: Protect trip temperature from dynamic update

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Tue, 2023-01-24 at 21:32 +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 23, 2023 at 6:26 PM Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > Trip temperatures are read using ACPI methods and stored in the
> > memory
> > during zone initializtion and when the firmware sends a
> > notification for
> > change. This trip temperature is returned when the thermal core
> > calls via
> > callback get_trip_temp().
> > 
> > But it is possible that while updating the memory copy of the trips
> > when
> > the firmware sends a notification for change, thermal core is
> > reading the
> > trip temperature via the callback get_trip_temp(). This may return
> > invalid
> > trip temperature.
> > 
> > To address this add a mutex to protect the invalid temperature
> > reads in
> > the callback get_trip_temp() and int340x_thermal_read_trips().
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > Cc: stable@vger.kernel.org # 5.0+
> > ---
> > v2:
> > - rebased on linux-next
> 
> So I've rebased it back onto 6.2-rc5 and pushed the result into the
> thermal-intel-fixes branch.  Please see if it looks good to you and
> let me know.
Looks good.

Thanks,
Srinivas

> 
> I'd prefer to push it for 6.2-rc.
> 
> > - Add ret variable and remove return as suugested by Rafael
> 
> Thanks!
> 
> >  .../int340x_thermal/int340x_thermal_zone.c     | 18
> > +++++++++++++++---
> >  .../int340x_thermal/int340x_thermal_zone.h     |  1 +
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > index 228f44260b27..5fda1e67b793 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > @@ -41,7 +41,9 @@ static int int340x_thermal_get_trip_temp(struct
> > thermal_zone_device *zone,
> >                                          int trip, int *temp)
> >  {
> >         struct int34x_thermal_zone *d = zone->devdata;
> > -       int i;
> > +       int i, ret = 0;
> > +
> > +       mutex_lock(&d->trip_mutex);
> > 
> >         if (trip < d->aux_trip_nr)
> >                 *temp = d->aux_trips[trip];
> > @@ -60,10 +62,12 @@ static int int340x_thermal_get_trip_temp(struct
> > thermal_zone_device *zone,
> >                         }
> >                 }
> >                 if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
> > -                       return -EINVAL;
> > +                       ret = -EINVAL;
> >         }
> > 
> > -       return 0;
> > +       mutex_unlock(&d->trip_mutex);
> > +
> > +       return ret;
> >  }
> > 
> >  static int int340x_thermal_get_trip_type(struct
> > thermal_zone_device *zone,
> > @@ -165,6 +169,8 @@ int int340x_thermal_read_trips(struct
> > int34x_thermal_zone *int34x_zone)
> >         int trip_cnt = int34x_zone->aux_trip_nr;
> >         int i;
> > 
> > +       mutex_lock(&int34x_zone->trip_mutex);
> > +
> >         int34x_zone->crt_trip_id = -1;
> >         if (!int340x_thermal_get_trip_config(int34x_zone->adev-
> > >handle, "_CRT",
> >                                              &int34x_zone-
> > >crt_temp))
> > @@ -192,6 +198,8 @@ int int340x_thermal_read_trips(struct
> > int34x_thermal_zone *int34x_zone)
> >                 int34x_zone->act_trips[i].valid = true;
> >         }
> > 
> > +       mutex_unlock(&int34x_zone->trip_mutex);
> > +
> >         return trip_cnt;
> >  }
> >  EXPORT_SYMBOL_GPL(int340x_thermal_read_trips);
> > @@ -215,6 +223,8 @@ struct int34x_thermal_zone
> > *int340x_thermal_zone_add(struct acpi_device *adev,
> >         if (!int34x_thermal_zone)
> >                 return ERR_PTR(-ENOMEM);
> > 
> > +       mutex_init(&int34x_thermal_zone->trip_mutex);
> > +
> >         int34x_thermal_zone->adev = adev;
> > 
> >         int34x_thermal_zone->ops =
> > kmemdup(&int340x_thermal_zone_ops,
> > @@ -277,6 +287,7 @@ struct int34x_thermal_zone
> > *int340x_thermal_zone_add(struct acpi_device *adev,
> >  err_trip_alloc:
> >         kfree(int34x_thermal_zone->ops);
> >  err_ops_alloc:
> > +       mutex_destroy(&int34x_thermal_zone->trip_mutex);
> >         kfree(int34x_thermal_zone);
> >         return ERR_PTR(ret);
> >  }
> > @@ -289,6 +300,7 @@ void int340x_thermal_zone_remove(struct
> > int34x_thermal_zone
> >         acpi_lpat_free_conversion_table(int34x_thermal_zone-
> > >lpat_table);
> >         kfree(int34x_thermal_zone->aux_trips);
> >         kfree(int34x_thermal_zone->ops);
> > +       mutex_destroy(&int34x_thermal_zone->trip_mutex);
> >         kfree(int34x_thermal_zone);
> >  }
> >  EXPORT_SYMBOL_GPL(int340x_thermal_zone_remove);
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > index e28ab1ba5e06..6610a9cc441b 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > @@ -32,6 +32,7 @@ struct int34x_thermal_zone {
> >         struct thermal_zone_device_ops *ops;
> >         void *priv_data;
> >         struct acpi_lpat_conversion_table *lpat_table;
> > +       struct mutex trip_mutex;
> >  };
> > 
> >  struct int34x_thermal_zone *int340x_thermal_zone_add(struct
> > acpi_device *,
> > --
> > 2.31.1
> >