Re: [PATCH v2] staging: greybus: audio: split gb_audio_gb_get_topology()

[Dan Carpenter]

On Tue, Jun 30, 2026 at 10:16:02PM +0530, adi25charis@xxxxxxxxx wrote:
> From: Aditya Chari S <adi25charis@xxxxxxxxx>
> 
> Split gb_audio_gb_get_topology() into two functions:
> gb_audio_gb_get_topology_size() to fetch the topology size, and
> gb_audio_gb_get_topology() to fetch the topology data into a
> caller-provided buffer. This moves buffer allocation out of the
> audio_gb protocol helper and into gb_audio_probe(), where it
> belongs, addressing a long-standing FIXME.
> 
> Signed-off-by: Aditya Chari S <adi25charis@xxxxxxxxx>
> ---
> v2:
> - Store size as size_t instead of u16, per Dan Carpenter.
> - Move the size validation (size < sizeof(*topology)) out of
>   gb_audio_gb_get_topology() and into gb_audio_probe(), before
>   the kzalloc(), per Dan Carpenter.
> - Fix dev_err() format strings so %d is no longer printed first;
>   put it at the end of the message instead, per Dan Carpenter.
> 
> Compile-tested with `make M=drivers/staging/greybus modules`.
> All modified files (audio_codec.h, audio_gb.c, audio_module.c)
> compile without errors or warnings. checkpatch.pl --no-tree passes
> clean on all three files.
> ---

This v2 patch is built on top of the v1 patch which we're not
going to apply.  It needs to be folded into the v1 patch instead.

>  drivers/staging/greybus/audio_codec.h  |  4 ++--
>  drivers/staging/greybus/audio_gb.c     |  7 ++-----
>  drivers/staging/greybus/audio_module.c | 12 +++++++++---
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index be5a2a86b..b45cd257d 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -179,9 +179,9 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module);
>  
>  /* protocol related */
>  int gb_audio_gb_get_topology_size(struct gb_connection *connection,
> -				  u16 *size);
> +				  size_t *size);
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
> -			     struct gb_audio_topology *topology, u16 size);
> +			     struct gb_audio_topology *topology, size_t size);
>  int gb_audio_gb_get_control(struct gb_connection *connection,
>  			    u8 control_id, u8 index,
>  			    struct gb_audio_ctl_elem_value *value);
> diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> index e6356643d..2e6f155d8 100644
> --- a/drivers/staging/greybus/audio_gb.c
> +++ b/drivers/staging/greybus/audio_gb.c
> @@ -9,7 +9,7 @@
>  #include "audio_codec.h"
>  
>  int gb_audio_gb_get_topology_size(struct gb_connection *connection,
> -				  u16 *size)
> +				  size_t *size)
>  {
>  	struct gb_audio_get_topology_size_response size_resp;
>  	int ret;
> @@ -26,11 +26,8 @@ int gb_audio_gb_get_topology_size(struct gb_connection *connection,
>  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size);
>  
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
> -			     struct gb_audio_topology *topology, u16 size)
> +			     struct gb_audio_topology *topology, size_t size)
>  {
> -	if (size < sizeof(*topology))
> -		return -ENODATA;
> -
>  	return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
>  				 topology, size);
>  }
> diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c
> index 1163cf093..806533f03 100644
> --- a/drivers/staging/greybus/audio_module.c
> +++ b/drivers/staging/greybus/audio_module.c
> @@ -239,7 +239,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
>  	struct gb_audio_manager_module_descriptor desc;
>  	struct gbaudio_data_connection *dai, *_dai;
>  	int ret, i;
> -	u16 size;
> +	size_t size;
>  	struct gb_audio_topology *topology;
>  
>  	/* There should be at least one Management and one Data cport */
> @@ -307,7 +307,13 @@ static int gb_audio_probe(struct gb_bundle *bundle,
>  
>  	ret = gb_audio_gb_get_topology_size(gbmodule->mgmt_connection, &size);
>  	if (ret) {
> -		dev_err(dev, "%d:Error while fetching topology size\n", ret);
> +		dev_err(dev, "Error while fetching topology size: %d\n", ret);

Just leave this one as-is.  It's from the original code.  Although
you added the word "size" so I guess that kind of makes changing it
acceptable as well.  The rule is you're allowed to make minor white
space changes to line you're touching.  So I guess either way is fine.

> +		goto disable_connection;
> +	}
> +
> +	if (size < sizeof(*topology)) {
> +		dev_err(dev, "Invalid topology size: %zu\n", size);
> +		ret = -ENODATA;

I would probably have said -EINVAL is more appropriate.

>  		goto disable_connection;
>  	}
>  
> @@ -319,7 +325,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
>  
>  	ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology, size);
>  	if (ret) {
> -		dev_err(dev, "%d:Error while fetching topology\n", ret);
> +		dev_err(dev, "Error while fetching topology: %d\n", ret);

This change is entirely unrelated so it's not allowed.

regards,
dan carpenter

>  		goto free_topology;
>  	}
>  
> -- 
> 2.53.0