| Message ID | 1459855447-17413-2-git-send-email-qca_merez@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote:
> Add __func__ to all wil log macros for easier debugging.
I think this is unnecessary and merely bloats code size.
For all the _dbg calls, dynamic debug can add function
names if desired.
If really desired, I suggest changing the logging
functions to use "%ps and __builtin_return_address(0)
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/6/2016 10:19 AM, Joe Perches wrote: > On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: >> Add __func__ to all wil log macros for easier debugging. > I think this is unnecessary and merely bloats code size. > For all the _dbg calls, dynamic debug can add function names if > desired. > > If really desired, I suggest changing the logging functions to use > "%ps and __builtin_return_address(0) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" > in > the body of a message to majordomo@vger.kernel.org More majordomo info > at http://vger.kernel.org/majordomo-info.html I implemented it with __builtin_return_address(0) at first but found its format less readable (e.g. wil_start_xmit+0x58/0x7e8). I agree that using __builtin_return_address(0) simplifies the code and I'm OK with your suggestion to use it. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Haim, Maya" <merez@qti.qualcomm.com> writes: > On 4/6/2016 10:19 AM, Joe Perches wrote: >> On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: >>> Add __func__ to all wil log macros for easier debugging. >> I think this is unnecessary and merely bloats code size. >> For all the _dbg calls, dynamic debug can add function names if >> desired. >> >> If really desired, I suggest changing the logging functions to use >> "%ps and __builtin_return_address(0) > > I implemented it with __builtin_return_address(0) at first but found > its format less readable (e.g. wil_start_xmit+0x58/0x7e8). Will that work with inline functions and with functions which the compiler has optimised out?
On Thu, 2016-04-07 at 15:04 +0000, Haim, Maya wrote: > On 4/6/2016 10:19 AM, Joe Perches wrote: > > On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: > > > Add __func__ to all wil log macros for easier debugging. > > I think this is unnecessary and merely bloats code size. > > For all the _dbg calls, dynamic debug can add function names if > > desired. > > > > If really desired, I suggest changing the logging functions to use > > "%ps and __builtin_return_address(0) [] > implemented it with __builtin_return_address(0) at first but found its format less readable (e.g. wil_start_xmit+0x58/0x7e8). > I agree that using __builtin_return_address(0) simplifies the code and I'm OK with your suggestion to use it. I believe you must have used %pS and not %ps See: Documentation/printk-formats.txt Symbols/Function Pointers: %pS versatile_init+0x0/0x110 %ps versatile_init -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/7/2016 6:41 PM, Kalle Valo wrote: > "Haim, Maya" <merez@qti.qualcomm.com> writes: > >> On 4/6/2016 10:19 AM, Joe Perches wrote: >>> On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: >>>> Add __func__ to all wil log macros for easier debugging. >>> I think this is unnecessary and merely bloats code size. >>> For all the _dbg calls, dynamic debug can add function names if >>> desired. >>> >>> If really desired, I suggest changing the logging functions to use >>> "%ps and __builtin_return_address(0) >> I implemented it with __builtin_return_address(0) at first but found >> its format less readable (e.g. wil_start_xmit+0x58/0x7e8). > Will that work with inline functions and with functions which the > compiler has optimised out? > That's a good point. I did a quick check and it doesn't work for inline or static functions - for such functions, the name of the calling function is printed. We can either (1) use my initial implementation (2) add __func__ only to the wil_dbg_... macros or (3) revert this patch completely. I find the addition of the function names very useful and since most of the code doesn't include it, it makes the analysis of issues less efficient. Kalle - what is your say on that? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Haim, Maya" <merez@qti.qualcomm.com> writes: > On 4/7/2016 6:41 PM, Kalle Valo wrote: >> "Haim, Maya" <merez@qti.qualcomm.com> writes: >> >>> On 4/6/2016 10:19 AM, Joe Perches wrote: >>>> On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: >>>>> Add __func__ to all wil log macros for easier debugging. >>>> I think this is unnecessary and merely bloats code size. >>>> For all the _dbg calls, dynamic debug can add function names if >>>> desired. >>>> >>>> If really desired, I suggest changing the logging functions to use >>>> "%ps and __builtin_return_address(0) >>> I implemented it with __builtin_return_address(0) at first but found >>> its format less readable (e.g. wil_start_xmit+0x58/0x7e8). >> Will that work with inline functions and with functions which the >> compiler has optimised out? > > That's a good point. I did a quick check and it doesn't work for > inline or static functions - for such functions, the name of the > calling function is printed. Thanks for checking. > We can either (1) use my initial implementation (2) add __func__ only > to the wil_dbg_... macros or (3) revert this patch completely. I find > the addition of the function names very useful and since most of the > code doesn't include it, it makes the analysis of issues less > efficient. Kalle - what is your say on that? I don't have any preference, it's up to you what you like most. One more possibility: in ath10k we have a kconfig option CONFIG_ATH10K_DEBUG to make it possible to disable all overhead from debug functionality, that would at least solve Joe's concern of extra memory usage.
:??? Kalle Valo, 2016-04-15 15:28 ?????? > "Haim, Maya" <merez@qti.qualcomm.com> writes: > >> On 4/7/2016 6:41 PM, Kalle Valo wrote: >>> "Haim, Maya" <merez@qti.qualcomm.com> writes: >>> >>>> On 4/6/2016 10:19 AM, Joe Perches wrote: >>>>> On Tue, 2016-04-05 at 14:24 +0300, Maya Erez wrote: >>>>>> Add __func__ to all wil log macros for easier debugging. >>>>> I think this is unnecessary and merely bloats code size. >>>>> For all the _dbg calls, dynamic debug can add function names if >>>>> desired. >>>>> >>>>> If really desired, I suggest changing the logging functions to use >>>>> "%ps and __builtin_return_address(0) >>>> I implemented it with __builtin_return_address(0) at first but found >>>> its format less readable (e.g. wil_start_xmit+0x58/0x7e8). >>> Will that work with inline functions and with functions which the >>> compiler has optimised out? >> >> That's a good point. I did a quick check and it doesn't work for >> inline or static functions - for such functions, the name of the >> calling function is printed. > > Thanks for checking. > >> We can either (1) use my initial implementation (2) add __func__ only >> to the wil_dbg_... macros or (3) revert this patch completely. I find >> the addition of the function names very useful and since most of the >> code doesn't include it, it makes the analysis of issues less >> efficient. Kalle - what is your say on that? > > I don't have any preference, it's up to you what you like most. > > One more possibility: in ath10k we have a kconfig option > CONFIG_ATH10K_DEBUG to make it possible to disable all overhead from > debug functionality, that would at least solve Joe's concern of extra > memory usage. I looked a bit more on the different options. For the dbg functions, the function name can be added as part of the dynamic debug options (as mentioned by Joe). Therefore I will remove the addition of __func__ from those macros. The dynamic debug also guarantees zero overhead so I don't see the need for an additional debug config flag (such as CONFIG_ATH10K_DEBUG). As for wil_err and wil_info, I believe we can add the __func__ to them, as wil_info is not commonly used in our code. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/wil6210/debug.c b/drivers/net/wireless/ath/wil6210/debug.c index 3249562..0a30127 100644 --- a/drivers/net/wireless/ath/wil6210/debug.c +++ b/drivers/net/wireless/ath/wil6210/debug.c @@ -17,7 +17,7 @@ #include "wil6210.h" #include "trace.h" -void wil_err(struct wil6210_priv *wil, const char *fmt, ...) +void __wil_err(struct wil6210_priv *wil, const char *fmt, ...) { struct net_device *ndev = wil_to_ndev(wil); struct va_format vaf = { @@ -32,7 +32,7 @@ void wil_err(struct wil6210_priv *wil, const char *fmt, ...) va_end(args); } -void wil_err_ratelimited(struct wil6210_priv *wil, const char *fmt, ...) +void __wil_err_ratelimited(struct wil6210_priv *wil, const char *fmt, ...) { if (net_ratelimit()) { struct net_device *ndev = wil_to_ndev(wil); @@ -49,7 +49,7 @@ void wil_err_ratelimited(struct wil6210_priv *wil, const char *fmt, ...) } } -void wil_info(struct wil6210_priv *wil, const char *fmt, ...) +void __wil_info(struct wil6210_priv *wil, const char *fmt, ...) { struct net_device *ndev = wil_to_ndev(wil); struct va_format vaf = { diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h index 4d699ea4..d005370 100644 --- a/drivers/net/wireless/ath/wil6210/wil6210.h +++ b/drivers/net/wireless/ath/wil6210/wil6210.h @@ -635,21 +635,30 @@ struct wil6210_priv { __printf(2, 3) void wil_dbg_trace(struct wil6210_priv *wil, const char *fmt, ...); __printf(2, 3) -void wil_err(struct wil6210_priv *wil, const char *fmt, ...); +void __wil_err(struct wil6210_priv *wil, const char *fmt, ...); __printf(2, 3) -void wil_err_ratelimited(struct wil6210_priv *wil, const char *fmt, ...); +void __wil_err_ratelimited(struct wil6210_priv *wil, const char *fmt, ...); __printf(2, 3) -void wil_info(struct wil6210_priv *wil, const char *fmt, ...); +void __wil_info(struct wil6210_priv *wil, const char *fmt, ...); #define wil_dbg(wil, fmt, arg...) do { \ netdev_dbg(wil_to_ndev(wil), fmt, ##arg); \ wil_dbg_trace(wil, fmt, ##arg); \ } while (0) -#define wil_dbg_irq(wil, fmt, arg...) wil_dbg(wil, "DBG[ IRQ]" fmt, ##arg) -#define wil_dbg_txrx(wil, fmt, arg...) wil_dbg(wil, "DBG[TXRX]" fmt, ##arg) -#define wil_dbg_wmi(wil, fmt, arg...) wil_dbg(wil, "DBG[ WMI]" fmt, ##arg) -#define wil_dbg_misc(wil, fmt, arg...) wil_dbg(wil, "DBG[MISC]" fmt, ##arg) -#define wil_dbg_pm(wil, fmt, arg...) wil_dbg(wil, "DBG[ PM ]" fmt, ##arg) +#define wil_dbg_irq(wil, fmt, arg...) \ + wil_dbg(wil, "DBG[ IRQ]%s: " fmt, __func__, ##arg) +#define wil_dbg_txrx(wil, fmt, arg...) \ + wil_dbg(wil, "DBG[TXRX]%s: " fmt, __func__, ##arg) +#define wil_dbg_wmi(wil, fmt, arg...) \ + wil_dbg(wil, "DBG[ WMI]%s: " fmt, __func__, ##arg) +#define wil_dbg_misc(wil, fmt, arg...) \ + wil_dbg(wil, "DBG[MISC]%s: " fmt, __func__, ##arg) +#define wil_dbg_pm(wil, fmt, arg...) \ + wil_dbg(wil, "DBG[ PM ]%s: " fmt, __func__, ##arg) +#define wil_err(wil, fmt, arg...) __wil_err(wil, "%s: " fmt, __func__, ##arg) +#define wil_info(wil, fmt, arg...) __wil_info(wil, "%s: " fmt, __func__, ##arg) +#define wil_err_ratelimited(wil, fmt, arg...) \ + __wil_err_ratelimited(wil, "%s: " fmt, __func__, ##arg) /* target operations */ /* register read */
Add __func__ to all wil log macros for easier debugging. Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> --- drivers/net/wireless/ath/wil6210/debug.c | 6 +++--- drivers/net/wireless/ath/wil6210/wil6210.h | 25 +++++++++++++++++-------- 2 files changed, 20 insertions(+), 11 deletions(-)