[PATCH 2/2] xhci:prevent "callbacks suppressed" when debug is not enabled

[Dmitry Kasatkin]

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.kas...@samsung.com>
---
drivers/usb/host/xhci-ring.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5b08cd8..5298232 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3060,8 +3060,7 @@ 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"
+ dev_dbg_ratelimited(&urb->dev->dev, "Driver uses different interval"
" (%d microframe%s) than xHCI "
"(%d microframe%s)\n",
ep_interval,
@@ -3849,8 +3848,7 @@ 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"
+ dev_dbg_ratelimited(&urb->dev->dev, "Driver uses different interval"
" (%d microframe%s) than xHCI "
"(%d microframe%s)\n",
ep_interval,
--
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Dmitry Kasatkin]

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.kas...@samsung.com>
Cc: sta...@vger.kernel.org
---
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, ...) \

[Greg KH]

On Thu, Aug 15, 2013 at 07:04:54PM +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.kas...@samsung.com>
> Cc: sta...@vger.kernel.org

How is this a stable issue? I don't see how the rules listed in
Documentation/stable_kernel_rules.txt apply here, what am I missing?

Not to say your patch isn't correct, just that it's not stable material,
right?

thanks,

greg k-h

[Dmitry Kasatkin]

There are few drivers which uses dev_dbg_ratelimited().
In the case someone tries to build kernel with those drivers with DEBUG
enabled and without CONFIG_DYNAMIC_DEBUG,
will get the build break. It will happen also on stable kernel.

- Dmitry

[Greg KH]

Do you have specific examples of this happening? Given that this code
has been this way for a very long time now, and Randy's excellent
'random .config bot' normally catches this type of thing, I think the
applicability to a stable release is pretty low.

Please read that Documentation file again about "actual" issues, not
just theoretical ones.

thanks,

greg k-h

[Greg KH]

On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> ---
> drivers/usb/host/xhci-ring.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)

Sarah, does this patch conflict with the trace debug patches being
worked on? I'll hold off on applying it for now, let me know if it's ok
or not.

thanks,

greg k-h

[Sarah Sharp]

On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
> On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> > ---
> > drivers/usb/host/xhci-ring.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
>
>
> Sarah, does this patch conflict with the trace debug patches being
> worked on? I'll hold off on applying it for now, let me know if it's ok
> or not.

It doesn't conflict with the trace debug patches, because those only
effect debugging with xhci_dbg with the host device, not dev_dbg with
the USB device. This should apply fine to usb-next.

Sarah Sharp

[Sarah Sharp]

On Fri, Aug 16, 2013 at 10:30:35AM -0700, Greg KH wrote:

> On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
> > On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
> > > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> > > > ---
> > > > drivers/usb/host/xhci-ring.c | 6 ++----
> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > >
> > > Sarah, does this patch conflict with the trace debug patches being
> > > worked on? I'll hold off on applying it for now, let me know if it's ok
> > > or not.
> >
> > It doesn't conflict with the trace debug patches, because those only
> > effect debugging with xhci_dbg with the host device, not dev_dbg with
> > the USB device. This should apply fine to usb-next.
>

> Ok, can I get your ack for it so I can apply it?

Dmitry, can you fix the indentation issue and resubmit this? I'll ack
it then.

[Greg KH]

On Fri, Aug 16, 2013 at 10:30:00AM -0700, Sarah Sharp wrote:

> On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
> > On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
> > > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> > > > ---
> > > > drivers/usb/host/xhci-ring.c | 6 ++----
> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > >
> > > Sarah, does this patch conflict with the trace debug patches being
> > > worked on? I'll hold off on applying it for now, let me know if it's ok
> > > or not.
> >
> > It doesn't conflict with the trace debug patches, because those only
> > effect debugging with xhci_dbg with the host device, not dev_dbg with
> > the USB device. This should apply fine to usb-next.
>

> At another glance, the patch removes two if blocks, but doesn't
> re-indent the rest of the lines:

