On Thu, Sep 1, 2011 at 7:20 AM, Bob Pearson <[email protected]> wrote: > From: [email protected] >> On Sun, Jul 24, 2011 at 9:43 PM, <[email protected]> wrote: >> > +static int rxe_query_port(struct ib_device *dev, >> > + u8 port_num, struct ib_port_attr *attr) >> > +{ >> > + struct rxe_dev *rxe = to_rdev(dev); >> > + struct rxe_port *port; >> > + >> > + if (unlikely(port_num < 1 || port_num > rxe->num_ports)) { >> > + pr_warn("invalid port_number %d\n", port_num); >> > + goto err1; >> > + } >> > + >> > + port = &rxe->port[port_num - 1]; >> > + >> > + *attr = port->attr; >> > + return 0; >> > + >> > +err1: >> > + return -EINVAL; >> > +} >> >> The pr_warn() statement in the above function looks to me like an >> example of how pr_warn() should not be used: the message it generates >> won't allow a user to find out which subsystem generated that message. >> I suggest either to leave out that pr_warn() statement entirely or to >> convert it into a pr_debug() statement and to use an appropriate >> prefix, e.g. via pr_fmt(). > > I am not familiar with pr_fmt but looked at some examples. It looks like the > conventional usage is to define pr_fmt(fmt) as KBUILD_MODNAME ": " fmt which > I suppose will output ib_rxe: or some variant. Ideally I would want rxe0: or > rxe1: which is the ib device name. > > Do you have an opinion about best use of pr_debug, pr_warn and pr_err and > also about use of something like CONFIG_RXE_DEBUG vs just plain DEBUG vs > always on?
I should have recommended dev_warn()/dev_dbg() instead of pr_fmt() here. These are defined in <linux/device.h>. And if you would like to make enabling debug statements configurable, please use the dynamic_debug infrastructure instead of introducing a configure option like CONFIG_RXE_DEBUG. See also http://lwn.net/Articles/434833/ for more information. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
