* [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled
2013-08-27 14:47 [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Dmitry Kasatkin
@ 2013-08-27 14:47 ` Dmitry Kasatkin
2013-08-27 14:52 ` Dmitry Kasatkin
2013-08-27 16:20 ` [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Joe Perches
2013-08-28 17:43 ` [PATCHv2 1/2] " Sarah Sharp
2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Kasatkin @ 2013-08-27 14:47 UTC (permalink / raw)
To: sarah.a.sharp, gregkh; +Cc: dmitry.kasatkin, linux-kernel
When debug is not enabled and dev_dbg() will expand to nothing,
log might be flooded with "callbacks suppressed". If it was not
done on purpose, better to use dev_dbg_ratelimited() instead.
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
drivers/usb/host/xhci-ring.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5b08cd8..3cbf1c0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3060,14 +3060,10 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
* to set the polling interval (once the API is added).
*/
if (xhci_interval != ep_interval) {
- if (printk_ratelimit())
- dev_dbg(&urb->dev->dev, "Driver uses different interval"
- " (%d microframe%s) than xHCI "
- "(%d microframe%s)\n",
- ep_interval,
- ep_interval == 1 ? "" : "s",
- xhci_interval,
- xhci_interval == 1 ? "" : "s");
+ dev_dbg_ratelimited(&urb->dev->dev,
+ "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
+ ep_interval, ep_interval == 1 ? "" : "s",
+ xhci_interval, xhci_interval == 1 ? "" : "s");
urb->interval = xhci_interval;
/* Convert back to frames for LS/FS devices */
if (urb->dev->speed == USB_SPEED_LOW ||
@@ -3849,14 +3845,10 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
* to set the polling interval (once the API is added).
*/
if (xhci_interval != ep_interval) {
- if (printk_ratelimit())
- dev_dbg(&urb->dev->dev, "Driver uses different interval"
- " (%d microframe%s) than xHCI "
- "(%d microframe%s)\n",
- ep_interval,
- ep_interval == 1 ? "" : "s",
- xhci_interval,
- xhci_interval == 1 ? "" : "s");
+ dev_dbg_ratelimited(&urb->dev->dev,
+ "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
+ ep_interval, ep_interval == 1 ? "" : "s",
+ xhci_interval, xhci_interval == 1 ? "" : "s");
urb->interval = xhci_interval;
/* Convert back to frames for LS/FS devices */
if (urb->dev->speed == USB_SPEED_LOW ||
--
1.8.1.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled
2013-08-27 14:47 ` [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled Dmitry Kasatkin
@ 2013-08-27 14:52 ` Dmitry Kasatkin
2013-08-27 17:55 ` Sarah Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Kasatkin @ 2013-08-27 14:52 UTC (permalink / raw)
To: Dmitry Kasatkin; +Cc: Sarah Sharp, Greg KH, linux-kernel
Used vim "=" as Sarah suggested.
- Dmitry
On Tue, Aug 27, 2013 at 5:47 PM, Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> When debug is not enabled and dev_dbg() will expand to nothing,
> log might be flooded with "callbacks suppressed". If it was not
> done on purpose, better to use dev_dbg_ratelimited() instead.
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
> drivers/usb/host/xhci-ring.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 5b08cd8..3cbf1c0 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3060,14 +3060,10 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> * to set the polling interval (once the API is added).
> */
> if (xhci_interval != ep_interval) {
> - if (printk_ratelimit())
> - dev_dbg(&urb->dev->dev, "Driver uses different interval"
> - " (%d microframe%s) than xHCI "
> - "(%d microframe%s)\n",
> - ep_interval,
> - ep_interval == 1 ? "" : "s",
> - xhci_interval,
> - xhci_interval == 1 ? "" : "s");
> + dev_dbg_ratelimited(&urb->dev->dev,
> + "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
> + ep_interval, ep_interval == 1 ? "" : "s",
> + xhci_interval, xhci_interval == 1 ? "" : "s");
> urb->interval = xhci_interval;
> /* Convert back to frames for LS/FS devices */
> if (urb->dev->speed == USB_SPEED_LOW ||
> @@ -3849,14 +3845,10 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> * to set the polling interval (once the API is added).
> */
> if (xhci_interval != ep_interval) {
> - if (printk_ratelimit())
> - dev_dbg(&urb->dev->dev, "Driver uses different interval"
> - " (%d microframe%s) than xHCI "
> - "(%d microframe%s)\n",
> - ep_interval,
> - ep_interval == 1 ? "" : "s",
> - xhci_interval,
> - xhci_interval == 1 ? "" : "s");
> + dev_dbg_ratelimited(&urb->dev->dev,
> + "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
> + ep_interval, ep_interval == 1 ? "" : "s",
> + xhci_interval, xhci_interval == 1 ? "" : "s");
> urb->interval = xhci_interval;
> /* Convert back to frames for LS/FS devices */
> if (urb->dev->speed == USB_SPEED_LOW ||
> --
> 1.8.1.2
>
--
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled
2013-08-27 14:52 ` Dmitry Kasatkin
@ 2013-08-27 17:55 ` Sarah Sharp
0 siblings, 0 replies; 12+ messages in thread
From: Sarah Sharp @ 2013-08-27 17:55 UTC (permalink / raw)
To: Dmitry Kasatkin; +Cc: Dmitry Kasatkin, Greg KH, linux-kernel
Hi Dmitry,
This patch looks fine, and applies ok. I'll send it off to Greg with a
couple more bug fixes shortly.
Sarah Sharp
On Tue, Aug 27, 2013 at 05:52:33PM +0300, Dmitry Kasatkin wrote:
> Used vim "=" as Sarah suggested.
>
> - Dmitry
>
> On Tue, Aug 27, 2013 at 5:47 PM, Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> > When debug is not enabled and dev_dbg() will expand to nothing,
> > log might be flooded with "callbacks suppressed". If it was not
> > done on purpose, better to use dev_dbg_ratelimited() instead.
> >
> > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > ---
> > drivers/usb/host/xhci-ring.c | 24 ++++++++----------------
> > 1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 5b08cd8..3cbf1c0 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3060,14 +3060,10 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> > * to set the polling interval (once the API is added).
> > */
> > if (xhci_interval != ep_interval) {
> > - if (printk_ratelimit())
> > - dev_dbg(&urb->dev->dev, "Driver uses different interval"
> > - " (%d microframe%s) than xHCI "
> > - "(%d microframe%s)\n",
> > - ep_interval,
> > - ep_interval == 1 ? "" : "s",
> > - xhci_interval,
> > - xhci_interval == 1 ? "" : "s");
> > + dev_dbg_ratelimited(&urb->dev->dev,
> > + "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
> > + ep_interval, ep_interval == 1 ? "" : "s",
> > + xhci_interval, xhci_interval == 1 ? "" : "s");
> > urb->interval = xhci_interval;
> > /* Convert back to frames for LS/FS devices */
> > if (urb->dev->speed == USB_SPEED_LOW ||
> > @@ -3849,14 +3845,10 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> > * to set the polling interval (once the API is added).
> > */
> > if (xhci_interval != ep_interval) {
> > - if (printk_ratelimit())
> > - dev_dbg(&urb->dev->dev, "Driver uses different interval"
> > - " (%d microframe%s) than xHCI "
> > - "(%d microframe%s)\n",
> > - ep_interval,
> > - ep_interval == 1 ? "" : "s",
> > - xhci_interval,
> > - xhci_interval == 1 ? "" : "s");
> > + dev_dbg_ratelimited(&urb->dev->dev,
> > + "Driver uses different interval (%d microframe%s) than xHCI (%d microframe%s)\n",
> > + ep_interval, ep_interval == 1 ? "" : "s",
> > + xhci_interval, xhci_interval == 1 ? "" : "s");
> > urb->interval = xhci_interval;
> > /* Convert back to frames for LS/FS devices */
> > if (urb->dev->speed == USB_SPEED_LOW ||
> > --
> > 1.8.1.2
> >
>
>
>
> --
> Thanks,
> Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-27 14:47 [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Dmitry Kasatkin
2013-08-27 14:47 ` [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled Dmitry Kasatkin
@ 2013-08-27 16:20 ` Joe Perches
2013-08-27 17:32 ` Jason Baron
2013-08-28 17:43 ` [PATCHv2 1/2] " Sarah Sharp
2 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-08-27 16:20 UTC (permalink / raw)
To: Dmitry Kasatkin, Hiroshi Doyu, Jason Baron
Cc: sarah.a.sharp, gregkh, dmitry.kasatkin, linux-kernel
On Tue, 2013-08-27 at 17:47 +0300, Dmitry Kasatkin wrote:
> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
> It leads to build break.
> For example, when I try to use dev_dbg_ratelimited in USB code and
> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
Jason?
Seems mostly sensible to me but I think the first check
needs to be
#if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
Also, it's curious why it was ever using __dynamic_pr_debug
instead of __dynamic_dev_dbg
Maybe for completeness there should be a
dev_vdbg_ratelimited too.
Additional bit below...
> CC [M] drivers/usb/host/xhci-ring.o
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
> drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
> drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
> make[1]: *** [drivers/usb/host] Error 2
> make: *** [drivers/usb/] Error 2
>
> This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
> include/linux/device.h | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 22b546a..d336beb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1099,17 +1099,28 @@ do { \
> dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
> #define dev_info_ratelimited(dev, fmt, ...) \
> dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
> -#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> +#if defined(CONFIG_DYNAMIC_DEBUG)
I think this needs to be
#if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
> #define dev_dbg_ratelimited(dev, fmt, ...) \
> do { \
> static DEFINE_RATELIMIT_STATE(_rs, \
> DEFAULT_RATELIMIT_INTERVAL, \
> DEFAULT_RATELIMIT_BURST); \
> DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> + /* descriptor check is first to prevent flooding with \
> + "callbacks suppressed" */ \
> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
> __ratelimit(&_rs)) \
> - __dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
> - ##__VA_ARGS__); \
> + __dynamic_dev_dbg(&descriptor, dev, fmt, \
> + ##__VA_ARGS__); \
> +} while (0)
> +#elif defined(DEBUG)
> +#define dev_dbg_ratelimited(dev, fmt, ...) \
> +do { \
> + static DEFINE_RATELIMIT_STATE(_rs, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + if (__ratelimit(&_rs)) \
> + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
> } while (0)
> #else
> #define dev_dbg_ratelimited(dev, fmt, ...) \
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-27 16:20 ` [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Joe Perches
@ 2013-08-27 17:32 ` Jason Baron
2013-08-27 18:16 ` Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Jason Baron @ 2013-08-27 17:32 UTC (permalink / raw)
To: Joe Perches
Cc: Dmitry Kasatkin, Hiroshi Doyu, sarah.a.sharp@linux.intel.com,
gregkh@linuxfoundation.org, dmitry.kasatkin@gmail.com,
linux-kernel@vger.kernel.org
On 08/27/2013 12:20 PM, Joe Perches wrote:
> On Tue, 2013-08-27 at 17:47 +0300, Dmitry Kasatkin wrote:
>> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
>> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
>> It leads to build break.
>> For example, when I try to use dev_dbg_ratelimited in USB code and
>> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
> Jason?
>
> Seems mostly sensible to me but I think the first check
> needs to be
>
> #if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
Why? All the other call-sites, do it the way Dmitry has done it.
> Also, it's curious why it was ever using __dynamic_pr_debug
> instead of __dynamic_dev_dbg
Not sure.
> Maybe for completeness there should be a
> dev_vdbg_ratelimited too.
I could also see the rate limiting being pushed further down, such that
dynamic debug could control how and whether or not its enabled.
Thanks,
-Jason
> Additional bit below...
>
>> CC [M] drivers/usb/host/xhci-ring.o
>> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
>> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
>> drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
>> drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
>> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
>> drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
>> cc1: some warnings being treated as errors
>> make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
>> make[1]: *** [drivers/usb/host] Error 2
>> make: *** [drivers/usb/] Error 2
>>
>> This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>> include/linux/device.h | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 22b546a..d336beb 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1099,17 +1099,28 @@ do { \
>> dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
>> #define dev_info_ratelimited(dev, fmt, ...) \
>> dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
>> -#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG)
> I think this needs to be
>
> #if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
>
>> #define dev_dbg_ratelimited(dev, fmt, ...) \
>> do { \
>> static DEFINE_RATELIMIT_STATE(_rs, \
>> DEFAULT_RATELIMIT_INTERVAL, \
>> DEFAULT_RATELIMIT_BURST); \
>> DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>> + /* descriptor check is first to prevent flooding with \
>> + "callbacks suppressed" */ \
>> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
>> __ratelimit(&_rs)) \
>> - __dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
>> - ##__VA_ARGS__); \
>> + __dynamic_dev_dbg(&descriptor, dev, fmt, \
>> + ##__VA_ARGS__); \
>> +} while (0)
>> +#elif defined(DEBUG)
>> +#define dev_dbg_ratelimited(dev, fmt, ...) \
>> +do { \
>> + static DEFINE_RATELIMIT_STATE(_rs, \
>> + DEFAULT_RATELIMIT_INTERVAL, \
>> + DEFAULT_RATELIMIT_BURST); \
>> + if (__ratelimit(&_rs)) \
>> + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
>> } while (0)
>> #else
>> #define dev_dbg_ratelimited(dev, fmt, ...) \
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-27 17:32 ` Jason Baron
@ 2013-08-27 18:16 ` Joe Perches
2013-08-27 18:30 ` Dmitry Kasatkin
0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-08-27 18:16 UTC (permalink / raw)
To: Jason Baron
Cc: Dmitry Kasatkin, Hiroshi Doyu, sarah.a.sharp@linux.intel.com,
gregkh@linuxfoundation.org, dmitry.kasatkin@gmail.com,
linux-kernel@vger.kernel.org
On Tue, 2013-08-27 at 13:32 -0400, Jason Baron wrote:
> On 08/27/2013 12:20 PM, Joe Perches wrote:
> > On Tue, 2013-08-27 at 17:47 +0300, Dmitry Kasatkin wrote:
> >> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
> >> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
> >> It leads to build break.
> >> For example, when I try to use dev_dbg_ratelimited in USB code and
> >> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
> > Jason?
> >
> > Seems mostly sensible to me but I think the first check
> > needs to be
> >
> > #if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
>
> Why? All the other call-sites, do it the way Dmitry has done it.
Fine. Originally I thought it useful to not
store the ratelimit state, but this way those
messages can be enabled via the dynamic_debug
control.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-27 18:16 ` Joe Perches
@ 2013-08-27 18:30 ` Dmitry Kasatkin
2013-08-28 17:50 ` [PATCH v3 1/1] " Sarah Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Kasatkin @ 2013-08-27 18:30 UTC (permalink / raw)
To: Joe Perches
Cc: Jason Baron, Dmitry Kasatkin, Hiroshi Doyu,
sarah.a.sharp@linux.intel.com, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org
On Tue, Aug 27, 2013 at 9:16 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2013-08-27 at 13:32 -0400, Jason Baron wrote:
>> On 08/27/2013 12:20 PM, Joe Perches wrote:
>> > On Tue, 2013-08-27 at 17:47 +0300, Dmitry Kasatkin wrote:
>> >> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
>> >> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
>> >> It leads to build break.
>> >> For example, when I try to use dev_dbg_ratelimited in USB code and
>> >> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
>> > Jason?
>> >
>> > Seems mostly sensible to me but I think the first check
>> > needs to be
>> >
>> > #if defined(CONFIG_DYNAMIC_DEBUG) && defined(DEBUG)
>>
>> Why? All the other call-sites, do it the way Dmitry has done it.
>
I followed the pattern..
> Fine. Originally I thought it useful to not
> store the ratelimit state, but this way those
> messages can be enabled via the dynamic_debug
> control.
>
Indeed.
Thanks.
- Dmitry
--
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/1] dev-core: fix build break when DEBUG is enabled
2013-08-27 18:30 ` Dmitry Kasatkin
@ 2013-08-28 17:50 ` Sarah Sharp
2013-08-28 17:56 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Sarah Sharp @ 2013-08-28 17:50 UTC (permalink / raw)
To: Joe Perches
Cc: Jason Baron, Dmitry Kasatkin, Hiroshi Doyu, Greg KH, linux-kernel,
linux-usb
Any objections to queuing this patch through Greg's usb-next tree? I've
already sent the 2/2 patch, which will cause build breakage without this
patch.
Changes since v2:
- Fixed a checkpatch warning caused by a comment in a macro.
Sarah Sharp
>8----------------------------------------------------------------8<
From: Dmitry Kasatkin <d.kasatkin@samsung.com>
When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
structures even when CONFIG_DYNAMIC_DEBUG is not defined.
It leads to build break.
For example, when I try to use dev_dbg_ratelimited in USB code and
CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
CC [M] drivers/usb/host/xhci-ring.o
drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
cc1: some warnings being treated as errors
make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
make[1]: *** [drivers/usb/host] Error 2
make: *** [drivers/usb/] Error 2
This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
[Note, Sarah moved the comment above the macro to avoid checkpatch
warnings.]
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
---
include/linux/device.h | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 22b546a..7d960d5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1099,7 +1099,8 @@ do { \
dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
#define dev_info_ratelimited(dev, fmt, ...) \
dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
-#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
+#if defined(CONFIG_DYNAMIC_DEBUG)
+/* descriptor check is first to prevent flooding with "callbacks suppressed" */
#define dev_dbg_ratelimited(dev, fmt, ...) \
do { \
static DEFINE_RATELIMIT_STATE(_rs, \
@@ -1108,8 +1109,17 @@ do { \
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
__ratelimit(&_rs)) \
- __dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
- ##__VA_ARGS__); \
+ __dynamic_dev_dbg(&descriptor, dev, fmt, \
+ ##__VA_ARGS__); \
+} while (0)
+#elif defined(DEBUG)
+#define dev_dbg_ratelimited(dev, fmt, ...) \
+do { \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ if (__ratelimit(&_rs)) \
+ dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
} while (0)
#else
#define dev_dbg_ratelimited(dev, fmt, ...) \
--
1.8.3.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/1] dev-core: fix build break when DEBUG is enabled
2013-08-28 17:50 ` [PATCH v3 1/1] " Sarah Sharp
@ 2013-08-28 17:56 ` Greg KH
0 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2013-08-28 17:56 UTC (permalink / raw)
To: Sarah Sharp
Cc: Joe Perches, Jason Baron, Dmitry Kasatkin, Hiroshi Doyu,
linux-kernel, linux-usb
On Wed, Aug 28, 2013 at 10:50:31AM -0700, Sarah Sharp wrote:
> Any objections to queuing this patch through Greg's usb-next tree? I've
> already sent the 2/2 patch, which will cause build breakage without this
> patch.
>
> Changes since v2:
> - Fixed a checkpatch warning caused by a comment in a macro.
>
> Sarah Sharp
>
> >8----------------------------------------------------------------8<
>
> From: Dmitry Kasatkin <d.kasatkin@samsung.com>
>
> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
> It leads to build break.
> For example, when I try to use dev_dbg_ratelimited in USB code and
> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
>
> CC [M] drivers/usb/host/xhci-ring.o
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
> drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
> drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
> make[1]: *** [drivers/usb/host] Error 2
> make: *** [drivers/usb/] Error 2
>
> This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
>
> [Note, Sarah moved the comment above the macro to avoid checkpatch
> warnings.]
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-27 14:47 [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Dmitry Kasatkin
2013-08-27 14:47 ` [PATCHv2 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled Dmitry Kasatkin
2013-08-27 16:20 ` [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled Joe Perches
@ 2013-08-28 17:43 ` Sarah Sharp
2013-08-28 17:49 ` Dmitry Kasatkin
2 siblings, 1 reply; 12+ messages in thread
From: Sarah Sharp @ 2013-08-28 17:43 UTC (permalink / raw)
To: Dmitry Kasatkin; +Cc: gregkh, dmitry.kasatkin, linux-kernel
Applying: dev-core: fix build break when DEBUG is enabled
WARNING: Avoid unnecessary line continuations
#18: FILE: include/linux/device.h:1110:
+ "callbacks suppressed" */ \
WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...
#33: FILE: include/linux/device.h:1123:
+ dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
total: 0 errors, 2 warnings, 31 lines checked
I'm just going to fix the first warning myself by moving your comment
above the macro. Please don't add comments inside macros, and please
run your patches through checkpatch.pl.
Sarah Sharp
On Tue, Aug 27, 2013 at 05:47:34PM +0300, Dmitry Kasatkin wrote:
> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
> It leads to build break.
> For example, when I try to use dev_dbg_ratelimited in USB code and
> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
>
> CC [M] drivers/usb/host/xhci-ring.o
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
> drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
> drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
> make[1]: *** [drivers/usb/host] Error 2
> make: *** [drivers/usb/] Error 2
>
> This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
> include/linux/device.h | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 22b546a..d336beb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1099,17 +1099,28 @@ do { \
> dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
> #define dev_info_ratelimited(dev, fmt, ...) \
> dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
> -#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> #define dev_dbg_ratelimited(dev, fmt, ...) \
> do { \
> static DEFINE_RATELIMIT_STATE(_rs, \
> DEFAULT_RATELIMIT_INTERVAL, \
> DEFAULT_RATELIMIT_BURST); \
> DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> + /* descriptor check is first to prevent flooding with \
> + "callbacks suppressed" */ \
> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
> __ratelimit(&_rs)) \
> - __dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
> - ##__VA_ARGS__); \
> + __dynamic_dev_dbg(&descriptor, dev, fmt, \
> + ##__VA_ARGS__); \
> +} while (0)
> +#elif defined(DEBUG)
> +#define dev_dbg_ratelimited(dev, fmt, ...) \
> +do { \
> + static DEFINE_RATELIMIT_STATE(_rs, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + if (__ratelimit(&_rs)) \
> + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
> } while (0)
> #else
> #define dev_dbg_ratelimited(dev, fmt, ...) \
> --
> 1.8.1.2
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCHv2 1/2] dev-core: fix build break when DEBUG is enabled
2013-08-28 17:43 ` [PATCHv2 1/2] " Sarah Sharp
@ 2013-08-28 17:49 ` Dmitry Kasatkin
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Kasatkin @ 2013-08-28 17:49 UTC (permalink / raw)
To: Sarah Sharp; +Cc: gregkh, dmitry.kasatkin, linux-kernel
On 28/08/13 20:43, Sarah Sharp wrote:
> Applying: dev-core: fix build break when DEBUG is enabled
> WARNING: Avoid unnecessary line continuations
> #18: FILE: include/linux/device.h:1110:
> + "callbacks suppressed" */ \
>
> WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...
> #33: FILE: include/linux/device.h:1123:
> + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
>
> total: 0 errors, 2 warnings, 31 lines checked
>
> I'm just going to fix the first warning myself by moving your comment
> above the macro. Please don't add comments inside macros, and please
> run your patches through checkpatch.pl.
I run checkpatch always.
The first warning is in my understanding is not related to comments
inside macro,
but just that avoid using "\".
I would not remove comment, because it is for explaining the macro line...
Second warning is natural in this case because macro itself defines
dev_dbg() functionality.
Internally it must use something else than itself...
- Dmitry
> Sarah Sharp
>
> On Tue, Aug 27, 2013 at 05:47:34PM +0300, Dmitry Kasatkin wrote:
>> When DEBUG is defined, dev_dbg_ratelimited uses dynamic debug data
>> structures even when CONFIG_DYNAMIC_DEBUG is not defined.
>> It leads to build break.
>> For example, when I try to use dev_dbg_ratelimited in USB code and
>> CONFIG_USB_DEBUG is enabled, but CONFIG_DYNAMIC_DEBUG is not, I get:
>>
>> CC [M] drivers/usb/host/xhci-ring.o
>> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_intr_tx’:
>> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘DEFINE_DYNAMIC_DEBUG_METADATA’ [-Werror=implicit-function-declaration]
>> drivers/usb/host/xhci-ring.c:3059:3: error: ‘descriptor’ undeclared (first use in this function)
>> drivers/usb/host/xhci-ring.c:3059:3: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/host/xhci-ring.c:3059:3: error: implicit declaration of function ‘__dynamic_pr_debug’ [-Werror=implicit-function-declaration]
>> drivers/usb/host/xhci-ring.c: In function ‘xhci_queue_isoc_tx_prepare’:
>> drivers/usb/host/xhci-ring.c:3847:3: error: ‘descriptor’ undeclared (first use in this function)
>> cc1: some warnings being treated as errors
>> make[2]: *** [drivers/usb/host/xhci-ring.o] Error 1
>> make[1]: *** [drivers/usb/host] Error 2
>> make: *** [drivers/usb/] Error 2
>>
>> This patch separates definition for CONFIG_DYNAMIC_DEBUG and DEBUG cases.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>> include/linux/device.h | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 22b546a..d336beb 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1099,17 +1099,28 @@ do { \
>> dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
>> #define dev_info_ratelimited(dev, fmt, ...) \
>> dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
>> -#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>> #define dev_dbg_ratelimited(dev, fmt, ...) \
>> do { \
>> static DEFINE_RATELIMIT_STATE(_rs, \
>> DEFAULT_RATELIMIT_INTERVAL, \
>> DEFAULT_RATELIMIT_BURST); \
>> DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>> + /* descriptor check is first to prevent flooding with \
>> + "callbacks suppressed" */ \
>> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
>> __ratelimit(&_rs)) \
>> - __dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
>> - ##__VA_ARGS__); \
>> + __dynamic_dev_dbg(&descriptor, dev, fmt, \
>> + ##__VA_ARGS__); \
>> +} while (0)
>> +#elif defined(DEBUG)
>> +#define dev_dbg_ratelimited(dev, fmt, ...) \
>> +do { \
>> + static DEFINE_RATELIMIT_STATE(_rs, \
>> + DEFAULT_RATELIMIT_INTERVAL, \
>> + DEFAULT_RATELIMIT_BURST); \
>> + if (__ratelimit(&_rs)) \
>> + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \
>> } while (0)
>> #else
>> #define dev_dbg_ratelimited(dev, fmt, ...) \
>> --
>> 1.8.1.2
>>
^ permalink raw reply [flat|nested] 12+ messages in thread