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

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

[adi25charis]

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).

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);
 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;
 
-	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);
+	if (ret) {
+		dev_err(dev, "%d:Error while fetching topology\n", ret);
+		goto free_topology;
+	}
+
 	/* process topology data */
 	ret = gbaudio_tplg_parse_data(gbmodule, topology);
 	if (ret) {
-- 
2.53.0

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

[Dan Carpenter]

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
> 

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

[Dan Carpenter]

On Tue, Jun 30, 2026 at 03:13:28PM +0300, Dan Carpenter wrote:
> On Mon, Jun 29, 2026 at 08:19:41PM +0530, adi25charis@gmail.com wrote:
> > +	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.
> 

(just ignore this comment.  It's just random thoughts, not related to
your patch.  Someone took some steps to try make this code less trusting
recently).

regards,
dan carpenter

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

[adi25charis]

From: Aditya Chari S <adi25charis@gmail.com>

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@gmail.com>
---
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.
---
 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);
+		goto disable_connection;
+	}
+
+	if (size < sizeof(*topology)) {
+		dev_err(dev, "Invalid topology size: %zu\n", size);
+		ret = -ENODATA;
 		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);
 		goto free_topology;
 	}
 
-- 
2.53.0

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@gmail.com wrote:
> From: Aditya Chari S <adi25charis@gmail.com>
> 
> 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@gmail.com>
> ---
> 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

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

[adi25charis]

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).

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

----------
v2:
- Fold in review feedback from Dan Carpenter.
- Store topology size as size_t instead of u16.
- Move topology size validation into gb_audio_probe() before kzalloc().
- Use -EINVAL for invalid topology size.
- Drop unrelated dev_err() formatting cleanup.
- Compile-tested with `make M=drivers/staging/greybus`.
- Run checkpatch.pl on the updated patch; no issues reported.
---
 drivers/staging/greybus/audio_codec.h  |  4 +++-
 drivers/staging/greybus/audio_gb.c     | 32 +++++++++-----------------
 drivers/staging/greybus/audio_module.c | 27 +++++++++++++++++-----
 3 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
index f3f7a7ec6..b45cd257d 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,
+				  size_t *size);
 int gb_audio_gb_get_topology(struct gb_connection *connection,
-			     struct gb_audio_topology **topology);
+			     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 9d8994fdb..2e6f155d8 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,
+				  size_t *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,25 +19,18 @@ 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;
-
-	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
-				topo, size);
-	if (ret) {
-		kfree(topo);
-		return ret;
-	}
-
-	*topology = topo;
+	*size = le16_to_cpu(size_resp.size);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size);
+
+int gb_audio_gb_get_topology(struct gb_connection *connection,
+			     struct gb_audio_topology *topology, size_t size)
+{
+	return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
+				 topology, size);
+}
 EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
 
 int gb_audio_gb_get_control(struct gb_connection *connection,
diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c
index 12c376c47..4cd1f42c1 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;
+	size_t size;
 	struct gb_audio_topology *topology;
 
 	/* There should be at least one Management and one Data cport */
@@ -304,16 +305,30 @@ 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;
+	}
+
+	if (size < sizeof(*topology)) {
+		dev_err(dev, "Invalid topology size: %zu\n", size);
+		ret = -EINVAL;
 		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);
+	if (ret) {
+		dev_err(dev, "%d:Error while fetching topology\n", ret);
+		goto free_topology;
+	}
+
 	/* process topology data */
 	ret = gbaudio_tplg_parse_data(gbmodule, topology);
 	if (ret) {
-- 
2.53.0

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

[Dan Carpenter]

On Wed, Jul 01, 2026 at 02:19:08AM +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).
> 
> Signed-off-by: Aditya Chari S <adi25charis@gmail.com>
> 
> ----------
> v2:
> - Fold in review feedback from Dan Carpenter.
> - Store topology size as size_t instead of u16.
> - Move topology size validation into gb_audio_probe() before kzalloc().
> - Use -EINVAL for invalid topology size.
> - Drop unrelated dev_err() formatting cleanup.
> - Compile-tested with `make M=drivers/staging/greybus`.
> - Run checkpatch.pl on the updated patch; no issues reported.
> ---

Thanks!

Reviewed-by: Dan Carpenter <error27@gmail.com>

regards,
dan carpenter