Re: [PATCH] ath10k: add configurable debugging.

[Kalle Valo <kvalo@qca.qualcomm.com>]

Adrian Chadd <adrian@freebsd.org> writes:

> This adds a few configurable debugging options:
>
> * driver debugging and tracing is now configurable per device

So this means that there's a debugfs file
/sys/kernel/debug/ieee80211/phy0/ath10k/debug for every ath10k device.
Sounds like a good idea. I just don't like the name of file, would
debug_mask be more descriptive?

> * driver debugging and tracing is now configurable at runtime

Correction, debug_mask has been always to possible change via
/sys/module/ath10k_core/parameters/debug_mask file. Now it doesn't work
anymore, which is ok, but I think we should make it clear for users as
well.

I wonder would that work if we change the mask to 0444:

module_param_named(debug_mask, ath10k_debug_mask, uint, 0644);

> * the debugging / tracing is not run at all (besides a mask check)
>   unless the specific debugging bitmap field is configured.

So I guess with this you mean that the function ath10k_dbg() is not
called if debug messages are disabled? This is something which is good
to do in another patch. And the original assumption was that if you care
about performance you won't even enable CONFIG_ATH10K_DEBUG, so that
path was not optimised in any way. How much benefit does this bring?

> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -2418,6 +2418,79 @@ int ath10k_debug_create(struct ath10k *ar)
>  	return 0;
>  }
> =20
> +#ifdef	CONFIG_ATH10K_DEBUGFS
> +static ssize_t ath10k_write_debug_mask(struct file *file,
> +				       const char __user *ubuf,
> +				       size_t count, loff_t *ppos)
> +{

The ifdef looks wrong, it's already inside CONFIG_ATH10K_DEBUGFS. Was it
supposed to be CONFIG_ATH10K_DEBUG?

--=20
Kalle Valo=