The FIXME in gb_audio_probe noted that memory allocation for the topology should happen within the codec driver rather than the greybus helper.
Move the size-check and kzalloc from audio_gb.c to audio_module.c and update the function signature of gb_audio_gb_get_topology to accept the pointer. This clarifies ownership of the allocated memory.
Signed-off-by: Jose A. Perez de Azpillaga azpijr@gmail.com --- drivers/staging/greybus/audio_gb.c | 33 +++----------------------- drivers/staging/greybus/audio_module.c | 29 ++++++++++++++++++---- 2 files changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 9d8994fdb41a..4037b3bf1e9f 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -8,38 +8,11 @@ #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) + struct gb_audio_topology *topology) { - 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, - NULL, 0, &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); - if (ret) { - kfree(topo); - return ret; - } - - *topology = topo; - - return 0; + return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, + NULL, 0, topology, le16_to_cpu(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 12c376c477b3..8efb720c3f30 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -240,6 +240,8 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gbaudio_data_connection *dai, *_dai; int ret, i; struct gb_audio_topology *topology; + struct gb_audio_get_topology_size_response size_resp; + u16 size;
/* There should be at least one Management and one Data cport */ if (bundle->num_cports < 2) @@ -304,13 +306,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_operation_sync(gbmodule->mgmt_connection, + GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, NULL, 0, + &size_resp, sizeof(size_resp)); + if (ret) + goto disable_connection; + + size = le16_to_cpu(size_resp.size); + if (size < sizeof(*topology)) { + ret = -ENODATA; + goto disable_connection; + } + + topology = kzalloc(size, GFP_KERNEL); + if (!topology) { + ret = -ENOMEM; + goto disable_connection; + } + + topology->size = cpu_to_le16(size); + + ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology); if (ret) { dev_err(dev, "%d:Error while fetching topology\n", ret); + kfree(topology); goto disable_connection; }
-- 2.53.0
Hi Jose,
kernel test robot noticed the following build errors:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/Jose-A-Perez-de-Azpillaga/sta... base: staging/staging-testing patch link: https://lore.kernel.org/r/20260223195939.71151-1-azpijr%40gmail.com patch subject: [PATCH] staging: greybus: move topology allocation to codec probe config: parisc-randconfig-001-20260224 (https://download.01.org/0day-ci/archive/20260224/202602240844.4eT24iVh-lkp@i...) compiler: hppa-linux-gcc (GCC) 10.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260224/202602240844.4eT24iVh-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202602240844.4eT24iVh-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/staging/greybus/audio_module.c: In function 'gb_audio_probe':
drivers/staging/greybus/audio_module.c:327:10: error: 'struct gb_audio_topology' has no member named 'size'
327 | topology->size = cpu_to_le16(size); | ^~
drivers/staging/greybus/audio_module.c:329:60: error: passing argument 2 of 'gb_audio_gb_get_topology' from incompatible pointer type [-Werror=incompatible-pointer-types]
329 | ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology); | ^~~~~~~~ | | | struct gb_audio_topology * In file included from drivers/staging/greybus/audio_module.c:12: drivers/staging/greybus/audio_codec.h:182:36: note: expected 'struct gb_audio_topology **' but argument is of type 'struct gb_audio_topology *' 182 | struct gb_audio_topology **topology); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~ cc1: some warnings being treated as errors --
drivers/staging/greybus/audio_gb.c:11:5: error: conflicting types for 'gb_audio_gb_get_topology'
11 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/staging/greybus/audio_gb.c:9: drivers/staging/greybus/audio_codec.h:181:5: note: previous declaration of 'gb_audio_gb_get_topology' was here 181 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/swab.h:5, from include/uapi/linux/byteorder/big_endian.h:14, from include/linux/byteorder/big_endian.h:5, from arch/parisc/include/uapi/asm/byteorder.h:5, from arch/parisc/include/asm/bitops.h:11, from include/linux/bitops.h:67, from include/linux/kernel.h:23, from include/linux/greybus.h:14, from drivers/staging/greybus/audio_gb.c:8: drivers/staging/greybus/audio_gb.c: In function 'gb_audio_gb_get_topology':
drivers/staging/greybus/audio_gb.c:15:43: error: 'struct gb_audio_topology' has no member named 'size'
15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:105:31: note: in definition of macro '__swab16' 105 | (__u16)(__builtin_constant_p(x) ? \ | ^ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:23: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~
drivers/staging/greybus/audio_gb.c:15:43: error: 'struct gb_audio_topology' has no member named 'size'
15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:106:2: note: in expansion of macro '___constant_swab16' 106 | ___constant_swab16(x) : \ | ^~~~~~~~~~~~~~~~~~ include/uapi/linux/byteorder/big_endian.h:37:26: note: in expansion of macro '__swab16' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^~~~~~~~ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:23: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~
drivers/staging/greybus/audio_gb.c:15:43: error: 'struct gb_audio_topology' has no member named 'size'
15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:16:12: note: in definition of macro '___constant_swab16' 16 | (((__u16)(x) & (__u16)0xff00U) >> 8))) | ^ include/uapi/linux/byteorder/big_endian.h:37:26: note: in expansion of macro '__swab16' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^~~~~~~~ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:23: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~
drivers/staging/greybus/audio_gb.c:15:43: error: 'struct gb_audio_topology' has no member named 'size'
15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:107:12: note: in definition of macro '__swab16' 107 | __fswab16(x)) | ^ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:23: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~ In file included from include/linux/linkage.h:7, from include/linux/kernel.h:18, from include/linux/greybus.h:14, from drivers/staging/greybus/audio_gb.c:8: drivers/staging/greybus/audio_gb.c: At top level: drivers/staging/greybus/audio_gb.c:17:19: error: conflicting types for 'gb_audio_gb_get_topology' 17 | EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); | ^~~~~~~~~~~~~~~~~~~~~~~~ include/linux/export.h:76:21: note: in definition of macro '__EXPORT_SYMBOL' 76 | extern typeof(sym) sym; \ | ^~~ include/linux/export.h:90:33: note: in expansion of macro '_EXPORT_SYMBOL' 90 | #define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "GPL") | ^~~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:17:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL' 17 | EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); | ^~~~~~~~~~~~~~~~~ In file included from drivers/staging/greybus/audio_gb.c:9: drivers/staging/greybus/audio_codec.h:181:5: note: previous declaration of 'gb_audio_gb_get_topology' was here 181 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c: In function 'gb_audio_gb_get_topology':
drivers/staging/greybus/audio_gb.c:16:1: warning: control reaches end of non-void function [-Wreturn-type]
16 | } | ^
vim +327 drivers/staging/greybus/audio_module.c
228 229 /* 230 * This is the basic hook get things initialized and registered w/ gb 231 */ 232 233 static int gb_audio_probe(struct gb_bundle *bundle, 234 const struct greybus_bundle_id *id) 235 { 236 struct device *dev = &bundle->dev; 237 struct gbaudio_module_info *gbmodule; 238 struct greybus_descriptor_cport *cport_desc; 239 struct gb_audio_manager_module_descriptor desc; 240 struct gbaudio_data_connection *dai, *_dai; 241 int ret, i; 242 struct gb_audio_topology *topology; 243 struct gb_audio_get_topology_size_response size_resp; 244 u16 size; 245 246 /* There should be at least one Management and one Data cport */ 247 if (bundle->num_cports < 2) 248 return -ENODEV; 249 250 /* 251 * There can be only one Management connection and any number of data 252 * connections. 253 */ 254 gbmodule = devm_kzalloc(dev, sizeof(*gbmodule), GFP_KERNEL); 255 if (!gbmodule) 256 return -ENOMEM; 257 258 gbmodule->num_data_connections = bundle->num_cports - 1; 259 INIT_LIST_HEAD(&gbmodule->data_list); 260 INIT_LIST_HEAD(&gbmodule->widget_list); 261 INIT_LIST_HEAD(&gbmodule->ctl_list); 262 INIT_LIST_HEAD(&gbmodule->widget_ctl_list); 263 INIT_LIST_HEAD(&gbmodule->jack_list); 264 gbmodule->dev = dev; 265 snprintf(gbmodule->name, sizeof(gbmodule->name), "%s.%s", dev->driver->name, 266 dev_name(dev)); 267 greybus_set_drvdata(bundle, gbmodule); 268 269 /* Create all connections */ 270 for (i = 0; i < bundle->num_cports; i++) { 271 cport_desc = &bundle->cport_desc[i]; 272 273 switch (cport_desc->protocol_id) { 274 case GREYBUS_PROTOCOL_AUDIO_MGMT: 275 ret = gb_audio_add_mgmt_connection(gbmodule, cport_desc, 276 bundle); 277 if (ret) 278 goto destroy_connections; 279 break; 280 case GREYBUS_PROTOCOL_AUDIO_DATA: 281 ret = gb_audio_add_data_connection(gbmodule, cport_desc, 282 bundle); 283 if (ret) 284 goto destroy_connections; 285 break; 286 default: 287 dev_err(dev, "Unsupported protocol: 0x%02x\n", 288 cport_desc->protocol_id); 289 ret = -ENODEV; 290 goto destroy_connections; 291 } 292 } 293 294 /* There must be a management cport */ 295 if (!gbmodule->mgmt_connection) { 296 ret = -EINVAL; 297 dev_err(dev, "Missing management connection\n"); 298 goto destroy_connections; 299 } 300 301 /* Initialize management connection */ 302 ret = gb_connection_enable(gbmodule->mgmt_connection); 303 if (ret) { 304 dev_err(dev, "%d: Error while enabling mgmt connection\n", ret); 305 goto destroy_connections; 306 } 307 gbmodule->dev_id = gbmodule->mgmt_connection->intf->interface_id; 308 309 ret = gb_operation_sync(gbmodule->mgmt_connection, 310 GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, NULL, 0, 311 &size_resp, sizeof(size_resp)); 312 if (ret) 313 goto disable_connection; 314 315 size = le16_to_cpu(size_resp.size); 316 if (size < sizeof(*topology)) { 317 ret = -ENODATA; 318 goto disable_connection; 319 } 320 321 topology = kzalloc(size, GFP_KERNEL); 322 if (!topology) { 323 ret = -ENOMEM; 324 goto disable_connection; 325 } 326
327 topology->size = cpu_to_le16(size);
328
329 ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology);
330 if (ret) { 331 dev_err(dev, "%d:Error while fetching topology\n", ret); 332 kfree(topology); 333 goto disable_connection; 334 } 335 336 /* process topology data */ 337 ret = gbaudio_tplg_parse_data(gbmodule, topology); 338 if (ret) { 339 dev_err(dev, "%d:Error while parsing topology data\n", 340 ret); 341 goto free_topology; 342 } 343 gbmodule->topology = topology; 344 345 /* Initialize data connections */ 346 list_for_each_entry(dai, &gbmodule->data_list, list) { 347 ret = gb_connection_enable(dai->connection); 348 if (ret) { 349 dev_err(dev, 350 "%d:Error while enabling %d:data connection\n", 351 ret, le16_to_cpu(dai->data_cport)); 352 goto disable_data_connection; 353 } 354 } 355 356 /* register module with gbcodec */ 357 ret = gbaudio_register_module(gbmodule); 358 if (ret) 359 goto disable_data_connection; 360 361 /* inform above layer for uevent */ 362 dev_dbg(dev, "Inform set_event:%d to above layer\n", 1); 363 /* prepare for the audio manager */ 364 strscpy(desc.name, gbmodule->name, sizeof(desc.name)); 365 desc.vid = 2; /* todo */ 366 desc.pid = 3; /* todo */ 367 desc.intf_id = gbmodule->dev_id; 368 desc.op_devices = gbmodule->op_devices; 369 desc.ip_devices = gbmodule->ip_devices; 370 gbmodule->manager_id = gb_audio_manager_add(&desc); 371 372 dev_dbg(dev, "Add GB Audio device:%s\n", gbmodule->name); 373 374 gb_pm_runtime_put_autosuspend(bundle); 375 376 return 0; 377 378 disable_data_connection: 379 list_for_each_entry_safe(dai, _dai, &gbmodule->data_list, list) 380 gb_connection_disable(dai->connection); 381 gbaudio_tplg_release(gbmodule); 382 gbmodule->topology = NULL; 383 384 free_topology: 385 kfree(topology); 386 387 disable_connection: 388 gb_connection_disable(gbmodule->mgmt_connection); 389 390 destroy_connections: 391 list_for_each_entry_safe(dai, _dai, &gbmodule->data_list, list) { 392 gb_connection_destroy(dai->connection); 393 list_del(&dai->list); 394 devm_kfree(dev, dai); 395 } 396 397 if (gbmodule->mgmt_connection) 398 gb_connection_destroy(gbmodule->mgmt_connection); 399 400 devm_kfree(dev, gbmodule); 401 402 return ret; 403 } 404
Hi Jose,
kernel test robot noticed the following build errors:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/Jose-A-Perez-de-Azpillaga/sta... base: staging/staging-testing patch link: https://lore.kernel.org/r/20260223195939.71151-1-azpijr%40gmail.com patch subject: [PATCH] staging: greybus: move topology allocation to codec probe config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20260224/202602240818.4lyiDRiD-lkp@i...) compiler: m68k-linux-gcc (GCC) 15.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260224/202602240818.4lyiDRiD-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202602240818.4lyiDRiD-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/staging/greybus/audio_gb.c:11:5: error: conflicting types for 'gb_audio_gb_get_topology'; have 'int(struct gb_connection *, struct gb_audio_topology *)'
11 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/staging/greybus/audio_gb.c:9: drivers/staging/greybus/audio_codec.h:181:5: note: previous declaration of 'gb_audio_gb_get_topology' with type 'int(struct gb_connection *, struct gb_audio_topology **)' 181 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/swab.h:5, from include/uapi/linux/byteorder/big_endian.h:14, from include/linux/byteorder/big_endian.h:5, from arch/m68k/include/uapi/asm/byteorder.h:5, from include/asm-generic/bitops/le.h:6, from arch/m68k/include/asm/bitops.h:569, from include/linux/bitops.h:67, from include/linux/kernel.h:23, from include/linux/greybus.h:14, from drivers/staging/greybus/audio_gb.c:8: drivers/staging/greybus/audio_gb.c: In function 'gb_audio_gb_get_topology': drivers/staging/greybus/audio_gb.c:15:64: error: 'struct gb_audio_topology' has no member named 'size' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:105:38: note: in definition of macro '__swab16' 105 | (__u16)(__builtin_constant_p(x) ? \ | ^ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:44: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:64: error: 'struct gb_audio_topology' has no member named 'size' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:106:9: note: in expansion of macro '___constant_swab16' 106 | ___constant_swab16(x) : \ | ^~~~~~~~~~~~~~~~~~ include/uapi/linux/byteorder/big_endian.h:37:26: note: in expansion of macro '__swab16' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^~~~~~~~ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:44: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:64: error: 'struct gb_audio_topology' has no member named 'size' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:16:19: note: in definition of macro '___constant_swab16' 16 | (((__u16)(x) & (__u16)0xff00U) >> 8))) | ^ include/uapi/linux/byteorder/big_endian.h:37:26: note: in expansion of macro '__swab16' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^~~~~~~~ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:44: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:64: error: 'struct gb_audio_topology' has no member named 'size' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~ include/uapi/linux/swab.h:107:19: note: in definition of macro '__swab16' 107 | __fswab16(x)) | ^ include/linux/byteorder/generic.h:91:21: note: in expansion of macro '__le16_to_cpu' 91 | #define le16_to_cpu __le16_to_cpu | ^~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:15:44: note: in expansion of macro 'le16_to_cpu' 15 | NULL, 0, topology, le16_to_cpu(topology->size)); | ^~~~~~~~~~~ In file included from include/linux/linkage.h:7, from include/linux/kernel.h:18: drivers/staging/greybus/audio_gb.c: At top level: drivers/staging/greybus/audio_gb.c:17:19: error: conflicting types for 'gb_audio_gb_get_topology'; have 'int(struct gb_connection *, struct gb_audio_topology *)' 17 | EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); | ^~~~~~~~~~~~~~~~~~~~~~~~ include/linux/export.h:76:28: note: in definition of macro '__EXPORT_SYMBOL' 76 | extern typeof(sym) sym; \ | ^~~ include/linux/export.h:90:41: note: in expansion of macro '_EXPORT_SYMBOL' 90 | #define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "GPL") | ^~~~~~~~~~~~~~ drivers/staging/greybus/audio_gb.c:17:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL' 17 | EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); | ^~~~~~~~~~~~~~~~~ drivers/staging/greybus/audio_codec.h:181:5: note: previous declaration of 'gb_audio_gb_get_topology' with type 'int(struct gb_connection *, struct gb_audio_topology **)' 181 | int gb_audio_gb_get_topology(struct gb_connection *connection, | ^~~~~~~~~~~~~~~~~~~~~~~~
vim +11 drivers/staging/greybus/audio_gb.c
184992e305f1de Mark Greer 2016-01-13 10 184992e305f1de Mark Greer 2016-01-13 @11 int gb_audio_gb_get_topology(struct gb_connection *connection, b554255ff0c003 Jose A. Perez de Azpillaga 2026-02-23 12 struct gb_audio_topology *topology) 184992e305f1de Mark Greer 2016-01-13 13 { b554255ff0c003 Jose A. Perez de Azpillaga 2026-02-23 14 return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, b554255ff0c003 Jose A. Perez de Azpillaga 2026-02-23 15 NULL, 0, topology, le16_to_cpu(topology->size)); 184992e305f1de Mark Greer 2016-01-13 16 } 184992e305f1de Mark Greer 2016-01-13 17 EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); 184992e305f1de Mark Greer 2016-01-13 18
The FIXME in gb_audio_probe noted that memory allocation for the topology should happen within the codec driver rather than the greybus helper.
Move the size-check and kzalloc from audio_gb.c to audio_module.c and update the function signature of gb_audio_gb_get_topology to accept the pointer. This clarifies ownership of the allocated memory.
Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202602240844.4eT24iVh-lkp@intel.com/ Signed-off-by: Jose A. Perez de Azpillaga azpijr@gmail.com --- v2: - Fixed build error by updating function prototype in audio_codec.h. - Fixed 'struct gb_audio_topology has no member named size' by passing size as an explicit argument to gb_audio_gb_get_topology(). --- drivers/staging/greybus/audio_codec.h | 2 +- drivers/staging/greybus/audio_gb.c | 33 +++----------------------- drivers/staging/greybus/audio_module.c | 27 +++++++++++++++++---- 3 files changed, 26 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6be4..2d7c3f928b1d 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -179,7 +179,7 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module);
/* protocol related */ 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 9d8994fdb41a..8459a6d6f289 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -8,38 +8,11 @@ #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) + struct gb_audio_topology *topology, 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, - NULL, 0, &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); - if (ret) { - kfree(topo); - return ret; - } - - *topology = topo; - - 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 12c376c477b3..84c591b59b4e 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -240,6 +240,8 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gbaudio_data_connection *dai, *_dai; int ret, i; struct gb_audio_topology *topology; + struct gb_audio_get_topology_size_response size_resp; + u16 size;
/* There should be at least one Management and one Data cport */ if (bundle->num_cports < 2) @@ -304,13 +306,28 @@ 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_operation_sync(gbmodule->mgmt_connection, + GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, NULL, 0, + &size_resp, sizeof(size_resp)); + if (ret) + goto disable_connection; + + size = le16_to_cpu(size_resp.size); + if (size < sizeof(*topology)) { + ret = -ENODATA; + 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); + kfree(topology); goto disable_connection; }
-- 2.53.0
On Tue, Feb 24, 2026 at 09:44:23AM +0100, Jose A. Perez de Azpillaga wrote:
The FIXME in gb_audio_probe noted that memory allocation for the topology should happen within the codec driver rather than the greybus helper.
Move the size-check and kzalloc from audio_gb.c to audio_module.c and update the function signature of gb_audio_gb_get_topology to accept the pointer. This clarifies ownership of the allocated memory.
Reported-by: kernel test robot lkp@intel.com
The kernel test robot did not ask for this patch, it reported a problem with your v1 patch.
Closes: https://lore.kernel.org/oe-kbuild-all/202602240844.4eT24iVh-lkp@intel.com/
Nor does this change fix anything.
Signed-off-by: Jose A. Perez de Azpillaga azpijr@gmail.com
v2:
- Fixed build error by updating function prototype in audio_codec.h.
- Fixed 'struct gb_audio_topology has no member named size' by passing size as an explicit argument to gb_audio_gb_get_topology().
Did you test this version as well? But step back, why is this change needed at all?
drivers/staging/greybus/audio_codec.h | 2 +- drivers/staging/greybus/audio_gb.c | 33 +++----------------------- drivers/staging/greybus/audio_module.c | 27 +++++++++++++++++---- 3 files changed, 26 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6be4..2d7c3f928b1d 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -179,7 +179,7 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module);
/* protocol related */ int gb_audio_gb_get_topology(struct gb_connection *connection,
struct gb_audio_topology **topology);
struct gb_audio_topology *topology, u16 size);
Adding a random new field to a function means that we need to look up what that value is to try to figure out what is going on. That makes things much harder overall. So did this make the code harder to understand?
Why can't the size be determined automatically by this function?
thanks,
greg k-h
Hi Greg,
Thank you for the feedback and the guidance.
You're absolutely right. I am new to kernel development, and in my attempt to satisfy an old FIXME, I created a clunkier API and added unnecessary complexity. The original implementation is much cleaner and more efficient than the change I proposed.
I am withdrawing this patch. I'm sorry for the noise, and I appreciate you taking the time to lead me in the right direction. I'll focus on finding more meaningful improvements where the logic is sound.
Best regards, Jose
-
Greg KH -
Jose A. Perez de Azpillaga -
kernel test robot