On 31-Mar-19 20:23, Leon Romanovsky wrote:
> 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?
It doesn't produce a warning, even if CONFIG_INFINIBAND is not set.
>
>> +
>> #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)"?
I was wondering as well :).
This is copied from the netdev_dbg counterpart, my best guess is some kind of
compilation checks in case debug is disabled but I can't even convince myself..
>
>> + 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
>>
