Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn

Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn

[Jakub Kicinski]

On Wed, 28 Jun 2023 11:26:18 -0700 Shannon Nelson wrote:
> > 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.

An explanation that this warning may in fact be hit and how in 
the commit message would be good.

Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn

[Jacob Keller]

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.