linux-kernel - Re: [PATCH 1/2] soundwire: add macro to selectively change error levels

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]

Message-ID: <28515962-6fb1-511d-fc6b-f1422b11e6ab@linux.intel.com>
Date:   Thu, 1 Apr 2021 13:43:53 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Vinod Koul <vkoul@...nel.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        hui.wang@...onical.com, sanyog.r.kale@...el.com,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH 1/2] soundwire: add macro to selectively change error
 levels


>>> My bigger issue with this is that this macro is crazy.  Why do you need
>>> debugging here at all for this type of thing?  That's what ftrace is
>>> for, do not sprinkle code with "we got this return value from here!" all
>>> over the place like what this does.
>>
>> We are not sprinkling the code all over the place with any new logs, they
>> exist already in the SoundWire code and this patch helps filter them out.
>> See e.g. patch 2/2
>>
>> -			dev_err(&slave->dev,
>> -				"Clk Stop type =%d failed: %d\n", type, ret);
>> +			sdw_dev_dbg_or_err(&slave->dev, ret != -ENODATA,
>> +					   "Clk Stop mode %d type =%d failed: %d\n",
>> +					   mode, type, ret);
> 
> You just added a debug log for no reason.

The number of logs is lower when dynamic debug is not enabled, and equal 
when it is. there's no addition.

The previous behavior was unconditional dev_err that everyone sees.

Now it's dev_err ONLY when the code is NOT -ENODATA, and dev_dgb 
otherwise, meaning it will seen ONLY be seen IF dynamic debug is enabled 
for drivers/soundwire/bus.c

Allow me to use another example from patch2:

-		if (ret == -ENODATA)
-			dev_dbg(bus->dev,
-				"ClockStopNow Broadcast msg ignored %d", ret);
-		else
-			dev_err(bus->dev,
-				"ClockStopNow Broadcast msg failed %d", ret);
+		sdw_dev_dbg_or_err(bus->dev, ret != -ENODATA,
+				   "ClockStopNow Broadcast msg failed %d\n", ret);

There's no new log, is there?

If that still gives you a heartburn, I would still like a macro that 
filters out dev_err so that we don't report an error when it's 
recoverable or harmless, and don't have spaghetti code as above.