greearb@xxxxxxxxxxxxxxx writes:
> From: Ben Greear <greearb@xxxxxxxxxxxxxxx>
>
> Store the firmware crash registers and last 128 or so
> firmware debug-log ids and present them to user-space
> via debugfs.
>
> Should help with figuring out why the firmware crashed.
>
> Signed-off-by: Ben Greear <greearb@xxxxxxxxxxxxxxx>
Review part 2.
> @@ -875,6 +877,80 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
> reg_dump_values[i + 2],
> reg_dump_values[i + 3]);
>
> + /* Dump the debug logs on the target */
> + host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
> + if (ath10k_pci_diag_read_mem(ar, host_addr,
> + ®_dump_area, sizeof(u32)) != 0) {
> + ath10k_warn("failed to read hi_dbglog_hdr\n");
Please print the error code.
> + goto save_regs_and_restart;
> + }
> +
> + ath10k_err("target register Debug Log Location: 0x%08X\n",
> + reg_dump_area);
Is this needed? I think this more like debug log material than something
we should report to the user.
> + ret = ath10k_pci_diag_read_mem(ar, reg_dump_area,
> + &dbg_hdr, sizeof(dbg_hdr));
> + if (ret != 0) {
> + ath10k_err("failed to dump Debug Log Area\n");
ath10k_warn("failed to dump debug log area: %d\n", ret);
> + goto save_regs_and_restart;
> + }
> +
> + ath10k_err("Debug Log Header, dbuf: 0x%x dropped: %i\n",
> + dbg_hdr.dbuf, dbg_hdr.dropped);
ath10k_dbg()?
> + dbufp = dbg_hdr.dbuf;
> + i = 0;
> + while (dbufp) {
for (i = 0; dbufp > 0; i++)?
> + struct ath10k_fw_dbglog_buf dbuf;
Please move to the beginning of the function.
> + ret = ath10k_pci_diag_read_mem(ar, dbufp,
> + &dbuf, sizeof(dbuf));
> + if (ret != 0) {
> + ath10k_err("failed to read Debug Log Area: 0x%x\n",
> + dbufp);
ath10k_warn("failed to read debug log area: %d\n");
> + goto save_regs_and_restart;
> + }
> +
> + /* We have a buffer of data */
> + ath10k_err("[%i] next: 0x%x buf: 0x%x sz: %i len: %i count: %i free: %i\n",
> + i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
> + dbuf.count, dbuf.free);
ath10k_dbg()?
> + if (dbuf.buffer && dbuf.length) {
I would prefer inverse here to keep the indentation clean, something
like:
if (dbuf.buffer == 0 || dbug.length == 0)
goto next;
> + u8 *buffer = kmalloc(dbuf.length, GFP_ATOMIC);
Should we have a maximum for the length so that we don't allocate insane
amounts of memory by accident?
> + if (buffer) {
Again I would prefer the inverse:
if (!buf)
break;
I assume there's no sense continuing here if kmalloc fails and better
just bail out.
> + ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer,
> + buffer,
> + dbuf.length);
> + if (ret != 0) {
> + ath10k_err("failed to read Debug Log buffer: 0x%x\n",
> + dbuf.buffer);
ath10k_warn("failed to read debug log buffer: %d\n", ret);
> + kfree(buffer);
> + goto save_regs_and_restart;
> + }
> +
> + ath10k_dbg_save_fw_dbg_buffer(ar, buffer,
> + dbuf.length);
> + kfree(buffer);
Instead of doing atomic allocations multiple times in a loop, would it
be better to allocate just one buffer before the loop and free it
afterwards?
> + }
> + }
> + dbufp = dbuf.next;
> + if (dbufp == dbg_hdr.dbuf) {
> + /* It is a circular buffer it seems, bail if next
> + * is head
> + */
> + break;
> + }
> + i++;
> + } /* While we have a debug buffer to read */
> +
> +save_regs_and_restart:
exit_save_regs:
> + if (!ar->crashed_since_read) {
> + ar->crashed_since_read = true;
> + memcpy(ar->reg_dump_values, reg_dump_values,
> + sizeof(ar->reg_dump_values));
> + }
How is the locking handled in this function?
> +do_restart:
exit:
> queue_work(ar->workqueue, &ar->restart_work);
> }
--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
