On 19 August 2014 10:22, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
> 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>
> Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxxxx>
> ---
[...]
> +struct ath10k_dump_file_data {
> + /* dump file information */
> +
> + /* "ATH10K-FW-DUMP" */
> + char df_magic[16];
> +
> + __le32 len;
> +
> + /* file dump version */
> + __le32 version;
> +
> + /* some info we can get from ath10k struct that might help */
> +
> + u8 uuid[16];
> +
> + __le32 chip_id;
> +
> + /* 0 for now, in place for later hardware */
> + __le32 bus_type;
> +
> + __le32 target_version;
> + __le32 fw_version_major;
> + __le32 fw_version_minor;
> + __le32 fw_version_release;
> + __le32 fw_version_build;
> + __le32 phy_capability;
> + __le32 hw_min_tx_power;
> + __le32 hw_max_tx_power;
> + __le32 ht_cap_info;
> + __le32 vht_cap_info;
> + __le32 num_rf_chains;
Hmm.. some of these values don't really change once driver is loaded
so we could probably just export them as separate debugfs entries but
perhaps that's an overkill just for dumping.
> +
> + dump_data->kernel_ver_code = cpu_to_le32(LINUX_VERSION_CODE);
> + strlcpy(dump_data->kernel_ver, VERMAGIC_STRING,
> + sizeof(dump_data->kernel_ver));
> +
> + dump_data->tv_sec = cpu_to_le64(crash_data->timestamp.tv_sec);
> + dump_data->tv_nsec = cpu_to_le64(crash_data->timestamp.tv_nsec);
> +
> + /* Gather dbg-log */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = cpu_to_le32(ATH10K_FW_CRASH_DUMP_DBGLOG);
> + dump_tlv->tlv_len = cpu_to_le32(sizeof(crash_data->dbglog_entry_data));
Hmm should this really be sizeof()? Not next_idx (which effectively
defines number of bytes of the dbglog)?
> + memcpy(dump_tlv->tlv_data, &crash_data->dbglog_entry_data,
> + sizeof(crash_data->dbglog_entry_data));
> + sofar += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
> +
> + /* Gather crash-dump */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = cpu_to_le32(ATH10K_FW_CRASH_DUMP_REGDUMP);
> + dump_tlv->tlv_len = cpu_to_le32(sizeof(crash_data->reg_dump_values));
> + memcpy(dump_tlv->tlv_data, &crash_data->reg_dump_values,
> + sizeof(crash_data->reg_dump_values));
> + sofar += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
I haven't seen you mention that your latest patchset is based on my
patches that remove implicit byte swap from some diag functions so I
infer you're might've missed that reg_dump_values will be byte swapped
on big-endian hosts and userspace will remain unaware of that since
there is no endianess indication in the crash dump structure anymore.
reg_dump_values must be swapped cpu_to_le32 here (or should be never
implicit byte swapped in the first place).
It's probably worth stating in the comments that
ATH10K_FW_CRASH_DUMP_REGDUMP returns a list of __le32.
> + ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer, buffer,
> + dbuf.length);
> + if (ret != 0) {
> + ath10k_warn("failed to read debug log buffer from address 0x%x: %d\n",
> + dbuf.buffer, ret);
> + kfree(buffer);
> + return;
> + }
> +
> + ath10k_debug_dbglog_add(ar, buffer, dbuf.length);
> + kfree(buffer);
Is the `buffer` really a string of bytes (u8) or just u32 in disguise?
If it's the former then on big-endian host the implicit byte swap will
mess it up and userspace will have no way of knowing that now.
Michał
--
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
