| Message ID | 20200627070307.516803-2-drinkcat@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Detect and remove trace_printk | expand |
On 20-06-27 15:03:04, Nicolas Boichat wrote: > trace_printk should not be used in production code, replace it > call with dev_dbg. > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > --- > > Unclear why a trace_printk was used in the first place, it's > possible that some rate-limiting is necessary here. > > drivers/usb/cdns3/gadget.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c > index 5e24c2e57c0d8c8..c303ab7c62d1651 100644 > --- a/drivers/usb/cdns3/gadget.c > +++ b/drivers/usb/cdns3/gadget.c > @@ -421,7 +421,7 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev, > if ((priv_req->flags & REQUEST_INTERNAL) || > (priv_ep->flags & EP_TDLCHK_EN) || > priv_ep->use_streams) { > - trace_printk("Blocking external request\n"); > + dev_dbg(priv_dev->dev, "Blocking external request\n"); > return ret; > } > } > -- Reviewed-by: Peter Chen <peter.chen@nxp.com>
Nicolas Boichat <drinkcat@chromium.org> writes: > trace_printk should not be used in production code, replace it > call with dev_dbg. > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > --- > > Unclear why a trace_printk was used in the first place, it's > possible that some rate-limiting is necessary here. > > drivers/usb/cdns3/gadget.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c > index 5e24c2e57c0d8c8..c303ab7c62d1651 100644 > --- a/drivers/usb/cdns3/gadget.c > +++ b/drivers/usb/cdns3/gadget.c > @@ -421,7 +421,7 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev, > if ((priv_req->flags & REQUEST_INTERNAL) || > (priv_ep->flags & EP_TDLCHK_EN) || > priv_ep->use_streams) { > - trace_printk("Blocking external request\n"); > + dev_dbg(priv_dev->dev, "Blocking external request\n"); Instead, I would suggest adding a proper trace event here; one that includes "priv_ep->flags" in the output.
On Thu, Jul 23, 2020 at 9:17 PM Felipe Balbi <balbi@kernel.org> wrote: > > Nicolas Boichat <drinkcat@chromium.org> writes: > > > trace_printk should not be used in production code, replace it > > call with dev_dbg. > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > --- > > > > Unclear why a trace_printk was used in the first place, it's > > possible that some rate-limiting is necessary here. > > > > drivers/usb/cdns3/gadget.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c > > index 5e24c2e57c0d8c8..c303ab7c62d1651 100644 > > --- a/drivers/usb/cdns3/gadget.c > > +++ b/drivers/usb/cdns3/gadget.c > > @@ -421,7 +421,7 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev, > > if ((priv_req->flags & REQUEST_INTERNAL) || > > (priv_ep->flags & EP_TDLCHK_EN) || > > priv_ep->use_streams) { > > - trace_printk("Blocking external request\n"); > > + dev_dbg(priv_dev->dev, "Blocking external request\n"); > > Instead, I would suggest adding a proper trace event here; one that > includes "priv_ep->flags" in the output. The patch was already merged by Greg (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/cdns3/gadget.c?id=b3a5ce874c2619c9b8a6c5bbcfefdb95e0227600), but feel free to do that as a follow-up CL. Looks like Peter -- the main author, is ok with dev_dbg (also, apologies for missing the R-b tag when I sent a v2 -- which is the one that was merged by Greg). Thanks, > > -- > balbi
Nicolas Boichat <drinkcat@chromium.org> writes: > On Thu, Jul 23, 2020 at 9:17 PM Felipe Balbi <balbi@kernel.org> wrote: >> >> Nicolas Boichat <drinkcat@chromium.org> writes: >> >> > trace_printk should not be used in production code, replace it >> > call with dev_dbg. >> > >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> >> > >> > --- >> > >> > Unclear why a trace_printk was used in the first place, it's >> > possible that some rate-limiting is necessary here. >> > >> > drivers/usb/cdns3/gadget.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c >> > index 5e24c2e57c0d8c8..c303ab7c62d1651 100644 >> > --- a/drivers/usb/cdns3/gadget.c >> > +++ b/drivers/usb/cdns3/gadget.c >> > @@ -421,7 +421,7 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev, >> > if ((priv_req->flags & REQUEST_INTERNAL) || >> > (priv_ep->flags & EP_TDLCHK_EN) || >> > priv_ep->use_streams) { >> > - trace_printk("Blocking external request\n"); >> > + dev_dbg(priv_dev->dev, "Blocking external request\n"); >> >> Instead, I would suggest adding a proper trace event here; one that >> includes "priv_ep->flags" in the output. > > The patch was already merged by Greg > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/cdns3/gadget.c?id=b3a5ce874c2619c9b8a6c5bbcfefdb95e0227600), > but feel free to do that as a follow-up CL. > > Looks like Peter -- the main author, is ok with dev_dbg (also, > apologies for missing the R-b tag when I sent a v2 -- which is the one > that was merged by Greg). That's okay, we can get a proper trace event for v5.10. Maybe Pawel or Roger would like to take the effort?
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index 5e24c2e57c0d8c8..c303ab7c62d1651 100644 --- a/drivers/usb/cdns3/gadget.c +++ b/drivers/usb/cdns3/gadget.c @@ -421,7 +421,7 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev, if ((priv_req->flags & REQUEST_INTERNAL) || (priv_ep->flags & EP_TDLCHK_EN) || priv_ep->use_streams) { - trace_printk("Blocking external request\n"); + dev_dbg(priv_dev->dev, "Blocking external request\n"); return ret; } }
trace_printk should not be used in production code, replace it call with dev_dbg. Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> --- Unclear why a trace_printk was used in the first place, it's possible that some rate-limiting is necessary here. drivers/usb/cdns3/gadget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)