Re: [PATCH] thermal: Fix potential race condition in suspend/resume

[Rafael J. Wysocki]

On Mon, Oct 23, 2023 at 3:20 AM Bo Ye (叶波) <Bo.Ye@xxxxxxxxxxxx> wrote:
>
> Yes, it is observed issue.

It does happen, so it's not just "potential" and the subject of the
patch is slightly misleading.  Please adjust it.

> Firstly, it needs to be clarified that this issue occurs in a real-
> world environment. By analyzing the logs, we inferred that the issue
> occurred just as the system was entering suspend mode, and the user was
> switching the thermal policy (this action causes all thermal zones to
> unregister/register). In addition, we conducted degradation tests and
> also reproduced this issue. The specific method is to first switch the
> thermal policy through a command, and then immediately put the system
> into suspend state through another command. This method can also
> reproduce the issue.

OK, so please add this information to the patch changelog.

> On Thu, 2023-10-12 at 07:35 +0000, Bo Ye (叶波) wrote:
> > On Sat, 2023-09-16 at 19:33 +0800, Bo Ye wrote:
> >
> > Correct mail title format: remove "Subject:" from mail title.
> >
> > > From: "yugang.wang" <yugang.wang@xxxxxxxxxxxx>
> > >
> > > Body:
> > > This patch fixes a race condition during system resume. It occurs
> > > if
> > > the system is exiting a suspend state and a user is trying to
> > > register/unregister a thermal zone concurrently. The root cause is
> > > that both actions access the `thermal_tz_list`.
> > >
> > > In detail:
> > >
> > > 1. At PM_POST_SUSPEND during the resume, the system reads all
> > > thermal
> > >    zones in `thermal_tz_list`, then resets and updates their
> > >    temperatures.
> > > 2. When registering/unregistering a thermal zone, the
> > >    `thermal_tz_list` gets manipulated.
> > >
> > > These two actions might occur concurrently, causing a race
> > > condition.
> > > To solve this issue, we introduce a mutex lock to protect
> > > `thermal_tz_list` from being modified while it's being read and
> > > updated during the resume from suspend.
> > >
> > > Kernel oops excerpt related to this fix:
> > >
> > > [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0]
> > > mutex_lock+0x34/0x170
> > > [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84]
> > > thermal_pm_notify+0xd4/0x26c
> > > [... cut for brevity ...]
> > > [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> > > [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> > > [ 5201.871076] [T316822]  state_store+0x15c/0x1e8

Well, the connection between the above log snippet and the issue
addressed by the patch is rather hard to establish.  Please include
more of the oops information.

> > >
> > > Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> > > Signed-off-by: yugang.wang <yugang.wang@xxxxxxxxxxxx>
> > > Signed-off-by: Bo Ye <bo.ye@xxxxxxxxxxxx>
> > > ---
> > >  drivers/thermal/thermal_core.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c
> > > index 8717a3343512..a7a18ed57b6d 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct
> > > notifier_block *nb,
> > >     case PM_POST_HIBERNATION:
> > >     case PM_POST_RESTORE:
> > >     case PM_POST_SUSPEND:
> > > +           mutex_lock(&thermal_list_lock);
> > >             atomic_set(&in_suspend, 0);

It is not clear to me why the above statement needs to be under the lock.

> > >             list_for_each_entry(tz, &thermal_tz_list, node) {
> > >                     thermal_zone_device_init(tz);
> > >                     thermal_zone_device_update(tz,
> > >                                                THERMAL_EVENT_UNSPEC
> > > IFIED);
> > >             }
> > > +           mutex_unlock(&thermal_list_lock);
> > >             break;
> > >     default:
> > >             break;