* Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn
[not found] ` <1b33f325-c104-8b0c-099f-f2d2e98fed66@amd.com>
2023-06-28 21:06 ` [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn Jakub Kicinski
@ 2023-06-29 17:25 ` Jacob Keller
1 sibling, 0 replies; 2+ messages in thread
From: Jacob Keller @ 2023-06-29 17:25 UTC (permalink / raw)
To: Shannon Nelson, netdev@vger.kernel.org, davem@davemloft.net,
kuba@kernel.org
Cc: brett.creeley@amd.com, drivers@pensando.io, nitya.sunkad@amd.com
On 6/28/2023 11:26 AM, Shannon Nelson wrote:
> On 6/28/23 10:57 AM, Keller, Jacob E wrote:
>>
>>> -----Original Message-----
>>> From: Shannon Nelson <shannon.nelson@amd.com>
>>> Sent: Wednesday, June 28, 2023 10:01 AM
>>> To: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org
>>> Cc: brett.creeley@amd.com; drivers@pensando.io; nitya.sunkad@amd.com;
>>> Shannon Nelson <shannon.nelson@amd.com>
>>> Subject: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn
>>>
>>> From: Nitya Sunkad <nitya.sunkad@amd.com>
>>>
>>> Remove instances of WARN_ON to prevent problematic panic_on_warn use
>>> resulting in kernel panics.
>>>
>>
>> This message could potentially use a bit more explanation since it doesn't look like you removed all the WARN_ONs in the driver, and it might help to explain why this particular WARN_ON was problematic. I don't think that would be worth a re-roll on its own though.
>
> There has been recent mention of not using WARNxxx macros because so
> many folks have been setting panic_on_warn [1]. This is intended to
> help mitigate the possibility of unnecessarily killing a machine when we
> can adjust and continue.
> [1]: https://lore.kernel.org/netdev/2023060820-atom-doorstep-9442@gregkh/
>
> I believe the only other WARNxxx in this driver is a WARN_ON_ONCE in
> ionic_regs.h which can be addressed in a separate patch.
>
> Neither of these are ever expected to be hit, but also neither should
> ever kill a machine.
>
Ok. Makes sense enough to me.
>>> - if (WARN_ON(n_qcq->flags & IONIC_QCQ_F_INTR)) {
>>> + if (n_qcq->flags & IONIC_QCQ_F_INTR) {
>>> + dev_warn(n_qcq->q.dev, "%s: n_qcq->flags and
>>> IONIC_QCQ_F_INTR set\n",
>>> + __func__);
>>
>> What calls this function? It feels a bit weird that the only action this code takes was in a WARN_ON state. Definitely agree this shouldn't be WARN_ON.
>
> This isn't the only action in this function - after this 'if' is a bit
> of code to link the queues onto the same interrupt.
>
Yea, I think I missed some context, and saw a '}' that I thought was the
end of the function.
^ permalink raw reply [flat|nested] 2+ messages in thread