>
> > @@ -3060,8 +3060,7 @@ 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"
> > + dev_dbg_ratelimited(&urb->dev->dev, "Driver uses different interval"
> > " (%d microframe%s) than xHCI "
> > "(%d microframe%s)\n",
> > ep_interval,
>

> That should probably be fixed.

Good catch, the string should also be fixed while the lines are being
touched, to not be split across multiple lines.

Dmitry, can you make that change and resubmit?

thanks,

greg k-h

[Dmitry Kasatkin]

On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <sarah....@intel.com> wrote:

> On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
>> On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
>> > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
>> > > ---
>> > > drivers/usb/host/xhci-ring.c | 6 ++----
>> > > 1 file changed, 2 insertions(+), 4 deletions(-)
>> >
>> >
>> > Sarah, does this patch conflict with the trace debug patches being
>> > worked on? I'll hold off on applying it for now, let me know if it's ok
>> > or not.
>>
>> It doesn't conflict with the trace debug patches, because those only
>> effect debugging with xhci_dbg with the host device, not dev_dbg with
>> the USB device. This should apply fine to usb-next.
>

> At another glance, the patch removes two if blocks, but doesn't
> re-indent the rest of the lines:
>

>> @@ -3060,8 +3060,7 @@ 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"
>> + dev_dbg_ratelimited(&urb->dev->dev, "Driver uses different interval"
>> " (%d microframe%s) than xHCI "
>> "(%d microframe%s)\n",
>> ep_interval,
>

> That should probably be fixed.

It actually looks correct when patch is applied.

But it depends what you mean of course.
It looks like it was before:

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",

Or may be you mean:

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",

Thanks,
Dmitry

[Sarah Sharp]

On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
> On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
> > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> > > ---
> > > drivers/usb/host/xhci-ring.c | 6 ++----
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >
> > Sarah, does this patch conflict with the trace debug patches being
> > worked on? I'll hold off on applying it for now, let me know if it's ok
> > or not.
>
> It doesn't conflict with the trace debug patches, because those only
> effect debugging with xhci_dbg with the host device, not dev_dbg with
> the USB device. This should apply fine to usb-next.

At another glance, the patch removes two if blocks, but doesn't
re-indent the rest of the lines:

> @@ -3060,8 +3060,7 @@ 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"
> + dev_dbg_ratelimited(&urb->dev->dev, "Driver uses different interval"
> " (%d microframe%s) than xHCI "
> "(%d microframe%s)\n",
> ep_interval,

That should probably be fixed.

[Greg KH]

On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
> On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
> > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
> > > ---
> > > drivers/usb/host/xhci-ring.c | 6 ++----
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >
> > Sarah, does this patch conflict with the trace debug patches being
> > worked on? I'll hold off on applying it for now, let me know if it's ok
> > or not.
>
> It doesn't conflict with the trace debug patches, because those only
> effect debugging with xhci_dbg with the host device, not dev_dbg with
> the USB device. This should apply fine to usb-next.

Ok, can I get your ack for it so I can apply it?

thanks,

greg k-h

[Dmitry Kasatkin]

On Fri, Aug 16, 2013 at 8:38 PM, Sarah Sharp
<sarah....@linux.intel.com> wrote:
> On Fri, Aug 16, 2013 at 10:30:35AM -0700, Greg KH wrote:
>> On Fri, Aug 16, 2013 at 10:26:35AM -0700, Sarah Sharp wrote:
>> > On Thu, Aug 15, 2013 at 05:17:16PM -0700, Greg KH wrote:
>> > > On Thu, Aug 15, 2013 at 07:04:55PM +0300, Dmitry Kasatkin 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.kas...@samsung.com>
>> > > > ---
>> > > > drivers/usb/host/xhci-ring.c | 6 ++----
>> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
>> > >
>> > >
>> > > Sarah, does this patch conflict with the trace debug patches being
>> > > worked on? I'll hold off on applying it for now, let me know if it's ok
>> > > or not.
>> >
>> > It doesn't conflict with the trace debug patches, because those only
>> > effect debugging with xhci_dbg with the host device, not dev_dbg with
>> > the USB device. This should apply fine to usb-next.
>>
>> Ok, can I get your ack for it so I can apply it?
>
> Dmitry, can you fix the indentation issue and resubmit this? I'll ack
> it then.
>
> Sarah Sharp

Sure.

Thanks
Dmitry

[Greg KH]

No, it should look like:

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",

and the rest of that call indented the same way.

thanks,

greg k-h

[Sarah Sharp]

Or just use the default tabbed indentation in your editor (use '==' if
you're using vim). I don't mind if the second line is indented with
spaces to align with the parenthesis, and the majority of the xHCI
driver uses cindent vim indentation. Either way is fine with me though.

Sarah Sharp

[Dmitry Kasatkin]

Hello. Sorry I was distracted so much from the kernel.

But putting string to one line make it much over 80 chars.
Is that considered OK?

- Dmitry

[Greg KH]

Yes it is.

[Dmitry Kasatkin]

Ok. I sent PATCHv2 patches couple of hours ago assuming this.

Thanks,
Dmitry


--
Thanks,
Dmitry