| 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
| ||
|
Message-ID: <918f9c287b7ef9efb707d88560a9ccde577c5dbf.camel@linux.intel.com>
Date: Wed, 31 Jul 2019 10:11:03 -0700
From: Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mark Brown <broonie@...nel.org>, Takashi Iwai <tiwai@...e.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
alsa-devel@...a-project.org, Liam Girdwood <lgirdwood@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [PATCH v2 3/3] ASoC: SOF: no need to check return
value of debugfs_create functions
On Wed, 2019-07-31 at 15:17 +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic
> should
> never do something different based on this.
>
> Also, if a debugfs call fails, userspace is notified with an error in
> the log, so no need to log the error again.
>
> Because we no longer need to check the return value, there's no need
> to
> save the dentry returned by debugfs. Just use the dentry in the file
> pointer if we really need to figure out the "name" of the file being
> opened.
>
> Cc: Liam Girdwood <lgirdwood@...il.com>
> Cc: Mark Brown <broonie@...nel.org>
> Cc: Jaroslav Kysela <perex@...ex.cz>
> Cc: Takashi Iwai <tiwai@...e.com>
> Cc: Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Cc: alsa-devel@...a-project.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
> v2: rebase on 5.3-rc1
> change Subject line to match the subsystem better
> rework based on debugfs core now reporting errors.
>
> sound/soc/sof/debug.c | 49 +++++++++++++++-----------------------
> --
> sound/soc/sof/sof-priv.h | 1 -
> sound/soc/sof/trace.c | 9 ++------
> 3 files changed, 20 insertions(+), 39 deletions(-)
>
> diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
> index 2388477a965e..40940b2fe9d5 100644
> --- a/sound/soc/sof/debug.c
> +++ b/sound/soc/sof/debug.c
> @@ -128,6 +128,7 @@ static ssize_t sof_dfsentry_write(struct file
> *file, const char __user *buffer,
> unsigned long ipc_duration_ms = 0;
> bool flood_duration_test = false;
> unsigned long ipc_count = 0;
> + struct dentry *dentry;
> int err;
> #endif
> size_t size;
> @@ -149,11 +150,12 @@ static ssize_t sof_dfsentry_write(struct file
> *file, const char __user *buffer,
> * ipc_duration_ms test floods the DSP for the time specified
> * in the debugfs entry.
> */
> - if (strcmp(dfse->dfsentry->d_name.name, "ipc_flood_count") &&
> - strcmp(dfse->dfsentry->d_name.name,
> "ipc_flood_duration_ms"))
> + dentry = file->f_path.dentry;
> + if (strcmp(dentry->d_name.name, "ipc_flood_count") &&
> + strcmp(dentry->d_name.name, "ipc_flood_duration_ms"))
> return -EINVAL;
>
> - if (!strcmp(dfse->dfsentry->d_name.name,
> "ipc_flood_duration_ms"))
> + if (!strcmp(dentry->d_name.name, "ipc_flood_duration_ms"))
> flood_duration_test = true;
>
> /* test completion criterion */
> @@ -219,6 +221,7 @@ static ssize_t sof_dfsentry_read(struct file
> *file, char __user *buffer,
> {
> struct snd_sof_dfsentry *dfse = file->private_data;
> struct snd_sof_dev *sdev = dfse->sdev;
> + struct dentry *dentry;
> loff_t pos = *ppos;
> size_t size_ret;
> int skip = 0;
> @@ -226,8 +229,9 @@ static ssize_t sof_dfsentry_read(struct file
> *file, char __user *buffer,
> u8 *buf;
>
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
> - if ((!strcmp(dfse->dfsentry->d_name.name, "ipc_flood_count") ||
> - !strcmp(dfse->dfsentry->d_name.name,
> "ipc_flood_duration_ms")) &&
> + dentry = file->f_path.dentry;
> + if ((!strcmp(dentry->d_name.name, "ipc_flood_count") ||
> + !strcmp(dentry->d_name.name, "ipc_flood_duration_ms")) &&
> dfse->cache_buf) {
> if (*ppos)
> return 0;
> @@ -290,8 +294,7 @@ static ssize_t sof_dfsentry_read(struct file
> *file, char __user *buffer,
> if (!pm_runtime_active(sdev->dev) &&
> dfse->access_type == SOF_DEBUGFS_ACCESS_D0_ONLY) {
> dev_err(sdev->dev,
> - "error: debugfs entry %s cannot be read
> in DSP D3\n",
> - dfse->dfsentry->d_name.name);
> + "error: debugfs entry cannot be read in
> DSP D3\n");
> kfree(buf);
> return -EINVAL;
> }
> @@ -356,17 +359,11 @@ int snd_sof_debugfs_io_item(struct snd_sof_dev
> *sdev,
> }
> #endif
>
> - dfse->dfsentry = debugfs_create_file(name, 0444, sdev-
> >debugfs_root,
> - dfse, &sof_dfs_fops);
> - if (!dfse->dfsentry) {
> - /* can't rely on debugfs, only log error and keep going
> */
> - dev_err(sdev->dev, "error: cannot create debugfs entry
> %s\n",
> - name);
> - } else {
> - /* add to dfsentry list */
> - list_add(&dfse->list, &sdev->dfsentry_list);
> + debugfs_create_file(name, 0444, sdev->debugfs_root, dfse,
> + &sof_dfs_fops);
Minor nit-pick but looks good otherwise. This will fit all in one line
(same with the lines below).
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>
>
> - }
> + /* add to dfsentry list */
> + list_add(&dfse->list, &sdev->dfsentry_list);
>
> return 0;
> }
> @@ -402,16 +399,10 @@ int snd_sof_debugfs_buf_item(struct snd_sof_dev
> *sdev,
> return -ENOMEM;
> #endif
>
> - dfse->dfsentry = debugfs_create_file(name, mode, sdev-
> >debugfs_root,
> - dfse, &sof_dfs_fops);
> - if (!dfse->dfsentry) {
> - /* can't rely on debugfs, only log error and keep going
> */
> - dev_err(sdev->dev, "error: cannot create debugfs entry
> %s\n",
> - name);
> - } else {
> - /* add to dfsentry list */
> - list_add(&dfse->list, &sdev->dfsentry_list);
> - }
> + debugfs_create_file(name, mode, sdev->debugfs_root, dfse,
> + &sof_dfs_fops);
> + /* add to dfsentry list */
> + list_add(&dfse->list, &sdev->dfsentry_list);
>
> return 0;
> }
> @@ -426,10 +417,6 @@ int snd_sof_dbg_init(struct snd_sof_dev *sdev)
>
> /* use "sof" as top level debugFS dir */
> sdev->debugfs_root = debugfs_create_dir("sof", NULL);
> - if (IS_ERR_OR_NULL(sdev->debugfs_root)) {
> - dev_err(sdev->dev, "error: failed to create debugfs
> directory\n");
> - return 0;
> - }
>
> /* init dfsentry list */
> INIT_LIST_HEAD(&sdev->dfsentry_list);
> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index b8c0b2a22684..79b6709d1874 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -228,7 +228,6 @@ enum sof_debugfs_access_type {
>
> /* FS entry for debug files that can expose DSP memories, registers
> */
> struct snd_sof_dfsentry {
> - struct dentry *dfsentry;
> size_t size;
> enum sof_dfsentry_type type;
> /*
> diff --git a/sound/soc/sof/trace.c b/sound/soc/sof/trace.c
> index befed975161c..4c3cff031fd6 100644
> --- a/sound/soc/sof/trace.c
> +++ b/sound/soc/sof/trace.c
> @@ -148,13 +148,8 @@ static int trace_debugfs_create(struct
> snd_sof_dev *sdev)
> dfse->size = sdev->dmatb.bytes;
> dfse->sdev = sdev;
>
> - dfse->dfsentry = debugfs_create_file("trace", 0444, sdev-
> >debugfs_root,
> - dfse,
> &sof_dfs_trace_fops);
> - if (!dfse->dfsentry) {
> - /* can't rely on debugfs, only log error and keep going
> */
> - dev_err(sdev->dev,
> - "error: cannot create debugfs entry for
> trace\n");
> - }
> + debugfs_create_file("trace", 0444, sdev->debugfs_root, dfse,
> + &sof_dfs_trace_fops);
>
> return 0;
> }
Powered by blists - more mailing lists