linux-kernel - Re: [PATCH] staging: greybus: extract a fxn to improve clarity

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: <5f036ea-4a8c-54ee-5eaa-4ee1aac0472c@inria.fr>
Date:   Fri, 17 Mar 2023 15:32:24 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Mark Thomas Heim <questioneight@...il.com>
cc:     Vaibhav Agarwal <vaibhav.sr@...il.com>,
        Mark Greer <mgreer@...malcreek.com>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, outreachy@...ts.linux.dev
Subject: Re: [PATCH] staging: greybus: extract a fxn to improve clarity



On Fri, 17 Mar 2023, Mark Thomas Heim wrote:

> The gb_audio_gb_get_topology function at the top of the file
> needs to be split per a TODO comment above the function. It
> is necessary to refactor the code to pull out a method
> that has fewer parameters to improve readability. A
> prototype for the new function is now in the relevant header,
> and the simpler function calls replace the old ones.

Mark,

Please go back and read the outreachy tutorial, in particular

https://kernelnewbies.org/PatchPhilosophy

The commit message is not written in the imperative, as it is required to
be.

Also, words like "needs to" and "necessary" express an opinion.  These
things may indeed be good things to do, but "needs to" and "necessary"
don't help to understand why the change is being made.

julia

>
> Signed-off-by: Mark Thomas Heim <questioneight@...il.com>
> ---
>  drivers/staging/greybus/audio_codec.h |  2 ++
>  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index ce15e800e607..a2e8361952b8 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
>  void gbaudio_unregister_module(struct gbaudio_module_info *module);
>
>  /* protocol related */
> +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> +			void *response, int response_size);
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology);
>  int gb_audio_gb_get_control(struct gb_connection *connection,
> diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> index 9d8994fdb41a..3c924d13f0e7 100644
> --- a/drivers/staging/greybus/audio_gb.c
> +++ b/drivers/staging/greybus/audio_gb.c
> @@ -8,7 +8,13 @@
>  #include <linux/greybus.h>
>  #include "audio_codec.h"
>
> -/* TODO: Split into separate calls */
> +int fetch_gb_audio_data(struct gb_connection *connection,
> +			int type, void *response, int response_size)
> +{
> +	return gb_operation_sync(connection, type, NULL, 0,
> +				 response, response_size);
> +}
> +
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology)
>  {
> @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
>  	u16 size;
>  	int ret;
>
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> -				NULL, 0, &size_resp, sizeof(size_resp));
> +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> +				  &size_resp, sizeof(size_resp));
>  	if (ret)
>  		return ret;
> -
>  	size = le16_to_cpu(size_resp.size);
>  	if (size < sizeof(*topo))
>  		return -ENODATA;
> -
>  	topo = kzalloc(size, GFP_KERNEL);
>  	if (!topo)
>  		return -ENOMEM;
> -
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> -				topo, size);
> +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY,
> +				  topo, size);
>  	if (ret) {
>  		kfree(topo);
>  		return ret;
>  	}
> -
>  	*topology = topo;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
> --
> 2.25.1
>
>
>