Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.

From: Ben Greear <greearb@candelatech.com>
To: Kalle Valo <kvalo@qca.qualcomm.com>
Cc: linux-wireless <linux-wireless@vger.kernel.org>,
	Michal Kazior <michal.kazior@tieto.com>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>
Subject: Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
Date: Thu, 05 Jun 2014 08:49:09 -0700	[thread overview]
Message-ID: <53909175.30009@candelatech.com> (raw)
In-Reply-To: <8738fjppba.fsf@kamboji.qca.qualcomm.com>

On 06/05/2014 04:29 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
> 
>> On 06/04/2014 02:04 AM, Michal Kazior wrote:
>>> On 3 June 2014 18:25,  <greearb@candelatech.com> wrote:
>>>
>>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>>> @@ -41,6 +41,8 @@
>>>>  #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>>>>  #define ATH10K_NUM_CHANS 38
>>>>
>>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>>>
>>> I don't like this.
> 
> Me neither.
> 
>> You want me to make a different define for this that duplicates
>> the '60' value?  At least with my method above, we should get compile
>> errors if the values ever diverge in the two files.
> 
> There should be only one define. Somewhere (forgot where) Michal
> suggested hw.h, I think this define would be a good candidate to move
> there too.

Ok, I'll move it to hw.h

>>>> +/* This will store at least the last 128 entries. */
>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>
>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>
>> It is from the guts of how the firmware does debug logs.
>>
>> Each entry is a max of 7 32-bit integers in length.
>>
>>> The 128 should probably be a separate #define?
>>
>> I don't see why...
> 
> To make the code more readable.
> 
>> dbglog messages are variable number of 32-bit integers in length, so
>> the 128 is fairly arbitrary.
> 
> A person should immeaditely understand where the numbers are coming from
> and not start googling about it. The minimum is to put your description
> above to the comment. And it would be clearer to replace 4 with
> sizeof(u32).

Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
be sufficient, or do you want actual defines?  To really understand
this code you need details from how the firmware encodes the messages.
I would rather a QCA person add that info just in case it is considered
protected info...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k