Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

From: Joe Perches <joe@perches.com>
To: Jason Baron <jbaron@akamai.com>,
	jim.cromie@gmail.com,
	Daniel Thompson <daniel.thompson@linaro.org>
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Documentation List <linux-doc@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-btrfs@vger.kernel.org, linux-acpi@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible
Date: Thu, 11 Jun 2020 15:33:45 -0700	[thread overview]
Message-ID: <3518483f1836bdfbc193292dc1639509ac33fe7c.camel@perches.com> (raw)
In-Reply-To: <e65d2c81-6d0b-3c1e-582c-56d707c0d1f1@akamai.com>

On Thu, 2020-06-11 at 17:59 -0400, Jason Baron wrote:
> 
> On 6/11/20 5:19 PM, jim.cromie@gmail.com wrote:
> > trimmed..
> > 
> > > > > Currently I think there not enough "levels" to map something like
> > > > > drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> > > > > but I couldn't find the bit of the code where the 5-bit level in struct
> > > > > _ddebug is converted from a mask to a bit number and vice-versa.
> > > > 
> > > > Here [1] is Joe's initial suggestion. But I decided that bitmask is a
> > > > good start for the discussion.
> > > > 
> > > > I guess we can add new member uint "level" in struct _ddebug so that we
> > > > can cover more "levels" (types, groups).
> > > 
> > > I don't think it is allocating only 5 bits that is the problem!

There were 6 unused bits in struct _ddebug;

The original idea was to avoid expanding the already somewhat
large struct _ddebug uses and the __verbose/__dyndbg section
that can have quite a lot of these structs.

I imagine adding another int or long wouldn't be too bad.

> > > The problem is that those 5 bits need not be encoded as a bitmask by
> > > dyndbg, that can simply be the category code for the message. They only
> > > need be converted into a mask when we compare them to the mask provided
> > > by the user.
> > > 

I also suggested adding a pointer to whatever is provided
by the developer so the address of something like
MODULE_PARM_DESC(variable, ...) can be also be used.

> > heres what I have in mind.  whats described here is working.
> > I'll send it out soon
> 
> Cool. thanks for working on this!

Truly, thank you both Jim and Stanimir.

Please remember that dynamic_debug is not required and
pr_debug should still work.

> >     API:
> > 
> >     - change pr_debug(...)  -->  pr_debug_typed(type_id=0, ...)
> >     - all existing uses have type_id=0
> >     - developer creates exclusive types of log messages with type_id>0
> >       1, 2, 3 are disjoint groups, for example: hi, mid, low

You could have a u8 for type if there are to be 3 classes

	bitmask
	level
	group by value

though I believe group by value might as well just be bitmask
and bool is_bitmask is enough (!is_bitmask would be level)

cheers, Joe