Received-SPF: pass (google.com: domain of linux-wireless+bounces-8548-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Kalle Valo <kvalo@kernel.org>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: "Nemanov, Michael" <michael.nemanov@ti.com>,  Johannes Berg
 <johannes.berg@intel.com>,  Breno Leitao <leitao@debian.org>,  Justin
 Stitt <justinstitt@google.com>,  Kees Cook <keescook@chromium.org>,
  linux-wireless@vger.kernel.org,  linux-kernel@vger.kernel.org,  Sabeeh
 Khan <sabeeh-khan@ti.com>
Subject: Re: [PATCH 08/17] Add main.c
References: <20240521171841.884576-1-michael.nemanov@ti.com>
	<20240521171841.884576-9-michael.nemanov@ti.com>
	<cfe33bf1-9df3-4d02-b4ed-e29a430b106d@kernel.org>
	<456c8076-1e3a-4cc9-895c-e707e68fe610@ti.com>
	<97d8acf9-6cb3-4da7-ad4e-0f2d0a63c172@kernel.org>
	<2e2ec1ba-0c24-4173-af60-ea51004f2e10@ti.com>
	<eea16e12-6e9d-4630-87e6-f44071ab1c4e@ti.com>
	<2c4283be-0369-4be3-ba07-987b9fb41476@kernel.org>
Date: Wed, 05 Jun 2024 14:13:17 +0300
In-Reply-To: <2c4283be-0369-4be3-ba07-987b9fb41476@kernel.org> (Krzysztof
	Kozlowski's message of "Wed, 5 Jun 2024 12:04:29 +0200")
Message-ID: <87a5jzmylu.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Krzysztof Kozlowski <krzk@kernel.org> writes:

>>>>> +u32 cc33xx_debug_level = DEBUG_NO_DATAPATH;
>>>>>
>>>> Why this is global? Why u32? Why global variable is defined at the end
>>>> of the file?!?!
>>>  
>>> cc33xx_debug_level together with cc33xx_debug/info/error() macros is how
>>> all traces were done in drivers/net/wireless/ti/wlcore/ (originally was
>>> wl1271_debug/info etc.)
>>> It enables / disables traces without rebuilding or even reloading which
>>> is very helpful for remote support. These macros map to dynamic_pr_debug
>>> / pr_debug. I saw similar wrappers in other wireless drivers (ath12k).
>>> This is also why there are plenty of cc33xx_debug() all over the code,
>>> most are silent by default.
>> 
>> Any more thoughts on debug traces? I'll remove all trivial function 
>> entry / exit traces as Krzysztof requested. Is it OK to keep other 
>> cc33xx_debug() calls which will be off by default?
>
> Sorry, I don't see the point. Dynamic debug gives you debug control. You
> just added orthogonal code to existing debug infrastructure, so as far
> as I am concerned, this should be dropped in favor of standard debugging
> calls.

I think most wireless drivers have this kind of debug level parameter,
for example debug_level in iwlwifi or debug_mask in ath12k. Our problem
with the dynamic debug framework is that it's either too fine grained
(ie. per line) or too coarse (per file), but in wireless we usually want
to have some kind of per functionality level debugging as well. For
example, to show only management frame transmissions, certain firmware
interface commands and so on.

A long time ago there was a discussion about extending dynamic debug
framework but not sure if that ever happened.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches