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 <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Cc: [email protected]
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
On 15/08/13 19:37, Greg KH wrote:
> 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 <[email protected]>
>> Cc: [email protected]
> 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
>
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
On Thu, Aug 15, 2013 at 07:53:13PM +0300, Dmitry Kasatkin wrote:
> On 15/08/13 19:37, Greg KH wrote:
> > 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 <[email protected]>
> >> Cc: [email protected]
> > 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
> >
>
> 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.
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
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 <[email protected]>
> ---
> 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
>
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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.
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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
On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
>> > > ---
>> > > 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: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 <[email protected]>
> > > > ---
> > > > 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
On Fri, Aug 16, 2013 at 8:38 PM, Sarah Sharp
<[email protected]> 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 <[email protected]>
>> > > > ---
>> > > > 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
On Fri, Aug 16, 2013 at 08:38:12PM +0300, Dmitry Kasatkin wrote:
> On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
> >> > > ---
> >> > > 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",
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
On Fri, Aug 16, 2013 at 10:45:16AM -0700, Greg KH wrote:
> On Fri, Aug 16, 2013 at 08:38:12PM +0300, Dmitry Kasatkin wrote:
> > On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
> > >> > > ---
> > >> > > 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",
>
> 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.
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
On 16/08/13 20:45, Greg KH wrote:
> On Fri, Aug 16, 2013 at 08:38:12PM +0300, Dmitry Kasatkin wrote:
>> On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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",
> 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",
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
> and the rest of that call indented the same way.
>
> thanks,
>
> greg k-h
>
On Tue, Aug 27, 2013 at 05:16:37PM +0300, Dmitry Kasatkin wrote:
> On 16/08/13 20:45, Greg KH wrote:
> > On Fri, Aug 16, 2013 at 08:38:12PM +0300, Dmitry Kasatkin wrote:
> >> On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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",
> > 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",
>
> 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?
Yes it is.
On Tue, Aug 27, 2013 at 8:39 PM, Greg KH <[email protected]> wrote:
> On Tue, Aug 27, 2013 at 05:16:37PM +0300, Dmitry Kasatkin wrote:
>> On 16/08/13 20:45, Greg KH wrote:
>> > On Fri, Aug 16, 2013 at 08:38:12PM +0300, Dmitry Kasatkin wrote:
>> >> On Fri, Aug 16, 2013 at 8:30 PM, Sarah Sharp <[email protected]> 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 <[email protected]>
>> >>>>>> ---
>> >>>>>> 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",
>> > 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",
>>
>> 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?
>
> Yes it is.
>
Ok. I sent PATCHv2 patches couple of hours ago assuming this.
Thanks,
Dmitry
--
Thanks,
Dmitry