linux-kernel - Re: [PATCH v9 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE

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] [day] [month] [year] [list]

Date:   Thu, 4 Nov 2021 00:31:58 -0600
From:   jim.cromie@...il.com
To:     Jason Baron <jbaron@...mai.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Sean Paul <seanpaul@...omium.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>
Subject: Re: [PATCH v9 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES
 bitmap to tracefs

On Wed, Nov 3, 2021 at 9:58 AM Jason Baron <jbaron@...mai.com> wrote:
>
>
>
> On 10/27/21 12:36 AM, Jim Cromie wrote:
> > Use new macro to create a sysfs control bitmap knob to control
> > print-to-trace in: /sys/module/drm/parameters/trace
> >
> > todo: reconsider this api, ie a single macro expecting both debug &
> > trace terms (2 each), followed by a single description and the
> > bitmap-spec::
> >
> > Good: declares bitmap once for both interfaces
> >
> > Bad: arg-type/count handling (expecting 4 args) is ugly,
> >      especially preceding the bitmap-init var-args.
> >
>
> Hi Jim,
>
> I agree having the bitmap declared twice seems redundant. But I like having fewer args and not necessarily combining the trace/log variants of
> DEBUG_CATEGORIES. hmmm...what if the DEFINE_DYNAMIC_DEBUG_CATEGORIES() took a pointer to the array of struct dyndbg_bitdesc map[] directly as the
> final argument instead of the __VA_ARGS__? Then, we could just declare the map once?
>

indeed. that seems obvious in retrospect,
thanks for the nudge.

also, Im inclined to (uhm, have now done) bikeshed the API in patch 1,
and  change _CATEGORIES to something else,
maybe  _FMTGRPS
or  _BITGRPS  < -- this one

ISTM better to be explicit wrt the underlying mechanisms, (least surprise)
let users decide the meaning of "CATEGORIES"

also, HEAD~1  added DEFINE_DYNAMIC_DEBUG_CATEGORIES_FLAGS
which could be used directly for both purposes (after a rename).
TLDR: flags exposes the shared nature of the decorator flags,
the trace and syslog customers of pr_debug traffic should agree on their use.

redoing now...




> Thanks,
>
> -Jason
>
> > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> > ---
> >  drivers/gpu/drm/drm_print.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index ce662d0f339b..7b49fbc5e21d 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -73,6 +73,25 @@ DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,


static mumble-map
> >                               [7] = { DRM_DBG_CAT_LEASE },
> >                               [8] = { DRM_DBG_CAT_DP },
> >                               [9] = { DRM_DBG_CAT_DRMRES });
> > +
> > +#ifdef CONFIG_TRACING
> > +unsigned long __drm_trace;
> > +EXPORT_SYMBOL(__drm_trace);
> > +DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES(trace, __drm_trace,
> > +                                   DRM_DEBUG_DESC,

                                mumble-map)