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

[Ben Greear <greearb@candelatech.com>]

On 06/08/2014 01:28 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 06/07/2014 05:55 AM, Kalle Valo wrote:
>>> Ben Greear <greearb@candelatech.com> writes:
>>>
>>>>>> 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?
>
> I don't see data_lock being used anywhere in hotpath, but of course I
> might have missed something. Anyway, if for optimisation reasons we need
> to introduce a new lock that should happen for a specific case in hot
> path. Handling firmware debug log events is not in that category.
>
>> 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.
>
> Sure, it's easier for you to just add a new lock and then forget :) But
> not for me who would have to maintain 20 different locks for years to
> come. My view is that a new lock should be added on very exceptional
> cases and with good justifications.

I have the opposite opinion on what makes it easy to maintain
locks, but I'll switch to the data-spinlock as you suggest.

Thanks,
Ben

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