From: Greg KH <gregkh@linuxfoundation.org> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: alsa-devel@alsa-project.org, vinod.koul@linaro.org, tiwai@suse.de, linux-kernel@vger.kernel.org, vkoul@kernel.org, broonie@kernel.org, srinivas.kandagatla@linaro.org, sanyog.r.kale@intel.com, Bard Liao <yung-chuan.liao@linux.intel.com>, bard.liao@intel.com Subject: Re: [PATCH 1/2] soundwire: intel: uniquify debug message Date: Mon, 13 Jun 2022 19:21:22 +0200 [thread overview] Message-ID: <YqdyErIYYk85NV41@kroah.com> (raw) In-Reply-To: <7328fbb2-06af-99c8-5e7f-9d22f8c4e4f6@linux.intel.com> On Mon, Jun 13, 2022 at 07:48:32AM -0500, Pierre-Louis Bossart wrote: > > > >>>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > >>>> index 505c5ef061e3..808e2f320052 100644 > >>>> --- a/drivers/soundwire/intel.c > >>>> +++ b/drivers/soundwire/intel.c > >>>> @@ -1328,8 +1328,8 @@ int intel_link_startup(struct auxiliary_device *auxdev) > >>>> > >>>> if (bus->prop.hw_disabled) { > >>>> dev_info(dev, > >>>> - "SoundWire master %d is disabled, ignoring\n", > >>>> - sdw->instance); > >>>> + "%s: SoundWire master %d is disabled, ignoring\n", > >>>> + __func__, sdw->instance); > >>> > >>> This is not a debug message, please make it such if you want to have > >>> __func__ And even then, it's not needed as you can get that from the > >>> kernel automatically. > >> > >> Sorry, I don't understand the feedback at all. > > > > dev_info() is not a way to send debug messages. > > > > If you want this to be only for debugging, use dev_dbg(). And when you > > use that, you get the __func__ location for free in the output already > > if you want that. > > > >> This message was added precisely to figure out why the expected > >> programming sequence was not followed, only to discover that we have > >> devices with spurious PCI wakes handled below. Without this added > >> difference with __func__, we wouldn't know if the issue happened during > >> the expected/regular programming sequence or not. > > > > Perhaps make the text unique then? Why would an informational message > > need a function name. Drivers should be quiet when all is going well. > > If something is not going well, dev_info() is not the kernel log level > > to be sending something out at. > > This is not a debug message - unlike all the others. The BIOS and/or the > user may disable a specific link and it's useful to know. Ok, but that's not what your subject and changelog said you were modifying here. Hence my confusion :) > The intent of __func__ was precisely to make the text unique across all > cases where we test for hw_disabled, but you have a point that this > specific info message should be clearer. The text was actually already > different from the 'disable or not-started' cases, I might have done one > too many edits in this file. Will fix. > > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1489,8 +1489,8 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev) > >>>> bus = &sdw->cdns.bus; > >>>> > >>>> if (bus->prop.hw_disabled || !sdw->startup_done) { > >>>> - dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n", > >>>> - bus->link_id); > >>>> + dev_dbg(dev, "%s: SoundWire master %d is disabled or not-started, ignoring\n", > >>>> + __func__, bus->link_id); > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1549,8 +1549,8 @@ static int __maybe_unused intel_pm_prepare(struct device *dev) > >>>> int ret; > >>>> > >>>> if (bus->prop.hw_disabled || !sdw->startup_done) { > >>>> - dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n", > >>>> - bus->link_id); > >>>> + dev_dbg(dev, "%s: SoundWire master %d is disabled or not-started, ignoring\n", > >>>> + __func__, bus->link_id); > >>> > >>> Not needed, it is provided automatically if you ask the kernel for this. > >>> Same for all other instances in this patch. > >> > >> provided how? Your comment is a bit cryptic here. > > > > the dynamic debug code in the kernel already adds the function name > > where the message was sent from, if you want to know this in userspace. > > Please read the documentation for details (I think the key is the 'f' > > flag to be enabled in userspace). > > > > So adding __func__ to dev_dbg() calls are redundant and never needed. > > Oh wow, I had no idea - and I am not the only one :-) > > Here's what I get with a grep on the kernel code > > git grep __func__ | grep dev_dbg | wc -l > > 3122 > > > Doing this for sound/ gives me 356, and that's for single-line logs only. > > There's clearly something that was missed by tons of people. Yes, it's not commonly known so cleanups are always welcome. Perhaps some of those usages predate the feature being added as well. thanks, greg k-h
WARNING: multiple messages have this Message-ID (diff)
From: Greg KH <gregkh@linuxfoundation.org> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>, alsa-devel@alsa-project.org, vkoul@kernel.org, vinod.koul@linaro.org, linux-kernel@vger.kernel.org, tiwai@suse.de, broonie@kernel.org, srinivas.kandagatla@linaro.org, sanyog.r.kale@intel.com, bard.liao@intel.com Subject: Re: [PATCH 1/2] soundwire: intel: uniquify debug message Date: Mon, 13 Jun 2022 19:21:22 +0200 [thread overview] Message-ID: <YqdyErIYYk85NV41@kroah.com> (raw) In-Reply-To: <7328fbb2-06af-99c8-5e7f-9d22f8c4e4f6@linux.intel.com> On Mon, Jun 13, 2022 at 07:48:32AM -0500, Pierre-Louis Bossart wrote: > > > >>>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > >>>> index 505c5ef061e3..808e2f320052 100644 > >>>> --- a/drivers/soundwire/intel.c > >>>> +++ b/drivers/soundwire/intel.c > >>>> @@ -1328,8 +1328,8 @@ int intel_link_startup(struct auxiliary_device *auxdev) > >>>> > >>>> if (bus->prop.hw_disabled) { > >>>> dev_info(dev, > >>>> - "SoundWire master %d is disabled, ignoring\n", > >>>> - sdw->instance); > >>>> + "%s: SoundWire master %d is disabled, ignoring\n", > >>>> + __func__, sdw->instance); > >>> > >>> This is not a debug message, please make it such if you want to have > >>> __func__ And even then, it's not needed as you can get that from the > >>> kernel automatically. > >> > >> Sorry, I don't understand the feedback at all. > > > > dev_info() is not a way to send debug messages. > > > > If you want this to be only for debugging, use dev_dbg(). And when you > > use that, you get the __func__ location for free in the output already > > if you want that. > > > >> This message was added precisely to figure out why the expected > >> programming sequence was not followed, only to discover that we have > >> devices with spurious PCI wakes handled below. Without this added > >> difference with __func__, we wouldn't know if the issue happened during > >> the expected/regular programming sequence or not. > > > > Perhaps make the text unique then? Why would an informational message > > need a function name. Drivers should be quiet when all is going well. > > If something is not going well, dev_info() is not the kernel log level > > to be sending something out at. > > This is not a debug message - unlike all the others. The BIOS and/or the > user may disable a specific link and it's useful to know. Ok, but that's not what your subject and changelog said you were modifying here. Hence my confusion :) > The intent of __func__ was precisely to make the text unique across all > cases where we test for hw_disabled, but you have a point that this > specific info message should be clearer. The text was actually already > different from the 'disable or not-started' cases, I might have done one > too many edits in this file. Will fix. > > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1489,8 +1489,8 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev) > >>>> bus = &sdw->cdns.bus; > >>>> > >>>> if (bus->prop.hw_disabled || !sdw->startup_done) { > >>>> - dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n", > >>>> - bus->link_id); > >>>> + dev_dbg(dev, "%s: SoundWire master %d is disabled or not-started, ignoring\n", > >>>> + __func__, bus->link_id); > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1549,8 +1549,8 @@ static int __maybe_unused intel_pm_prepare(struct device *dev) > >>>> int ret; > >>>> > >>>> if (bus->prop.hw_disabled || !sdw->startup_done) { > >>>> - dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n", > >>>> - bus->link_id); > >>>> + dev_dbg(dev, "%s: SoundWire master %d is disabled or not-started, ignoring\n", > >>>> + __func__, bus->link_id); > >>> > >>> Not needed, it is provided automatically if you ask the kernel for this. > >>> Same for all other instances in this patch. > >> > >> provided how? Your comment is a bit cryptic here. > > > > the dynamic debug code in the kernel already adds the function name > > where the message was sent from, if you want to know this in userspace. > > Please read the documentation for details (I think the key is the 'f' > > flag to be enabled in userspace). > > > > So adding __func__ to dev_dbg() calls are redundant and never needed. > > Oh wow, I had no idea - and I am not the only one :-) > > Here's what I get with a grep on the kernel code > > git grep __func__ | grep dev_dbg | wc -l > > 3122 > > > Doing this for sound/ gives me 356, and that's for single-line logs only. > > There's clearly something that was missed by tons of people. Yes, it's not commonly known so cleanups are always welcome. Perhaps some of those usages predate the feature being added as well. thanks, greg k-h
next prev parent reply other threads:[~2022-06-13 17:22 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-10 2:35 [PATCH 0/2] soundwire: Intel: add trigger callback Bard Liao 2022-06-10 2:35 ` Bard Liao 2022-06-10 2:35 ` [PATCH 1/2] soundwire: intel: uniquify debug message Bard Liao 2022-06-10 2:35 ` Bard Liao 2022-06-10 5:25 ` Greg KH 2022-06-10 5:25 ` Greg KH 2022-06-10 15:06 ` Pierre-Louis Bossart 2022-06-10 15:06 ` Pierre-Louis Bossart 2022-06-11 7:20 ` Greg KH 2022-06-11 7:20 ` Greg KH 2022-06-13 12:48 ` Pierre-Louis Bossart 2022-06-13 12:48 ` Pierre-Louis Bossart 2022-06-13 17:21 ` Greg KH [this message] 2022-06-13 17:21 ` Greg KH 2022-06-13 19:21 ` Pierre-Louis Bossart 2022-06-10 2:35 ` [PATCH 2/2] soundwire: Intel: add trigger callback Bard Liao 2022-06-10 2:35 ` Bard Liao 2022-06-10 5:26 ` Greg KH 2022-06-10 5:26 ` Greg KH 2022-06-10 14:53 ` Pierre-Louis Bossart 2022-06-14 7:13 ` Liao, Bard
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=YqdyErIYYk85NV41@kroah.com \ --to=gregkh@linuxfoundation.org \ --cc=alsa-devel@alsa-project.org \ --cc=bard.liao@intel.com \ --cc=broonie@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=sanyog.r.kale@intel.com \ --cc=srinivas.kandagatla@linaro.org \ --cc=tiwai@suse.de \ --cc=vinod.koul@linaro.org \ --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: linkBe 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.