Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] thermal: Fix a NULL pointer dereference
To:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Amit Kucheria <amitk@kernel.org>
CC:     <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        David Collins <quic_collinsd@quicinc.com>,
        Manaf Meethalavalappu Pallikunhi <manafm@codeaurora.org>,
        Ram Chandrasekar <rkumbako@codeaurora.org>,
        <stable@vger.kernel.org>
References: <1631041289-11804-1-git-send-email-quic_subbaram@quicinc.com>
 <003252f2-510f-e9ea-0032-6034f26aad11@linaro.org>
 <16af9946-b662-0bbf-206f-278b7ef98123@quicinc.com>
 <8cda69a6-907b-e09e-ba64-011b0216a4df@quicinc.com>
 <dbf9a03f-6c66-dcfe-6158-d93532272a5c@linaro.org>
From:   Subbaraman Narayanamurthy <quic_subbaram@quicinc.com>
Message-ID: <1e0371e7-5c15-a552-870a-a501ec11a8f9@quicinc.com>
Date:   Thu, 21 Oct 2021 11:29:20 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <dbf9a03f-6c66-dcfe-6158-d93532272a5c@linaro.org>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 10/19/21 12:35 AM, Daniel Lezcano wrote:
> On 19/10/2021 03:21, Subbaraman Narayanamurthy wrote:
>> On 10/8/21 12:50 PM, Subbaraman Narayanamurthy wrote:
>>> On 10/6/21 4:08 AM, Daniel Lezcano wrote:
> [ ... ]
>
>>> /sys/devices/virtual/thermal/thermal_zone87 # echo 120000 > trip_point_0_temp  
>>> [  184.290964][  T211] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
>>> [  184.300896][  T211] Mem abort info:                                         
>>> [  184.304486][  T211]   ESR = 0x96000006                                      
>>> [  184.308348][  T211]   EC = 0x25: DABT (current EL), IL = 32 bits            
>>> [  184.314531][  T211]   SET = 0, FnV = 0                                      
>>> [  184.318384][  T211]   EA = 0, S1PTW = 0                                     
>>> [  184.322323][  T211] Data abort info:                                        
>>> [  184.325993][  T211]   ISV = 0, ISS = 0x00000006                             
>>> [  184.330655][  T211]   CM = 0, WnR = 0                                       
>>> [  184.334425][  T211] user pgtable: 4k pages, 39-bit VAs, pgdp=000000081a7a2000
>>> [  184.341750][  T211] [0000000000000020] pgd=000000081a7a7003, p4d=000000081a7a7003, pud=000000081a7a7003, pmd=0000000000000000
>>> [  184.353359][  T211] Internal error: Oops: 96000006 [#1] PREEMPT SMP         
>>> [  184.359797][  T211] Dumping ftrace buffer:                                  
>>> [  184.364001][  T211]    (ftrace buffer empty)
>>>
>>> Hope this helps.
>> Hi Daniel,
>> Have you got a chance to look at this?
> Hi Subbaraman,
>
> Actually, I think the root problem is the thermal zone is showing up
> while there is no sensor associated with it. You can read the
> temperature and get a kernel warning also.
>
> That's what should be fixed IMO.
>

Hi Daniel,

If I understand your statement correctly, are you saying that a thermal_zone device should be created only after a thermal sensor driver supplying it probes?

From what I can see,

thermal_init()
    --> of_parse_thermal_zones()
            --> thermal_zone_device_register()
                    --> thermal_zone_create_device_groups()
                            <followed by>
                        device_register() with thermal_zone%d


This happens way before a thermal sensor driver probes. So, creating a thermal_zone device only after its thermal sensor probes would require more changes to the framework.

Also, I see a similar NULL check exists in the framework code already.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/thermal_of.c?h=v5.15-rc6#n103

So, extending the logic for other callsites (below) makes sense to me.

- of_thermal_get_temp()
- of_thermal_set_emul_temp()
- of_thermal_get_trend()
- of_thermal_set_trip_temp()

Thanks,
Subbaraman