Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210309060121.9099-1-pkshih@realtek.com>
In-Reply-To: <20210309060121.9099-1-pkshih@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Mon, 3 May 2021 15:55:36 -0700
Message-ID: <CA+ASDXO1Eh0D3k4ssCqgymgbonNf0RVNy=AxpnzLO6+Ch5=hWQ@mail.gmail.com>
Subject: Re: [PATCH] rtw88: 8822c: support FW crash dump when FW crash
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     Yan-Hsuan Chuang <tony0620emma@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        kevin_yang@realtek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Mar 8, 2021 at 10:02 PM Ping-Ke Shih <pkshih@realtek.com> wrote:
> Although FW crash logs are already supported for dumping in driver, the
> logs may not be sufficient to analyze field issues. To improve this part,
> we add a support to dump FW memory.
>
> When driver receives FW crash notifications, driver uses DDMA, which is a
> HW ability, to copy specified FW memory to FW fifo. Driver can then dump
> these information from FW fifo. With this support, not only FW crash log
> but also specified FW memory will be dumped while FW crash. Besides,
> specified registers are also dumped.
>
> This feature is implemeted on 8822C first due to difference of FW layouts
> between ICs. In addition, we add a debugfs to trigger FW crash. It can
> simulate the process of crash, dump and reset. Through it, we can also
> check if a reset is ongoing.
>
> Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
...
> +int rtw_dump_fw(struct rtw_dev *rtwdev, const u32 ocp_src, u32 size,
> +               const char *prefix_str)
> +{
...
> +       print_hex_dump(KERN_ERR, prefix_str, DUMP_PREFIX_OFFSET, 16, 1,
> +                      buf, done_size, true);
> +
> +exit:
> +       vfree(buf);
> +       return ret;
> +}
> +EXPORT_SYMBOL(rtw_dump_fw);
> +
> +int rtw_dump_reg(struct rtw_dev *rtwdev, const u32 addr, const u32 size,
> +                const char *prefix_str)
> +{
....
> +       print_hex_dump(KERN_ERR, prefix_str, DUMP_PREFIX_OFFSET, 16, 4, buf,
> +                      size, true);
> +
> +       vfree(buf);
> +       return 0;
> +}


For the record, all this print_hex_dump() stuff should just be using
devcoredump instead. There are numerous problems with dumping this
much unreadable (and opaque to anyone but Realtek) garbage directly
into the kernel logs.

My bad for not reviewing this earlier. With luck, Realtek will add
devcoredump themselves, although I suppose it wouldn't be too hard for
a drive-by contributor (e.g., me) to fix, given the time...

Brian