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: [EXT] [PATCH 09/15] qed: use new module_firmware_crashed()
To:     Luis Chamberlain <mcgrof@kernel.org>
CC:     <jeyu@kernel.org>, <akpm@linux-foundation.org>, <arnd@arndb.de>,
        <rostedt@goodmis.org>, <mingo@redhat.com>, <aquini@redhat.com>,
        <cai@lca.pw>, <dyoung@redhat.com>, <bhe@redhat.com>,
        <peterz@infradead.org>, <tglx@linutronix.de>,
        <gpiccoli@canonical.com>, <pmladek@suse.com>, <tiwai@suse.de>,
        <schlad@suse.de>, <andriy.shevchenko@linux.intel.com>,
        <keescook@chromium.org>, <daniel.vetter@ffwll.ch>,
        <will@kernel.org>, <mchehab+samsung@kernel.org>,
        <kvalo@codeaurora.org>, <davem@davemloft.net>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Ariel Elior <aelior@marvell.com>,
        GR-everest-linux-l2 <GR-everest-linux-l2@marvell.com>
References: <20200509043552.8745-1-mcgrof@kernel.org>
 <20200509043552.8745-10-mcgrof@kernel.org>
 <2aaddb69-2292-ff3f-94c7-0ab9dbc8e53c@marvell.com>
 <20200509164229.GJ11244@42.do-not-panic.com>
From:   Igor Russkikh <irusskikh@marvell.com>
Message-ID: <e10b611e-f925-f12d-bcd2-ba60d86dd8d0@marvell.com>
Date:   Tue, 12 May 2020 19:23:28 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:77.0) Gecko/20100101
 Thunderbird/77.0
MIME-Version: 1.0
In-Reply-To: <20200509164229.GJ11244@42.do-not-panic.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


>> So I think its not a good place to insert this call.
>> Its hard to find exact good place to insert it in qed.
> 
> Is there a way to check if what happened was indeed a fw crash?

Our driver has two firmwares (slowpath and fastpath).
For slowpath firmware the way to understand it crashed is to observe command
response timeout. This is in qed_mcp.c, around "The MFW failed to respond to
command" traceout.

For fastpath this is tricky, think you may leave the above place as the only
place to invoke module_firmware_crashed()

> 
>> One more thing is that AFAIU taint flag gets permanent on kernel, but
> for
>> example our device can recover itself from some FW crashes, thus it'd be
>> transparent for user.
> 
> Similar things are *supposed* to recoverable with other device, however
> this can also sometimes lead to a situation where devices are not usable
> anymore, and require a full driver unload / load.
> 
>> Whats the logical purpose of module_firmware_crashed? Does it mean fatal
>> unrecoverable error on device?
> 
> Its just to annotate on the module and kernel that this has happened.
> 
> I take it you may agree that, firmware crashing *often* is not good
> design,
> and these issues should be reported to / fixed by vendors. In cases
> where driver bugs are reported it is good to see if a firmware crash has
> happened before, so that during analysis this is ruled out.

Probably, but still I see some misalignment here, in sense that taint is about
the kernel state, not about a hardware state indication.

devlink health could really be a much better candidate for such things.

Regards
  Igor