Received-SPF: pass (google.com: domain of linux-kernel+bounces-150839-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <4149777d-d8ab-43aa-8de2-f240fee2ba2b@nvidia.com>
Date: Thu, 18 Apr 2024 15:33:19 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Documentation: coding-style: don't encourage WARN*()
To: Eric Biggers <ebiggers@kernel.org>, Alex Elder <elder@linaro.org>
CC: <corbet@lwn.net>, <gregkh@linuxfoundation.org>,
	<workflows@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240414170850.148122-1-elder@linaro.org>
 <20240418161430.GB2410@sol.localdomain>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20240418161430.GB2410@sol.localdomain>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 22:33:46.0187
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b325d0fe-5246-45a3-4e41-08dc5ff79c29
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	CY4PEPF0000FCC3.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB8888

On 4/18/24 9:14 AM, Eric Biggers wrote:
> On Sun, Apr 14, 2024 at 12:08:50PM -0500, Alex Elder wrote:
>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
>> index 9c7cf73473943..bce43b01721cb 100644
>> --- a/Documentation/process/coding-style.rst
>> +++ b/Documentation/process/coding-style.rst
>> @@ -1235,17 +1235,18 @@ example. Again: WARN*() must not be used for a condition that is expected
>>   to trigger easily, for example, by user space actions. pr_warn_once() is a
>>   possible alternative, if you need to notify the user of a problem.
>>   
>> -Do not worry about panic_on_warn users

This is still[1] good advice. panic_on_warn users have made a
trade-off that works for them, but that should not mean that
all of the valid cases for WARN*() suddenly disappear. In fact,
without the WARN*() calls, panic_on_warn is a no-op, as someone
else has already pointed out.


>> -**************************************
>> +The panic_on_warn kernel option
>> +********************************
>>   
>> -A few more words about panic_on_warn: Remember that ``panic_on_warn`` is an
>> -available kernel option, and that many users set this option. This is why
>> -there is a "Do not WARN lightly" writeup, above. However, the existence of
>> -panic_on_warn users is not a valid reason to avoid the judicious use
>> -WARN*(). That is because, whoever enables panic_on_warn has explicitly
>> -asked the kernel to crash if a WARN*() fires, and such users must be
>> -prepared to deal with the consequences of a system that is somewhat more
>> -likely to crash.
>> +Note that ``panic_on_warn`` is an available kernel option. If it is enabled,
>> +a WARN*() call whose condition holds leads to a kernel panic.  Many users
>> +(including Android and many cloud providers) set this option, and this is
>> +why there is a "Do not WARN lightly" writeup, above.
>> +
>> +The existence of this option is not a valid reason to avoid the judicious
>> +use of warnings. There are other options: ``dev_warn*()`` and ``pr_warn*()``
>> +issue warnings but do **not** cause the kernel to crash. Use these if you
>> +want to prevent such panics.
>>   
> 
> Nacked-by: Eric Biggers <ebiggers@google.com>

Yes. I agree with this NAK.

> 
> WARN*() are for recoverable assertions, i.e. situations where the condition
> being true can only happen due to a kernel bug but where they can be recovered
> from (unlike BUG*() which are for unrecoverable situations).  The people who use
> panic_on_warn *want* the kernel to crash when such a situation happens so that
> the underlying issue can be discovered and fixed.  That's the whole point.
> 
> Also, it's not true that "Android" sets this option.  It might be the case that
> some individual Android OEMs have decided to use it for some reason (they do
> have the ability to customize their kernel command line, after all).  It's
> certainly not used by default or even recommended.
> 
> - Eric
> 

[1] https://lore.kernel.org/lkml/0db131cf-013e-6f0e-c90b-5c1e840d869c@nvidia.com/T/#md6836102150ac1e6265569aad55a692e3af75f34

thanks,
-- 
John Hubbard
NVIDIA