| Message ID | 1555330841-7904-2-git-send-email-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/efa: Elastic Fabric Adapter (EFA) driver | expand |
On 4/15/2019 8:20 AM, Gal Pressman wrote: > 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); I don't think BUG_ON is going to be acceptable. -Denny
On 16-Apr-19 19:49, Dennis Dalessandro wrote: > On 4/15/2019 8:20 AM, Gal Pressman wrote: >> 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); > > I don't think BUG_ON is going to be acceptable. These are the exact same BUG_ON's that are present in __dynamic_pr_dbg, __dynamic_dev_dbg and __dynamic_netdev_dbg. I prefer the ibdev variation to be consistent with its counterparts. Jason, do you prefer to remove them?
On Wed, Apr 17, 2019 at 09:41:11AM +0300, Gal Pressman wrote: > On 16-Apr-19 19:49, Dennis Dalessandro wrote: > > On 4/15/2019 8:20 AM, Gal Pressman wrote: > >> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > >> index 7bdf98c37e91..dfcf6cfa1c70 100644 > >> +++ 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); > > > > I don't think BUG_ON is going to be acceptable. > > These are the exact same BUG_ON's that are present in __dynamic_pr_dbg, > __dynamic_dev_dbg and __dynamic_netdev_dbg. > I prefer the ibdev variation to be consistent with its counterparts. > Jason, do you prefer to remove them? Linus has yelled at RDMA for adding them, so if this goes through the rdma tree (which I prefer) then it must be removed, or someone who owns that file must stand up to say they are needed.. Jason
On 17-Apr-19 13:12, Jason Gunthorpe wrote: > On Wed, Apr 17, 2019 at 09:41:11AM +0300, Gal Pressman wrote: >> On 16-Apr-19 19:49, Dennis Dalessandro wrote: >>> On 4/15/2019 8:20 AM, Gal Pressman wrote: >>>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c >>>> index 7bdf98c37e91..dfcf6cfa1c70 100644 >>>> +++ 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); >>> >>> I don't think BUG_ON is going to be acceptable. >> >> These are the exact same BUG_ON's that are present in __dynamic_pr_dbg, >> __dynamic_dev_dbg and __dynamic_netdev_dbg. >> I prefer the ibdev variation to be consistent with its counterparts. >> Jason, do you prefer to remove them? > > Linus has yelled at RDMA for adding them, so if this goes through the > rdma tree (which I prefer) then it must be removed, or someone who > owns that file must stand up to say they are needed.. Jason Baron (CC'd) is listed as the dynamic debug maintainer, unless he objects I'll remove it.
On Mon, Apr 15, 2019 at 03:20:30PM +0300, 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@akamai.com> > Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> > Suggested-by: Leon Romanovsky <leon@kernel.org> > Signed-off-by: Gal Pressman <galpress@amazon.com> > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++ > include/linux/dynamic_debug.h | 11 ++++++++ > include/rdma/ib_verbs.h | 30 ++++++++++++++++++++ > lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++ > 4 files changed, 141 insertions(+) I can remove the two BUG_ONs if we get an ack from the dyanmic_debug maintainer.. Jason Baron? Thanks, Jason
On 22-Apr-19 19:32, Jason Gunthorpe wrote: > On Mon, Apr 15, 2019 at 03:20:30PM +0300, 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@akamai.com> >> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >> Suggested-by: Leon Romanovsky <leon@kernel.org> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> Reviewed-by: Leon Romanovsky <leonro@mellanox.com> >> --- >> drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/dynamic_debug.h | 11 ++++++++ >> include/rdma/ib_verbs.h | 30 ++++++++++++++++++++ >> lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++ >> 4 files changed, 141 insertions(+) > > I can remove the two BUG_ONs if we get an ack from the dyanmic_debug > maintainer.. > > Jason Baron? I thought we agreed to remove the BUG_ONs unless he objects?
On Mon, Apr 22, 2019 at 08:53:55PM +0300, Gal Pressman wrote: > On 22-Apr-19 19:32, Jason Gunthorpe wrote: > > On Mon, Apr 15, 2019 at 03:20:30PM +0300, 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@akamai.com> > >> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> > >> Suggested-by: Leon Romanovsky <leon@kernel.org> > >> Signed-off-by: Gal Pressman <galpress@amazon.com> > >> Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > >> drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++ > >> include/linux/dynamic_debug.h | 11 ++++++++ > >> include/rdma/ib_verbs.h | 30 ++++++++++++++++++++ > >> lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++ > >> 4 files changed, 141 insertions(+) > > > > I can remove the two BUG_ONs if we get an ack from the dyanmic_debug > > maintainer.. > > > > Jason Baron? > > I thought we agreed to remove the BUG_ONs unless he objects? Regardless of the BUG_ONs, it still should have an ack from the maintainer who's file we are changing if it goes through the rdma tree Jason
>Subject: [PATCH rdma-next v5 01/12] RDMA/core: Introduce RDMA subsystem >ibdev_* print functions > >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@akamai.com> >Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >Suggested-by: Leon Romanovsky <leon@kernel.org> >Signed-off-by: Gal Pressman <galpress@amazon.com> >Reviewed-by: Leon Romanovsky <leonro@mellanox.com> >--- Looks ok to me. Reviewed-by: Shiraz Saleem <shiraz.saleem@intel.com>
On 4/22/2019 12:32 PM, Jason Gunthorpe wrote: > On Mon, Apr 15, 2019 at 03:20:30PM +0300, 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@akamai.com> >> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >> Suggested-by: Leon Romanovsky <leon@kernel.org> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> Reviewed-by: Leon Romanovsky <leonro@mellanox.com> >> --- >> drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/dynamic_debug.h | 11 ++++++++ >> include/rdma/ib_verbs.h | 30 ++++++++++++++++++++ >> lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++ >> 4 files changed, 141 insertions(+) > > I can remove the two BUG_ONs if we get an ack from the dyanmic_debug > maintainer.. > > Jason Baron? Assuming that's what happens... Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
On 22-Apr-19 21:01, Jason Gunthorpe wrote: > On Mon, Apr 22, 2019 at 08:53:55PM +0300, Gal Pressman wrote: >> On 22-Apr-19 19:32, Jason Gunthorpe wrote: >>> On Mon, Apr 15, 2019 at 03:20:30PM +0300, 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@akamai.com> >>>> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >>>> Suggested-by: Leon Romanovsky <leon@kernel.org> >>>> Signed-off-by: Gal Pressman <galpress@amazon.com> >>>> Reviewed-by: Leon Romanovsky <leonro@mellanox.com> >>>> drivers/infiniband/core/device.c | 60 ++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/dynamic_debug.h | 11 ++++++++ >>>> include/rdma/ib_verbs.h | 30 ++++++++++++++++++++ >>>> lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++ >>>> 4 files changed, 141 insertions(+) >>> >>> I can remove the two BUG_ONs if we get an ack from the dyanmic_debug >>> maintainer.. >>> >>> Jason Baron? >> >> I thought we agreed to remove the BUG_ONs unless he objects? > > Regardless of the BUG_ONs, it still should have an ack from the > maintainer who's file we are changing if it goes through the rdma tree It's been quite some time with no response, can we proceed with this patch? I can remove the dynamic debug part if that's what's holding us back (although I prefer not).
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 88c4238bbee1..8dd80b4c4276 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -197,6 +197,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, ...); + #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 43a75ab8ea8a..7fb6090dbdd1 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -74,6 +74,36 @@ 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...) \ + dynamic_ibdev_dbg(__dev, format, ##args) +#elif defined(DEBUG) +#define ibdev_dbg(__dev, format, args...) \ + ibdev_printk(KERN_DEBUG, __dev, format, ##args) +#else +__printf(2, 3) __cold +static inline +void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {} +#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];