Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20230916113327.85693-1-bo.ye@mediatek.com> <542a64a842d715329b35654db908681e074b0db5.camel@mediatek.com>
 <148c8e2d10bbb932177f732dfc44230681dc4d91.camel@mediatek.com>
In-Reply-To: <148c8e2d10bbb932177f732dfc44230681dc4d91.camel@mediatek.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 25 Oct 2023 20:21:20 +0200
Message-ID: <CAJZ5v0hSeCcpOr4qOpji8sbN89GnsYYc1pYBhV+LmS=TiJQMnA@mail.gmail.com>
Subject: Re: [PATCH] thermal: Fix potential race condition in suspend/resume
To:     =?UTF-8?B?Qm8gWWUgKOWPtuazoik=?= <Bo.Ye@mediatek.com>
Cc:     "rui.zhang@intel.com" <rui.zhang@intel.com>,
        "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
        "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
        "angelogioacchino.delregno@collabora.com" 
        <angelogioacchino.delregno@collabora.com>,
        "amitk@kernel.org" <amitk@kernel.org>,
        "rafael@kernel.org" <rafael@kernel.org>,
        =?UTF-8?B?WW9uZ2RvbmcgWmhhbmcgKOW8oOawuOS4nCk=?= 
        <Yongdong.Zhang@mediatek.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        =?UTF-8?B?QnJvd3NlIFpoYW5nICjlvKDno4op?= 
        <Browse.Zhang@mediatek.com>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        =?UTF-8?B?WXVnYW5nIFdhbmcgKOeOi+eOieWImik=?= 
        <Yugang.Wang@mediatek.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Oct 23, 2023 at 3:20 AM Bo Ye (叶波) <Bo.Ye@mediatek.com> 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@mediatek.com>
> > >
> > > 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@mediatek.com>
> > > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> > > ---
> > >  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;