linux-kernel - Re: [PATCH v4 2/3] venus: Make debug infrastructure more flexible

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]

Message-ID: <0f53eb20-6f52-e76e-3957-f7af38b88d35@linaro.org>
Date:   Tue, 16 Jun 2020 14:23:44 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 2/3] venus: Make debug infrastructure more flexible

Hi Greg,

On 6/15/20 3:03 PM, Greg KH wrote:
> On Mon, Jun 15, 2020 at 12:55:29PM +0300, Stanimir Varbanov wrote:
>> Hi Greg,
>>
>> On 6/14/20 9:37 AM, Greg KH wrote:
>>> On Sun, Jun 14, 2020 at 01:39:18AM +0300, Stanimir Varbanov wrote:
>>>>  	if (slot == -1) {
>>>> -		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
>>>> +		VDBGH("no free slot for timestamp\n");
>>>
>>> Again, no, you just lost a lot of valuable information by changing to a
>>> different format (like driver, specific device, etc.).  Please don't do
>>> this, it just makes the information less than before.
>>
>> OK, one of the reasons to use pr_debug inside VDBGH macro is to avoid
>> having struct device *dev variable in every function with dev_dbg even
>> when the function doesn't use it.
> 
> But the function _is_ using it, as you are referring to the device that
> is being controlled by the driver.  That's the point, you are stripping
> off that very valuable information for no git grep dev_dbg | wc -lreason.
> 
> Which means to me that you never really actually _NEED_ these debugging
> messages, as you have not used them to see if it provides you with
> something that can tell you something about something.
> 
> So, let me push harder, why do you even want this message at all?  What
> can it provide you now that the driver is up and working properly?

I will delete that message.

> 
>> Are you fine with s/pr_debug/dev_dbg in VDBGX macros?
> 
> I would be a bit happier yes, but the fact that you didn't use it means
> you aren't even looking at these messages, which implies that it isn't
> even needed.
> 
> So, how about just stripping all of these debugging messages out

I'm not sure for which messages you are talking. The messages added by
this patch or the messages which currently exist?

> entirely?  What do they provide that you don't already know?  Who would
> use them?

Presently in 5.8-rc1 debug messages count for similar (encoder/decoder)
drivers compared with Venus one:

Venus
$git grep dev_dbg | wc -l
15

Coda
$git grep coda_dbg | wc -l
56

Mtk-vcodec
$git grep mtk_v4l2_debug | wc -l
95

Mfc
$git grep mfc_debug | wc -l
227

As you can see Venus driver is the one with smallest count of debug
messages. It is smallest because I also don't want to overload the code
with so many debugs and thus make it unreadable.

I personally don't need so much debug messages. I can add them to debug
some particular issue and drop them before sending the fix. But now when
the driver is going to be used more widely I've been asked to "improve"
debug infrastructure. That will help to unfamiliar with the driver
persons to enable debug messages and send bug reports to help them to
diagnose the problem.

What messages are needed and where is a subjective question. I'm relying
on my experience with the driver and issues I had previously.

> 
> thanks,
> 
> greg k-h
> 

-- 
regards,
Stan