Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0891fecd-88e9-cd50-b2ac-8b50c7149b19@intel.com>
Date:   Thu, 30 Mar 2023 21:08:16 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [BUG v6.3-rc4+] WARNING: CPU: 0 PID: 1 at
 drivers/thermal/thermal_sysfs.c:879 cooling_device_stats_setup+0xac/0xc0
To:     "Zhang, Rui" <rui.zhang@intel.com>,
        "Torvalds, Linus" <torvalds@linux-foundation.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230329165819.50ce6ab1@gandalf.local.home>
 <CAHk-=wiEdcjdceNdfVGPEcbSmAKh_rjtBSy5_Z3Yyx2GFEgLFA@mail.gmail.com>
 <22cab1d16c75cf07e0846a136c355615bc9b05dd.camel@intel.com>
Content-Language: en-US
From:   "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>
In-Reply-To: <22cab1d16c75cf07e0846a136c355615bc9b05dd.camel@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SVE4WStRSjZqTjBIbngwQVd5VmdSVXVoWlo2REoyQjAzNnMvVTRmd3NpUS9o?=
 =?utf-8?B?TFNvalJ5aHp6bENabjd3RXZJamdCaE5wVExPZFlMR2tBREF4MnRQRk4yRUdD?=
 =?utf-8?B?Z0U5Sy8xSDBIMFRhODFqQVg1RVlyK1RRSG9yTUhPSU8vMDVQWDJ6VVByczd3?=
 =?utf-8?B?OHZkTDR1emMxamsvTC95WEpZbW5ONkxJQW1FMUtRZndCcXFnT3BYVWVEWDJR?=
 =?utf-8?B?d1NEUVZUZjhrcjNjTnlXeHR2dktTNHdqNXd6WmR5bFRHVmhQNjNPS0lCWXpL?=
 =?utf-8?B?cUlNR1dIK3puS1pqSjlKQ2V4ZFJyUXVIY3FJcWg0ZDRmRGVpbE9JMFhYeGZU?=
 =?utf-8?B?anVEdVhHYnpNRHZIVXF2Tk4vTDBIRWg3ellTdVJxMHJTOEx6WlBCbFBkaHBN?=
 =?utf-8?B?bWkyM2NiTGJJeEhOaGI4cjMvbnpucTdVdzFSUnJpTXZ4aDQ2OGdqdEEwT2tm?=
 =?utf-8?B?NDk1bW5CZ1RVQkhUOEdyOUptRDV6STlLbEswMk00OGc2V2VBUlRBMVRleHN3?=
 =?utf-8?B?YVNENEZjaW9yVzF0R2d3eGYwMjU2QTNSUGZiTHM5YjZWcDRpOTVEWWpIMTh5?=
 =?utf-8?B?a2FDYTdISCt1cGFiSyt1THlJd1RjMXFSUnN3eWtGV2FzdTVsakRDYkRabTJQ?=
 =?utf-8?B?RUV4V1BXeXBSZDVsTysydHZpTVFRS2ozTVRQemxXNVF6aHV5QzNwMnlhdXdk?=
 =?utf-8?B?dGZFWjNGOXIrTzJlRSsyeGtNUklZSmVkTERiZnpoSUxYbUFmSjBrZEJkR205?=
 =?utf-8?B?aENqTU5nOEEvQ1VabGJGOVRIR3k3TC83ODY1TGxUMnU1Z1ZueE8xV0daUm40?=
 =?utf-8?B?bllwNFFFMHdJeDdicXRvOVN3dVhFdG93OUNBc0k2ZmhwSzFDYnR3dDZpbzNq?=
 =?utf-8?B?Nzh2ay9GeUtONnZaOWwvc2dJaU1RcGRHNG5FWnBpbno5TjI0WlNUVnB3Qk84?=
 =?utf-8?B?OEFOK3AreEJMdjltOUVmNkozRDQzK1BnTTBMTnE0VzluY2NMdzdtcE91WWJX?=
 =?utf-8?B?bVhTNkhSQzVtY2NvWjZpODhNWDB1UDE1WEI0RzE2cXBRZit0SmZqaTRXOFEx?=
 =?utf-8?B?eUxmQTZsN2xHRmVGU2xWUCtzTW0vU3paWkhpUTRIU0RseGVFekdwZ0hsaEI2?=
 =?utf-8?B?NUtKU0JiWEdHK0I0WGF3eE03SE9jL09ZWElpQ1YxcXoxaCtEOHhsdXRzTWJV?=
 =?utf-8?B?WllzQlhVaDdkdTNHbFNSOXgvcnJyRXNxeFMzNGZhSVJDU2UwTWcxYVhKTmx0?=
 =?utf-8?B?VVNYaENiWVZ6WTk5aWNuVEpuNmZ4czBmK1ZkS1FjV216QndObVJqWDYyMEgw?=
 =?utf-8?B?dnRxUk5oS1VuVTNrZCtGTTFwWFo0QjdXM3dBVk1adk9BNmhyT1l6OWpkYmtG?=
 =?utf-8?B?Vy91RG9NVmY2VXIzMGJkTjcvd2plcSszK1FFTTRWWlhHS0VBOVg1V0VUd0VR?=
 =?utf-8?B?dGs4UjF0TU1pOHZRQytaVTR1a2MzUlR6WFdZRlAxQkRYbElaRmJiTVBMVVJm?=
 =?utf-8?B?aTRiQlFPcGlFTHJKdVR1bXkrWk1PZUUvQWloTTB0dXZ4SkRNZWJyT3BDaDlu?=
 =?utf-8?B?aW1UVjNZNTNDUlYzT2Y1QSs4cHhldWxNQlNYVUVOalVBL2RDSkNEcG5aZEZI?=
 =?utf-8?B?TnBxYjl1clg4L1FvWmF5QmxQSGR3SVppaStmVzMyQVdlQ0NGZFpoUHRSRkZQ?=
 =?utf-8?B?MXdYSlp2YzFUcTdReit4RjhlcnJ5c3RieDhYNlY5ek1iU0luc2liSUtkbVE1?=
 =?utf-8?B?UFR0eGRBdDhWR1Q3YzdiUWJwSVZsa2RJM2w5NWQwMmZ6ektMZ2VoTHRTS0hw?=
 =?utf-8?B?bTd0d2gxUGJ3MXYwT3poaDNoL2p6T1JHVWQwWlpOS3l2YjdBRm81UUJ4R0h6?=
 =?utf-8?B?WkhPTWtDbHEzNkFuR2VuQzdIdWhLUXloTDFPdXZsbnk3TFYwZEQ2dkpMbThR?=
 =?utf-8?B?VHQ2Y3VjVDFsTWhpa2ZiTzgzTFU4cEdNODFiOHF1V2J0UEVjcGVVcEc2V1ZY?=
 =?utf-8?B?bWFybXdYUTNGTVRFUi8wTGh2eGh3V1Z1ZEI3RWlqemlrRGQ2ZldRNURpK1Nu?=
 =?utf-8?B?SGtLZU1IcEswc0tocTJwbWN3VVhrTmozS1czbTdrSERDc1pxUEZIUlFuQS91?=
 =?utf-8?B?YU5Obk52RkFvT0x6Yk4rQTN1eEFPN080WlpLTm9rUkNyVSt6bjlQc2k3Vmdr?=
 =?utf-8?B?aGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ebd4d07c-93a8-4aa9-42cf-08db31522209
