[PATCH v6 2/8] ath10k: provide firmware crash info via debugfs

[Michal Kazior]

On 18 August 2014 13:39, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior at tieto.com> writes:
>
>> On 9 August 2014 20:08, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
>>> From: Ben Greear <greearb at candelatech.com>
>>>
>>> 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 at candelatech.com>
>>> Signed-off-by: Kalle Valo <kvalo at qca.qualcomm.com>
>>> ---
>>>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>>>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>>>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>>>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>>>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>>>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>>>  6 files changed, 478 insertions(+), 10 deletions(-)
>> [...]
>>
>>> +struct ath10k_dump_file_data {
>>> +       /* dump file information */
>>> +
>>> +       /* "ATH10K-FW-DUMP" */
>>> +       char df_magic[16];
>>> +
>>> +       u32 len;
>>> +
>>> +       /* 0x1 if host is big-endian */
>>> +       u32 big_endian;
>>
>> This isn't entirely correct. Depending on host endianess you'll end up
>> with 0x1 or 0x1000000. This will still work if you do a boolean
>> evaluation of it in userspace or compare it to 0, but god forbid to
>> compare it with 0x1.
>
> That's true. Didn't you at one point suggest just always using little
> endian? I think that's simplest approach here.

Yes. I did point that out at some time ago.


>>> +       /* some info we can get from ath10k struct that might help */
>>> +
>>> +       u8 uuid[16];
>>> +
>>> +       u32 chip_id;
>>> +
>>> +       /* 0 for now, in place for later hardware */
>>> +       u32 bus_type;
>>
>> Maybe we should have an enum for that instead of using a hardcoded 0?
>
> We had that but you removed it in 3a0861fffd223 =)

.. right :-) I suppose we could just remove the bus_type then? We do
have an unused[128] for future expansion, don't we?


>> and/or the memory should be allocated outside this function completely
>> and make it consume the crashed_since_read (i.e. set it to false) once
>> it's done (while the data_lock is still held).
>
> You mean like allocating the memory in advance, for example during
> module probe time? Then we would have a big chunk of memory we don't use
> for anything most of the time.

I meant just moving it up in the call stack, e.g. to
ath10k_fw_crash_dump_open().


Michał