| Message ID | 1513680256-8153-1-git-send-email-bharat@chelsio.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Dec 19, 2017 at 04:14:16PM +0530, Potnuri Bharat Teja wrote: > @@ -64,12 +64,6 @@ > #define DRV_NAME "iw_cxgb4" > #define MOD DRV_NAME ":" > > -#ifdef pr_fmt > -#undef pr_fmt > -#endif > - > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > - Lots of other places in RDMA set a prefix, can we come to some agreement if we should drop or keep this unverisally? drivers/infiniband/core/mad.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt drivers/infiniband/core/netlink.c:#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__ drivers/infiniband/core/user_mad.c:#define pr_fmt(fmt) "user_mad: " fmt drivers/infiniband/hw/cxgb3/cxio_hal.h:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt drivers/infiniband/hw/cxgb4/iw_cxgb4.h:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt drivers/infiniband/hw/hfi1/driver.c:#define pr_fmt(fmt) DRIVER_NAME ": " fmt drivers/infiniband/hw/hfi1/file_ops.c:#define pr_fmt(fmt) DRIVER_NAME ": " fmt drivers/infiniband/hw/hfi1/init.c:#define pr_fmt(fmt) DRIVER_NAME ": " fmt drivers/infiniband/hw/mlx4/mlx4_ib.h:#define pr_fmt(fmt) "<" MLX4_IB_DRV_NAME "> %s: " fmt, __func__ drivers/infiniband/hw/qib/qib_diag.c:#define pr_fmt(fmt) QIB_DRV_NAME ": " fmt drivers/infiniband/hw/qib/qib_file_ops.c:#define pr_fmt(fmt) QIB_DRV_NAME ": " fmt drivers/infiniband/hw/qib/qib_iba7322.c:#define pr_fmt(fmt) QIB_DRV_NAME " " fmt drivers/infiniband/hw/qib/qib_init.c:#define pr_fmt(fmt) QIB_DRV_NAME ": " fmt drivers/infiniband/sw/rxe/rxe.h:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt drivers/infiniband/ulp/srp/ib_srp.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt drivers/infiniband/ulp/srpt/ib_srpt.c:#define pr_fmt(fmt) DRV_NAME " " fmt -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Dec 19, 2017 at 04:14:16PM +0530, Potnuri Bharat Teja wrote: > > @@ -64,12 +64,6 @@ > > #define DRV_NAME "iw_cxgb4" > > #define MOD DRV_NAME ":" > > > > -#ifdef pr_fmt > > -#undef pr_fmt > > -#endif > > - > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > - > > Lots of other places in RDMA set a prefix, can we come to some agreement if > we > should drop or keep this unverisally? Either way we go, pr_fmt being redefined screws up pr_debug() which optionally prepends the function, line, module names depending on the dynamic debug settings. You end up with the module name embedded in the middle of the log statement, and duplicated with +m is used. That is why Bharat removed it from iw_cxgb4. In general, I like module names in log statements, but we just need to do it the right way to avoid the pr_debug issue... steve --- This email has been checked for viruses by AVG. http://www.avg.com -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 19, 2017 at 03:12:01PM -0600, Steve Wise wrote: > > On Tue, Dec 19, 2017 at 04:14:16PM +0530, Potnuri Bharat Teja wrote: > > > @@ -64,12 +64,6 @@ > > > #define DRV_NAME "iw_cxgb4" > > > #define MOD DRV_NAME ":" > > > > > > -#ifdef pr_fmt > > > -#undef pr_fmt > > > -#endif > > > - > > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > - > > > > Lots of other places in RDMA set a prefix, can we come to some agreement > if > > we > > should drop or keep this unverisally? > > Either way we go, pr_fmt being redefined screws up pr_debug() which > optionally prepends the function, line, module names depending on the > dynamic debug settings. You end up with the module name embedded in the > middle of the log statement, and duplicated with +m is used. That is why > Bharat removed it from iw_cxgb4. > > In general, I like module names in log statements, but we just need to do it > the right way to avoid the pr_debug issue... Seems like a bigger problem than for linux-rdma? Shouldn't many of these prints be using dev_* stuff and be prefixed with their device name? Usually pr should only be used in places where the device does not yet exist. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On Tue, Dec 19, 2017 at 03:12:01PM -0600, Steve Wise wrote: > > > On Tue, Dec 19, 2017 at 04:14:16PM +0530, Potnuri Bharat Teja wrote: > > > > @@ -64,12 +64,6 @@ > > > > #define DRV_NAME "iw_cxgb4" > > > > #define MOD DRV_NAME ":" > > > > > > > > -#ifdef pr_fmt > > > > -#undef pr_fmt > > > > -#endif > > > > - > > > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > - > > > > > > Lots of other places in RDMA set a prefix, can we come to some agreement > > if > > > we > > > should drop or keep this unverisally? > > > > Either way we go, pr_fmt being redefined screws up pr_debug() which > > optionally prepends the function, line, module names depending on the > > dynamic debug settings. You end up with the module name embedded in the > > middle of the log statement, and duplicated with +m is used. That is why > > Bharat removed it from iw_cxgb4. > > > > In general, I like module names in log statements, but we just need to do it > > the right way to avoid the pr_debug issue... > > Seems like a bigger problem than for linux-rdma? > If redefining pr_fmt is actually the preferred way to add module names, then I think pr_debug() might need to be fixed. > Shouldn't many of these prints be using dev_* stuff and be prefixed > with their device name? Yea, that makes sense. steve --- This email has been checked for viruses by AVG. http://www.avg.com -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 19, 2017 at 04:32:54PM -0600, Steve Wise wrote: > > > In general, I like module names in log statements, but we just > > > need to do it the right way to avoid the pr_debug issue... > > > > Seems like a bigger problem than for linux-rdma? > > > > If redefining pr_fmt is actually the preferred way to add module names, then > I think pr_debug() might need > to be fixed. > > > Shouldn't many of these prints be using dev_* stuff and be prefixed > > with their device name? > > Yea, that makes sense. Okay, I've dropped this patch. The two changes to pr_debug should probably become dev_dbg, and resend? Deal with the 'what should pr_fmt be' question someplace else :) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, December 12/22/17, 2017 at 23:02:07 +0530, Jason Gunthorpe wrote: > On Tue, Dec 19, 2017 at 04:32:54PM -0600, Steve Wise wrote: > > > > > In general, I like module names in log statements, but we just > > > > need to do it the right way to avoid the pr_debug issue... > > > > > > Seems like a bigger problem than for linux-rdma? > > > > > > > If redefining pr_fmt is actually the preferred way to add module names, then > > I think pr_debug() might need > > to be fixed. > > > > > Shouldn't many of these prints be using dev_* stuff and be prefixed > > > with their device name? > > > > Yea, that makes sense. > > Okay, I've dropped this patch. > > The two changes to pr_debug should probably become dev_dbg, and > resend? For now I am resending the patch without pr_fmt change. dev_dbg change would be an overall change if necessary. > > Deal with the 'what should pr_fmt be' question someplace else :) Agreed > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 29, 2017 at 12:13:38PM +0530, Potnuri Bharat Teja wrote: > On Friday, December 12/22/17, 2017 at 23:02:07 +0530, Jason Gunthorpe wrote: > > On Tue, Dec 19, 2017 at 04:32:54PM -0600, Steve Wise wrote: > > > > > > > In general, I like module names in log statements, but we just > > > > > need to do it the right way to avoid the pr_debug issue... > > > > > > > > Seems like a bigger problem than for linux-rdma? > > > > > > > > > > If redefining pr_fmt is actually the preferred way to add module names, then > > > I think pr_debug() might need > > > to be fixed. > > > > > > > Shouldn't many of these prints be using dev_* stuff and be prefixed > > > > with their device name? > > > > > > Yea, that makes sense. > > > > Okay, I've dropped this patch. > > > > The two changes to pr_debug should probably become dev_dbg, and > > resend? > For now I am resending the patch without pr_fmt change. > dev_dbg change would be an overall change if necessary. It is not appropriate for a driver to use pr_*, so you should consider fixing it. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c index 21db3b48a617..eecdfb0c6be6 100644 --- a/drivers/infiniband/hw/cxgb4/cm.c +++ b/drivers/infiniband/hw/cxgb4/cm.c @@ -3567,8 +3567,8 @@ int c4iw_ep_disconnect(struct c4iw_ep *ep, int abrupt, gfp_t gfp) case MORIBUND: case ABORTING: case DEAD: - pr_info("%s ignoring disconnect ep %p state %u\n", - __func__, ep, ep->com.state); + pr_debug("ignoring disconnect ep %p state %u\n", + ep, ep->com.state); break; default: WARN_ONCE(1, "Bad endpoint state %u\n", ep->com.state); diff --git a/drivers/infiniband/hw/cxgb4/ev.c b/drivers/infiniband/hw/cxgb4/ev.c index a252d5c40ae3..3e9d8b277ab9 100644 --- a/drivers/infiniband/hw/cxgb4/ev.c +++ b/drivers/infiniband/hw/cxgb4/ev.c @@ -236,7 +236,7 @@ int c4iw_ev_handler(struct c4iw_dev *dev, u32 qid) if (atomic_dec_and_test(&chp->refcnt)) wake_up(&chp->wait); } else { - pr_warn("%s unknown cqid 0x%x\n", __func__, qid); + pr_debug("unknown cqid 0x%x\n", qid); spin_unlock_irqrestore(&dev->lock, flag); } return 0; diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h index 470f97a79ebb..7d6ec29ec9d7 100644 --- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h @@ -64,12 +64,6 @@ #define DRV_NAME "iw_cxgb4" #define MOD DRV_NAME ":" -#ifdef pr_fmt -#undef pr_fmt -#endif - -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include "t4.h" #define PBL_OFF(rdev_p, a) ((a) - (rdev_p)->lldi.vr->pbl.start)
These prints not neccesarily mean error, so changing them to debug. Redifining pr_fmt to print the kernel module name, prints the module name twice when +m flag of pr_debug is enabled, hence removing the pr_fmt redefinition. Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> --- drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- drivers/infiniband/hw/cxgb4/ev.c | 2 +- drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 6 ------ 3 files changed, 3 insertions(+), 9 deletions(-)