On Sun, Aug 21, 2016 at 09:43:22PM +0200, SF Markus Elfring wrote:

From: Markus Elfring elfring@users.sourceforge.net Date: Sun, 21 Aug 2016 21:02:06 +0200

Reuse existing functionality from memdup_user() instead of keeping duplicate source code.

This issue was detected by using the Coccinelle software.

It usually helps to have Coccinelle script in changelog.

But nevertheless

Acked-by: Vinod Koul vinod.koul@intel.com

Signed-off-by: Markus Elfring elfring@users.sourceforge.net

sound/core/compress_offload.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 2c49848..583d407 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -553,13 +553,9 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) * we should allow parameter change only when stream has been * opened not in other cases */

• params = kmalloc(sizeof(*params), GFP_KERNEL);
  

• if (!params)
  

• 	return -ENOMEM;
  

• if (copy_from_user(params, (void __user *)arg, sizeof(*params))) {
  

• 	retval = -EFAULT;
  

• 	goto out;
  

• }
  

• params = memdup_user((void __user *)arg, sizeof(*params));
  

• if (IS_ERR(params))
  

• 	return PTR_ERR(params);
  
  

  retval = snd_compress_check_input(params); if (retval)

-- 2.9.3

On Sun, 2016-08-21 at 21:45 +0200, SF Markus Elfring wrote:

Reduce the scope for the local variables to an if branch.

[]

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c

[]

@@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params)  static int  snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)  {

• struct snd_compr_params *params;
• int retval;

if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {

Likely better not reducing variable scope but changing:

if (stream->runtime->state == SNDRV_PCM_STATE_OPEN)

to

if (stream->runtime->state != SNDRV_PCM_STATE_OPEN) return -EPERM;

and unindenting the remainder of the code one level instead.

From: Markus Elfring elfring@users.sourceforge.net Date: Mon, 22 Aug 2016 10:01:52 +0200

* Reverse a condition check.

* Reduce the indentation one level then for some source code from a previous if branch.

Suggested-by: Joe Perches joe@perches.com Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/core/compress_offload.c | 62 +++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 2c49848..a10d139 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -548,43 +548,41 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) struct snd_compr_params *params; int retval;

- if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { - /* - * we should allow parameter change only when stream has been - * opened not in other cases - */ - params = kmalloc(sizeof(*params), GFP_KERNEL); - if (!params) - return -ENOMEM; - if (copy_from_user(params, (void __user *)arg, sizeof(*params))) { - retval = -EFAULT; - goto out; - } + if (stream->runtime->state != SNDRV_PCM_STATE_OPEN) + return -EPERM; + /* + * we should allow parameter change only when stream has been + * opened not in other cases + */ + params = kmalloc(sizeof(*params), GFP_KERNEL); + if (!params) + return -ENOMEM; + if (copy_from_user(params, (void __user *)arg, sizeof(*params))) { + retval = -EFAULT; + goto out; + }

- retval = snd_compress_check_input(params); - if (retval) - goto out; + retval = snd_compress_check_input(params); + if (retval) + goto out;

- retval = snd_compr_allocate_buffer(stream, params); - if (retval) { - retval = -ENOMEM; - goto out; - } + retval = snd_compr_allocate_buffer(stream, params); + if (retval) { + retval = -ENOMEM; + goto out; + }

- retval = stream->ops->set_params(stream, params); - if (retval) - goto out; + retval = stream->ops->set_params(stream, params); + if (retval) + goto out;

- stream->metadata_set = false; - stream->next_track = false; + stream->metadata_set = false; + stream->next_track = false;

- if (stream->direction == SND_COMPRESS_PLAYBACK) - stream->runtime->state = SNDRV_PCM_STATE_SETUP; - else - stream->runtime->state = SNDRV_PCM_STATE_PREPARED; - } else { - return -EPERM; - } + if (stream->direction == SND_COMPRESS_PLAYBACK) + stream->runtime->state = SNDRV_PCM_STATE_SETUP; + else + stream->runtime->state = SNDRV_PCM_STATE_PREPARED; out: kfree(params); return retval;

On Sun, Aug 21, 2016 at 04:36:22PM -0400, Julia Lawall wrote:

On Sun, 21 Aug 2016, SF Markus Elfring wrote:

...

From: Markus Elfring elfring@users.sourceforge.net Date: Sun, 21 Aug 2016 21:26:18 +0200

Reduce the scope for the local variables to an if branch.

Signed-off-by: Markus Elfring elfring@users.sourceforge.net

sound/core/compress_offload.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 583d407..b43aec5 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params) static int snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) {

• struct snd_compr_params *params;
• int retval;
• if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { /*
  • we should allow parameter change only when stream has been
  • opened not in other cases
  */

• int retval;
  

• struct snd_compr_params *params;
  

I don't like this at all. Local variables should be at the top of the function, not hiding under 4 lines of comments in the middle of the code.

I agree with you this, it doesn't help IMO as well