X-MS-Exchange-CrossTenant-AuthSource: MW5PR11MB5810.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 19:08:23.5386
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: j4SZWWkpFHa/NYYdW5UY28LycDG+ymnB8ZFCyRCqe1ZeOHS73m9SoJfIlhWrQzGH3cDIaS4HJZAm3UNIl5G2OlQJrLZe8buaCJrV/U7CnLw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4749
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/30/2023 11:57 AM, Zhang, Rui wrote:
> Hi, Linus,
>
> On Wed, 2023-03-29 at 15:52 -0700, Linus Torvalds wrote:
>> On Wed, Mar 29, 2023 at 1:58 PM Steven Rostedt <rostedt@goodmis.org>
>> wrote:
>>> In preparation to adding my patch that checks for some kinds of
>>> bugs in
>>> trace events, I decided to run it on the Linus's latest branch, to
>>> see if
>>> there's any other trace events that may cause issues. But instead I
>>> hit
>>> this unrelated bug. Looks to be triggering an added
>>> lockdep_assert() on
>>> boot up.
>> So I think that lockdep assert is likely bogus.

It is, sorry about that.

I have a fix queued up, 
https://patchwork.kernel.org/project/linux-pm/patch/2681615.mvXUDI8C0e@kreacher


>> It was added in commit 790930f44289 ("thermal: core: Introduce
>> thermal_cooling_device_update()") but the reason I say it's bogus is
>> that I don't think it has ever been tested:
>>
>>> static void cooling_device_stats_setup(struct
>>> thermal_cooling_device *cdev)
>>> {
>>>          lockdep_assert_held(&cdev->lock); <<<---- line 879
>> Yeah, so cooling_device_stats_setup() is called from two places:
>>
>>   - thermal_cooling_device_setup_sysfs()
>>
>>   - thermal_cooling_device_stats_reinit()
>>
>> and that first place is when that cdev is created, before it's
>> registered anywhere. It's not locked in that case, and yes, the
>> lockdep_assert_held() will trigger.
>>
>> As far as I can tell it will always trigger, and this
>> lockdep_assert()
>> has thus never been tested with lockdep enabled.
>>
>> The "stats_reinit" case seems to also be called from only one place
>> (thermal_cooling_device_update()), and that path does indeed hold the
>> cdev->lock.
>>
>> That lockdep could be made happy by having
>> thermal_cooling_device_setup_sysfs() create that device with the cdev
>> lock held. I guess that's easy enough, although somewhat annoyingly
>> there is no "mutex_init_locked()", you have to actually do
>> "mutex_init()" followed by a "mutex_lock()". And obviously unlock it
>> after doing the setup_sysfs().
>>
>> But I question whether the lockdep test should be done at all. I find
>> it distasteful that it was added with absolutely zero testing.
>>
>>
> I just realized why I cannot reproduce this problem on my testbox.
>
> In order to test the original patch with ACPI passive cooling enabled,
> I rebuild the kernel with customized DSDT.
> This taints the kernel, and clears the debug_locks, thus I didn't get
> any lockdep warnings...

So I was happy when I got a Tested-by from Rui on this and I didn't 
double check.  Turns out this was a mistake.

Cheers,

Rafael