Re: [PATCH 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@vger.kernel.org, ath10k@lists.infradead.org
Subject: Re: [PATCH 1/4] ath10k:  provide firmware crash info via debugfs.
Date: Sat, 07 Jun 2014 08:32:49 -0700	[thread overview]
Message-ID: <539330A1.9050307@candelatech.com> (raw)
In-Reply-To: <874mzwkher.fsf@kamboji.qca.qualcomm.com>

On 06/07/2014 05:55 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 06/05/2014 11:10 PM, Kalle Valo wrote:
>>> Ben Greear <greearb@candelatech.com> writes:
>>>
>>>> I did not do this because I figure we will want ethtool support w/out
>>>> forcing debugfs to be enabled someday soon.
>>>
>>> When we add ethtool support, it's easy to move these back. And then we
>>> need to move the code out from debug.c anyway.
>>
>> Well, it seems like needless churn and makes it harder to follow
>> 'git blame' when you move big chunks around.
>
> I don't care about 'git blame', the actual code is far more important to
> me. If git blame is so important, someone else can improve 'git blame'
> to detect moving code chunks.
>
>> We would not have to move the code out of debug.c, but if you do want
>> it moved, lets pick that place now and just put it there to begin
>> with.
>
> Code in debug.c should have the data it owns in struct ath10k_debug.

Ok, I'll move the data structs to the debug logic for next version of the patch.

>>>> I'm a bit leery of adding spin-locks in the dump routine just for
>>>> this, but I can add and use a new spin-lock if you prefer.
>>>
>>> Why a new spinlock? I didn't review the locking requirements, but I
>>> would first check ar->data_lock can be used.
>>
>> I think it has to be a spin-lock because the crash dump is gathered
>> in the irq handler, so I can't use a mutex as far as I know...
>>
>> I'll work on adding such a lock today.
>
> I asked why add a _new_ spinlock as ar->data_lock is already a spinlock.

Surely we do not want to impede traffic flow just to dump debug info?

And, it is easier to review a specific spinlock rather than use one big
global-ish lock and have to review every use of that lock for issues.

My -v2 had a new spinlock for this new debug data.  I'll move it into the
debug struct with the other new data fields for next version.

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