From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Vinod Koul <vkoul@kernel.org>,
Bard Liao <yung-chuan.liao@linux.intel.com>,
alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
hui.wang@canonical.com, sanyog.r.kale@intel.com,
rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
Date: Thu, 1 Apr 2021 13:43:53 -0500 [thread overview]
Message-ID: <28515962-6fb1-511d-fc6b-f1422b11e6ab@linux.intel.com> (raw)
In-Reply-To: <YGYQIJh8X2C8sW44@kroah.com>
>>> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
hui.wang@canonical.com, Vinod Koul <vkoul@kernel.org>,
sanyog.r.kale@intel.com,
Bard Liao <yung-chuan.liao@linux.intel.com>,
rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
Date: Thu, 1 Apr 2021 13:43:53 -0500 [thread overview]
Message-ID: <28515962-6fb1-511d-fc6b-f1422b11e6ab@linux.intel.com> (raw)
In-Reply-To: <YGYQIJh8X2C8sW44@kroah.com>
>>> 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.
next prev parent reply other threads:[~2021-04-01 18:55 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-31 1:13 [PATCH 0/2] soundwire: bus: handle errors in clock stop/start sequences Bard Liao
2021-03-31 1:13 ` Bard Liao
2021-03-31 1:13 ` [PATCH 1/2] soundwire: add macro to selectively change error levels Bard Liao
2021-03-31 1:13 ` Bard Liao
2021-04-01 7:24 ` Vinod Koul
2021-04-01 7:24 ` Vinod Koul
2021-04-01 14:30 ` Pierre-Louis Bossart
2021-04-01 16:46 ` Greg KH
2021-04-01 16:46 ` Greg KH
2021-04-01 18:07 ` Pierre-Louis Bossart
2021-04-01 18:07 ` Pierre-Louis Bossart
2021-04-01 18:25 ` Greg KH
2021-04-01 18:25 ` Greg KH
2021-04-01 18:43 ` Pierre-Louis Bossart [this message]
2021-04-01 18:43 ` Pierre-Louis Bossart
2021-04-01 20:56 ` Greg KH
2021-04-01 20:56 ` Greg KH
2021-04-01 22:05 ` Pierre-Louis Bossart
2021-04-01 22:05 ` Pierre-Louis Bossart
2021-03-31 1:13 ` [PATCH 2/2] soundwire: bus: handle errors in clock stop/start sequences Bard Liao
2021-03-31 1:13 ` Bard Liao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28515962-6fb1-511d-fc6b-f1422b11e6ab@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=bard.liao@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hui.wang@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rander.wang@linux.intel.com \
--cc=sanyog.r.kale@intel.com \
--cc=vkoul@kernel.org \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.