On Thu, Mar 28, 2019 at 02:39:21PM +0200, Gal Pressman wrote:
> Similarly to dev/netdev/etc printk helpers, add standard printk helpers
> for the RDMA subsystem.
>
> Example output:
> efa 0000:00:06.0 efa_0: Hello World!
> efa_0: Hello World! (no parent device set)
> (NULL ib_device): Hello World! (ibdev is NULL)
>
> Cc: Jason Baron <jbaron@xxxxxxxxxx>
> Signed-off-by: Gal Pressman <galpress@xxxxxxxxxx>
Thanks Gal,
Overall looks very good.
> ---
> drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++
> include/linux/dynamic_debug.h | 11 ++++++++
> include/rdma/ib_verbs.h | 34 +++++++++++++++++++++++
> lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++
> 4 files changed, 145 insertions(+)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 7421ec4883fb..a300218d3f99 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -147,6 +147,66 @@ static int ib_security_change(struct notifier_block *nb, unsigned long event,
> static void ib_policy_change_task(struct work_struct *work);
> static DECLARE_WORK(ib_policy_change_work, ib_policy_change_task);
>
> +static void __ibdev_printk(const char *level, const struct ib_device *ibdev,
> + struct va_format *vaf)
> +{
> + if (ibdev && ibdev->dev.parent)
> + dev_printk_emit(level[1] - '0',
> + ibdev->dev.parent,
> + "%s %s %s: %pV",
> + dev_driver_string(ibdev->dev.parent),
> + dev_name(ibdev->dev.parent),
> + dev_name(&ibdev->dev),
> + vaf);
> + else if (ibdev)
> + printk("%s%s: %pV",
> + level, dev_name(&ibdev->dev), vaf);
> + else
> + printk("%s(NULL ib_device): %pV", level, vaf);
> +}
> +
> +void ibdev_printk(const char *level, const struct ib_device *ibdev,
> + const char *format, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + va_start(args, format);
> +
> + vaf.fmt = format;
> + vaf.va = &args;
> +
> + __ibdev_printk(level, ibdev, &vaf);
> +
> + va_end(args);
> +}
> +EXPORT_SYMBOL(ibdev_printk);
> +
> +#define define_ibdev_printk_level(func, level) \
> +void func(const struct ib_device *ibdev, const char *fmt, ...) \
> +{ \
> + struct va_format vaf; \
> + va_list args; \
> + \
> + va_start(args, fmt); \
> + \
> + vaf.fmt = fmt; \
> + vaf.va = &args; \
> + \
> + __ibdev_printk(level, ibdev, &vaf); \
> + \
> + va_end(args); \
> +} \
> +EXPORT_SYMBOL(func);
> +
> +define_ibdev_printk_level(ibdev_emerg, KERN_EMERG);
> +define_ibdev_printk_level(ibdev_alert, KERN_ALERT);
> +define_ibdev_printk_level(ibdev_crit, KERN_CRIT);
> +define_ibdev_printk_level(ibdev_err, KERN_ERR);
> +define_ibdev_printk_level(ibdev_warn, KERN_WARNING);
> +define_ibdev_printk_level(ibdev_notice, KERN_NOTICE);
> +define_ibdev_printk_level(ibdev_info, KERN_INFO);
> +
> static struct notifier_block ibdev_lsm_nb = {
> .notifier_call = ib_security_change,
> };
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index c2be029b9b53..6c809440f319 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -71,6 +71,13 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
> const struct net_device *dev,
> const char *fmt, ...);
>
> +struct ib_device;
> +
> +extern __printf(3, 4)
> +void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
> + const struct ib_device *ibdev,
> + const char *fmt, ...);
You guarded the implementation of this function with
"CONFIG_INFINIBAND", isn't the empty declaration produced
compilation warnings about missing implementation?
> +
> #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
> static struct _ddebug __aligned(8) \
> __attribute__((section("__verbose"))) name = { \
> @@ -154,6 +161,10 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
> _dynamic_func_call(fmt, __dynamic_netdev_dbg, \
> dev, fmt, ##__VA_ARGS__)
>
> +#define dynamic_ibdev_dbg(dev, fmt, ...) \
> + _dynamic_func_call(fmt, __dynamic_ibdev_dbg, \
> + dev, fmt, ##__VA_ARGS__)
> +
> #define dynamic_hex_dump(prefix_str, prefix_type, rowsize, \
> groupsize, buf, len, ascii) \
> _dynamic_func_call_no_desc(__builtin_constant_p(prefix_str) ? prefix_str : "hexdump", \
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 9b9e17bcc201..f796e2d44425 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -72,6 +72,40 @@ extern struct workqueue_struct *ib_wq;
> extern struct workqueue_struct *ib_comp_wq;
> extern struct workqueue_struct *ib_comp_unbound_wq;
>
> +__printf(3, 4) __cold
> +void ibdev_printk(const char *level, const struct ib_device *ibdev,
> + const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_emerg(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_alert(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_crit(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_err(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_warn(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_notice(const struct ib_device *ibdev, const char *format, ...);
> +__printf(2, 3) __cold
> +void ibdev_info(const struct ib_device *ibdev, const char *format, ...);
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> +#define ibdev_dbg(__dev, format, args...) \
> +do { \
> + dynamic_ibdev_dbg(__dev, format, ##args); \
> +} while (0)
> +#elif defined(DEBUG)
> +#define ibdev_dbg(__dev, format, args...) \
> + ibdev_printk(KERN_DEBUG, __dev, format, ##args)
> +#else
> +#define ibdev_dbg(__dev, format, args...) \
> +({ \
> + if (0) \
Why isn't "empty macro" enough here and we need "if (0)"?
> + ibdev_printk(KERN_DEBUG, __dev, format, ##args); \
> +})
> +#endif
> +
> union ib_gid {
> u8 raw[16];
> struct {
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 7bdf98c37e91..dfcf6cfa1c70 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -37,6 +37,8 @@
> #include <linux/device.h>
> #include <linux/netdevice.h>
>
> +#include <rdma/ib_verbs.h>
> +
> extern struct _ddebug __start___verbose[];
> extern struct _ddebug __stop___verbose[];
>
> @@ -636,6 +638,44 @@ EXPORT_SYMBOL(__dynamic_netdev_dbg);
>
> #endif
>
> +#if IS_ENABLED(CONFIG_INFINIBAND)
> +
> +void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
> + const struct ib_device *ibdev, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + BUG_ON(!descriptor);
> + BUG_ON(!fmt);
> +
> + va_start(args, fmt);
> +
> + vaf.fmt = fmt;
> + vaf.va = &args;
> +
> + if (ibdev && ibdev->dev.parent) {
> + char buf[PREFIX_SIZE];
> +
> + dev_printk_emit(LOGLEVEL_DEBUG, ibdev->dev.parent,
> + "%s%s %s %s: %pV",
> + dynamic_emit_prefix(descriptor, buf),
> + dev_driver_string(ibdev->dev.parent),
> + dev_name(ibdev->dev.parent),
> + dev_name(&ibdev->dev),
> + &vaf);
> + } else if (ibdev) {
> + printk(KERN_DEBUG "%s: %pV", dev_name(&ibdev->dev), &vaf);
> + } else {
> + printk(KERN_DEBUG "(NULL ib_device): %pV", &vaf);
> + }
> +
> + va_end(args);
> +}
> +EXPORT_SYMBOL(__dynamic_ibdev_dbg);
> +
> +#endif
> +
> #define DDEBUG_STRING_SIZE 1024
> static __initdata char ddebug_setup_string[DDEBUG_STRING_SIZE];
>
> --
> 2.7.4
>
Attachment:
signature.asc
Description: PGP signature
