On 5/31/2024 4:50 PM, Nemanov, Michael wrote:
> On 5/22/2024 12:46 PM, Krzysztof Kozlowski wrote:>
>>> +
>>> +static int cc33xx_probe(struct platform_device *pdev)
>>> +{
>>> + struct cc33xx *cc;
>>> + struct ieee80211_hw *hw;
>>> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
>>> + const char *nvs_name;
>>> + int ret;
>>> +
>>> + cc33xx_debug(DEBUG_CC33xx, "Wireless Driver Version %s", DRV_VERSION);
>>
>> Drop
>>
>>> +
>>> + if (!pdev_data) {
>>> + cc33xx_error("can't access platform data");
>>
>> Do not use your own print code. Use standard dev_() calls. This applies
>> *everywhere*.
>>
>> [...]
>>
>>> + cc33xx_debug(DEBUG_CC33xx, "WLAN CC33xx platform device probe done");
>>
>> Drop, tracing/sysfs gices you this. Do not print simple
>> success/entry/exit messages.
>>
>> [...]
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, cc33xx_id_table);
>>> +
>>> +static struct platform_driver cc33xx_driver = {
>>> + .probe = cc33xx_probe,
>>> + .remove = cc33xx_remove,
>>> + .id_table = cc33xx_id_table,
>>> + .driver = {
>>> + .name = "cc33xx_driver",
>>> + }
>>> +};
>>> +
>>> +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?
Michael.
On 05/06/2024 11:55, Nemanov, Michael wrote:
> On 5/31/2024 4:50 PM, Nemanov, Michael wrote:
>> On 5/22/2024 12:46 PM, Krzysztof Kozlowski wrote:>
>>>> +
>>>> +static int cc33xx_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct cc33xx *cc;
>>>> + struct ieee80211_hw *hw;
>>>> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
>>>> + const char *nvs_name;
>>>> + int ret;
>>>> +
>>>> + cc33xx_debug(DEBUG_CC33xx, "Wireless Driver Version %s", DRV_VERSION);
>>>
>>> Drop
>>>
>>>> +
>>>> + if (!pdev_data) {
>>>> + cc33xx_error("can't access platform data");
>>>
>>> Do not use your own print code. Use standard dev_() calls. This applies
>>> *everywhere*.
>>>
>>> [...]
>>>
>>>> + cc33xx_debug(DEBUG_CC33xx, "WLAN CC33xx platform device probe done");
>>>
>>> Drop, tracing/sysfs gices you this. Do not print simple
>>> success/entry/exit messages.
>>>
>>> [...]
>>>
>>>> +};
>>>> +MODULE_DEVICE_TABLE(platform, cc33xx_id_table);
>>>> +
>>>> +static struct platform_driver cc33xx_driver = {
>>>> + .probe = cc33xx_probe,
>>>> + .remove = cc33xx_remove,
>>>> + .id_table = cc33xx_id_table,
>>>> + .driver = {
>>>> + .name = "cc33xx_driver",
>>>> + }
>>>> +};
>>>> +
>>>> +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.
Best regards,
Krzysztof
Krzysztof Kozlowski <[email protected]> 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