Re: [PATCH] staging: greybus: audio: split topology get into size and data calls

[Dan Carpenter <error27@gmail.com>]

On Mon, Jun 29, 2026 at 08:19:41PM +0530, adi25charis@gmail.com wrote:
> From: Aditya Chari S <adi25charis@gmail.com>
> 
> gb_audio_gb_get_topology() combined three separate responsibilities
> into a single call: querying the topology size, allocating a buffer
> for it, and fetching the topology data into that buffer. This left
> callers with no way to perform any of these steps independently, and
> forced the kzalloc() allocation to live inside the protocol-layer
> driver rather than the caller, as already flagged by a FIXME comment
> at the call site in audio_module.c.
> 
> Split the function into two:
> 
>   gb_audio_gb_get_topology_size() - queries only the topology size
>   gb_audio_gb_get_topology()      - fetches topology data into a
>                                      caller-supplied buffer of a
>                                      given size
> 
> Update the only caller, gb_audio_probe() in audio_module.c, to query
> the size first, allocate the topology buffer itself, then fetch the
> data into it, freeing the buffer via the existing free_topology error
> path on failure.
> 
> This resolves both the "TODO: Split into separate calls" comment
> above the original function in audio_gb.c and the FIXME comment at
> the call site in audio_module.c, both of which are removed as part
> of this change.
> 
> No functional change in behavior for the existing probe path.
> 
> Compile-tested with W=1, sparse (C=2), and checkpatch.pl; all clean
> on the three changed files (audio_gb.c, audio_module.c, audio_codec.h).

Put this sort of meta commentary under the --- cut off line.
Also compile the whole module, not just the modified files.

> 
> Signed-off-by: Aditya Chari S <adi25charis@gmail.com>
> ---
  ^^^

>  drivers/staging/greybus/audio_codec.h  |  4 +++-
>  drivers/staging/greybus/audio_gb.c     | 33 ++++++++++----------------
>  drivers/staging/greybus/audio_module.c | 21 +++++++++++-----
>  3 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index f3f7a7ec6..be5a2a86b 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -178,8 +178,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
>  void gbaudio_unregister_module(struct gbaudio_module_info *module);
>  
>  /* protocol related */
> +int gb_audio_gb_get_topology_size(struct gb_connection *connection,
> +				  u16 *size);

Please store sizes in size_t.

>  int gb_audio_gb_get_topology(struct gb_connection *connection,
> -			     struct gb_audio_topology **topology);
> +			     struct gb_audio_topology *topology, u16 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 9d8994fdb..e6356643d 100644
> --- a/drivers/staging/greybus/audio_gb.c
> +++ b/drivers/staging/greybus/audio_gb.c
> @@ -8,13 +8,10 @@
>  #include <linux/greybus.h>
>  #include "audio_codec.h"
>  
> -/* TODO: Split into separate calls */
> -int gb_audio_gb_get_topology(struct gb_connection *connection,
> -			     struct gb_audio_topology **topology)
> +int gb_audio_gb_get_topology_size(struct gb_connection *connection,
> +				  u16 *size)
>  {
>  	struct gb_audio_get_topology_size_response size_resp;
> -	struct gb_audio_topology *topo;
> -	u16 size;
>  	int ret;
>  
>  	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> @@ -22,24 +19,20 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
>  	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;
> +	*size = le16_to_cpu(size_resp.size);
>  
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> -				topo, size);
> -	if (ret) {
> -		kfree(topo);
> -		return ret;
> -	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size);
>  
> -	*topology = topo;
> +int gb_audio_gb_get_topology(struct gb_connection *connection,
> +			     struct gb_audio_topology *topology, u16 size)
> +{
> +	if (size < sizeof(*topology))
> +		return -ENODATA;

This check should be done in gb_audio_probe() before the kzalloc().

>  
> -	return 0;
> +	return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> +				 topology, size);
>  }
>  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
>  
> diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c
> index 12c376c47..1163cf093 100644
> --- a/drivers/staging/greybus/audio_module.c
> +++ b/drivers/staging/greybus/audio_module.c
> @@ -239,6 +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;
>  	struct gb_audio_topology *topology;
>  
>  	/* There should be at least one Management and one Data cport */
> @@ -304,16 +305,24 @@ static int gb_audio_probe(struct gb_bundle *bundle,
>  	}
>  	gbmodule->dev_id = gbmodule->mgmt_connection->intf->interface_id;
>  
> -	/*
> -	 * FIXME: malloc for topology happens via audio_gb driver
> -	 * should be done within codec driver itself
> -	 */
> -	ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology);
> +	ret = gb_audio_gb_get_topology_size(gbmodule->mgmt_connection, &size);
>  	if (ret) {
> -		dev_err(dev, "%d:Error while fetching topology\n", ret);
> +		dev_err(dev, "%d:Error while fetching topology size\n", ret);
> +		goto disable_connection;
> +	}
> +
> +	topology = kzalloc(size, GFP_KERNEL);
> +	if (!topology) {
> +		ret = -ENOMEM;
>  		goto disable_connection;
>  	}
>  
> +	ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology, size);

It's unfortunate that we don't save the size anywhere.  This code
relies on trusting the firmware for its security.  It would be better
to move away from that.

> +	if (ret) {
> +		dev_err(dev, "%d:Error while fetching topology\n", ret);

This is a weird format for a warning.  Normally it %d would come last.
I see that you copied the other printk from earlier, but don't do that.
Only copy good code, not the weirdo stuff.

regards,
dan carpenter

> +		goto free_topology;
> +	}
> +
>  	/* process topology data */
>  	ret = gbaudio_tplg_parse_data(gbmodule, topology);
>  	if (ret) {
> -- 
> 2.53.0
>