linux-kernel - Re: [PATCH v6 23/57] drm: POC drm on dyndbg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 9 Sep 2022 13:06:27 -0600
From:   jim.cromie@...il.com
To:     Jim Cromie <jim.cromie@...il.com>, Jason Baron <jbaron@...mai.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        amd-gfx mailing list <amd-gfx@...ts.freedesktop.org>,
        intel-gvt-dev@...ts.freedesktop.org,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sean Paul <seanpaul@...omium.org>, robdclark@...il.com,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Joe Perches <joe@...ches.com>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH v6 23/57] drm: POC drm on dyndbg - use in core, 2 helpers,
 3 drivers.

On Wed, Sep 7, 2022 at 12:13 AM Daniel Vetter <daniel@...ll.ch> wrote:
>
> On Sun, Sep 04, 2022 at 03:41:00PM -0600, Jim Cromie wrote:
> > Use DECLARE_DYNDBG_CLASSMAP across DRM:
> >
> >  - in .c files, since macro defines/initializes a record
> >
> >  - in drivers, $mod_{drv,drm,param}.c
> >    ie where param setup is done, since a classmap is param related
> >
> >  - in drm/drm_print.c
> >    since existing __drm_debug param is defined there,
> >    and we ifdef it, and provide an elaborated alternative.
> >
> >  - in drm_*_helper modules:
> >    dp/drm_dp - 1st item in makefile target
> >    drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.
> >
> > Since these modules all use identical CLASSMAP declarations (ie: names
> > and .class_id's) they will all respond together to "class DRM_UT_*"
> > query-commands:
> >
> >   :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control
> >
> > NOTES:
> >
> > This changes __drm_debug from int to ulong, so BIT() is usable on it.
> >
> > DRM's enum drm_debug_category values need to sync with the index of
> > their respective class-names here.  Then .class_id == category, and
> > dyndbg's class FOO mechanisms will enable drm_dbg(DRM_UT_KMS, ...).
> >
> > Though DRM needs consistent categories across all modules, thats not
> > generally needed; modules X and Y could define FOO differently (ie a
> > different NAME => class_id mapping), changes are made according to
> > each module's private class-map.
> >
> > No callsites are actually selected by this patch, since none are
> > class'd yet.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
>
> So maybe I should just try, but what happens if a drm module doesn't have
> these classbits declared? You simply have to use the raw number instead?

without the classnames declared via macro,
dyndbg has no names by which to validate the query.
raw class numbers are not usable into >control.
This is what privatizes the module's class-id space.

If the macro is missing, the drm_dbg()s ( after conversion to reside
atop dyndbg)
will do this in `cat control`
                        seq_printf(m, " class unknown, _id:%d", dp->class_id);



>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +++++++++++++
> >  drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++
> >  drivers/gpu/drm/drm_crtc_helper.c       | 13 ++++++++++++
> >  drivers/gpu/drm/drm_print.c             | 27 +++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/i915_params.c      | 12 +++++++++++
> >  drivers/gpu/drm/nouveau/nouveau_drm.c   | 13 ++++++++++++
> >  include/drm/drm_print.h                 |  3 ++-
> >  7 files changed, 92 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index de7144b06e93..97e184f44a52 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -38,6 +38,8 @@
> >  #include <linux/mmu_notifier.h>
> >  #include <linux/suspend.h>
> >  #include <linux/cc_platform.h>
> > +#include <linux/fb.h>
> > +#include <linux/dynamic_debug.h>
> >
> >  #include "amdgpu.h"
> >  #include "amdgpu_irq.h"
> > @@ -185,6 +187,18 @@ int amdgpu_vcnfw_log;
> >
> >  static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);
> >
> > +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
>
> Iirc we've talked about maybe some kbuild trickery so that any module
> under drivers/gpu/drm gets these by default. I don't think we need to have
> this for the first cut, but a macro to avoid the copypaste mistakes would
> be really good here.

It *may be* that theres a perfect place to declare it once, for everyone.
For me thats exploratory, error prone.
Proving that the sub-optimal worked seemed a good place to stop.

that said, theres a macro in test-dynamic-debug that is a candidate
for wider availability - it needs a better name

#define DD_SYS_WRAP(_model, _flags)                                     \
        static unsigned long bits_##_model;                             \
        static struct ddebug_class_param _flags##_model = {             \
                .bits = &bits_##_model,                                 \
                .flags = #_flags,                                       \
                .map = &map_##_model,                                   \
        };                                                              \
        module_param_cb(_flags##_##_model, &param_ops_dyndbg_classes,
&_flags##_model, 0600)