greybus-dev

greybus-dev@lists.linaro.org

1 participants
806 discussions

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

by adi25charis＠gmail.com

From: Aditya Chari S <adi25charis(a)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(a)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

1 day, 17 hours

[PATCH] staging: greybus: add comments to mutex declarations

by suryasaimadhu

Add comments to mutex members in gbaudio_codec_info struct to describe what each mutex protects, as recommended by checkpatch. Signed-off-by: suryasaimadhu <suryasaimadhu369(a)gmail.com> --- drivers/staging/greybus/audio_codec.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6..326770b99 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -70,8 +70,8 @@ struct gbaudio_codec_info { struct list_head module_list; /* to maintain runtime stream params for each DAI */ struct list_head dai_list; - struct mutex lock; - struct mutex register_mutex; + struct mutex lock; /* protects module_list and dai_list */ + struct mutex register_mutex; /* protects module registration */ }; struct gbaudio_widget { -- 2.47.3

1 week

[PATCH] staging: greybus: add missing newlines in sysfs_emit calls

by suryasaimadhu

Add missing terminating newlines to sysfs_emit format strings in audio_manager_module.c as required by sysfs ABI. Signed-off-by: suryasaimadhu <suryasaimadhu369(a)gmail.com> --- drivers/staging/greybus/audio_manager_module.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/audio_manager_module.c b/drivers/staging/greybus/audio_manager_module.c index dc90cc2d2..5737f2a32 100644 --- a/drivers/staging/greybus/audio_manager_module.c +++ b/drivers/staging/greybus/audio_manager_module.c @@ -75,7 +75,7 @@ static void gb_audio_module_release(struct kobject *kobj) static ssize_t gb_audio_module_name_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%s", module->desc.name); + return sysfs_emit(buf, "%s\n", module->desc.name); } static struct gb_audio_manager_module_attribute gb_audio_module_name_attribute = @@ -84,7 +84,7 @@ static struct gb_audio_manager_module_attribute gb_audio_module_name_attribute = static ssize_t gb_audio_module_vid_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.vid); + return sysfs_emit(buf, "%d\n", module->desc.vid); } static struct gb_audio_manager_module_attribute gb_audio_module_vid_attribute = @@ -93,7 +93,7 @@ static struct gb_audio_manager_module_attribute gb_audio_module_vid_attribute = static ssize_t gb_audio_module_pid_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.pid); + return sysfs_emit(buf, "%d\n", module->desc.pid); } static struct gb_audio_manager_module_attribute gb_audio_module_pid_attribute = @@ -103,7 +103,7 @@ static ssize_t gb_audio_module_intf_id_show(struct gb_audio_manager_module *modu struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.intf_id); + return sysfs_emit(buf, "%d\n", module->desc.intf_id); } static struct gb_audio_manager_module_attribute @@ -114,7 +114,7 @@ static ssize_t gb_audio_module_ip_devices_show(struct gb_audio_manager_module *m struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "0x%X", module->desc.ip_devices); + return sysfs_emit(buf, "0x%X\n", module->desc.ip_devices); } static struct gb_audio_manager_module_attribute @@ -125,7 +125,7 @@ static ssize_t gb_audio_module_op_devices_show(struct gb_audio_manager_module *m struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "0x%X", module->desc.op_devices); + return sysfs_emit(buf, "0x%X\n", module->desc.op_devices); } static struct gb_audio_manager_module_attribute -- 2.47.3

1 week

[RFC] staging: greybus: arche still depends on missing USB3613 provider

by Pengpeng Hou

Hi, while auditing conditional provider/header contracts, I noticed that Greybus Arche still appears to describe a USB3613 provider world that is absent from current mainline. drivers/staging/greybus/Kconfig still has: depends on USB_HSIC_USB3613 || COMPILE_TEST and drivers/staging/greybus/arche-platform.c still conditionally includes the USB3613 header and calls usb3613_hub_mode_ctrl() when CONFIG_USB_HSIC_USB3613 is enabled. However, the current tree does not appear to provide include/linux/usb/usb3613.h or a Kconfig provider for USB_HSIC_USB3613. I am not sending a patch yet because this is staging/hardware policy sensitive. The possible directions seem to be: 1. restore or move the USB3613 provider/header if the hardware path is still intended; 2. remove the stale USB3613 integration path and rely on the local stub; 3. change the Kconfig dependency to describe only current supported worlds; or 4. keep the contract if an out-of-tree provider is intentionally expected. Could you advise which direction is expected for Arche? This is static source/Kconfig/header analysis only. I have not tested Arche hardware. Thanks, Pengpeng

1 week

[staging] staging: greybus: loopback: add missing comment for mutex

by Arnav Kapoor

Add a comment for the mutex as suggested by checkpatch. Signed-off-by: Arnav Kapoor <kapoorarnav43(a)gmail.com> --- drivers/staging/greybus/loopback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index ea57b1f5d..bf827e672 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -61,7 +61,7 @@ struct gb_loopback { struct dentry *file; struct kfifo kfifo_lat; - struct mutex mutex; + struct mutex mutex; /* protects the entire structure */ struct task_struct *task; struct device *dev; wait_queue_head_t wq; -- 2.53.0

1 week, 3 days

staging: greybus: audio: possible out of bounds read in the topology parser

by Maoyi Xie

Hi all, I think the Greybus audio topology parser in drivers/staging/greybus/audio_topology.c can read past the topology blob when a module reports inconsistent counts. I would appreciate it if you could take a look. gb_audio_gb_get_topology() allocates the blob to a device-reported u16 size and only checks that it is at least sizeof(struct gb_audio_topology). size = le16_to_cpu(size_resp.size); if (size < sizeof(*topo)) return -ENODATA; topo = kzalloc(size, GFP_KERNEL); After that the parser trusts the interior fields. gb_generate_enum_strings() walks one C string per item with no end pointer. items = le32_to_cpu(gbenum->items); for (i = 0; i < items; i++) { strings[i] = data; while (*data != '\0') data++; data++; } items is a device le32 and there is no guarantee the blob holds that many NUL terminated strings, so the while loop runs off the end of the allocation. The same pattern is in gbaudio_tplg_process_header(), which derives the control, widget and route offsets by adding device le32 block sizes with no check against the end of the blob, and in the csize cursor advance in gbaudio_tplg_process_kcontrols() and gbaudio_tplg_create_widget(). The whole blob is device data parsed on the probe path with no privilege needed. The attacker here is a malicious or faulty Greybus audio module on the interface. I reproduced the enum string walk under KASAN on 7.1-rc7. A blob with one NUL per item stays inside the allocation. A blob with missing terminators makes KASAN report a slab out of bounds read past the buffer. I have a partial fix that passes the buffer end into the enum walk and stops on overrun. A complete fix also needs to bound the header offsets and the csize advances against the allocation size, so I wanted to ask before sending a series. Does this look like a real bug to you? If it does I am happy to send a proper patch. The parser was added in c8e6336bb3f8 ("greybus: audio: Add topology parser for GB codec"), which I think is the right Fixes tag. Thanks, Maoyi https://maoyixie.com/

1 week, 6 days

[PATCH v2] staging: greybus: vibrator: return device_create() errors

by Alfie Varghese

gb_vibrator_probe() maps any device_create() failure to -EINVAL. This loses the real errno returned by the driver core, such as -ENOMEM, and makes probe failures harder to diagnose correctly. Return PTR_ERR(dev) instead so callers receive the actual failure reason while preserving the existing cleanup path. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- v2: No code changes, resending as a single properly versioned patch. drivers/staging/greybus/vibrator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 0ec4d317c9db..763c234fbc03 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -161,7 +161,7 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, dev = device_create(&vibrator_class, &bundle->dev, MKDEV(0, 0), vib, "vibrator%d", vib->minor); if (IS_ERR(dev)) { - retval = -EINVAL; + retval = PTR_ERR(dev); goto err_ida_remove; } vib->dev = dev; -- 2.53.0

2 weeks

[PATCH] staging: greybus: vibrator: return device_create() errors

by Alfie Varghese

gb_vibrator_probe() maps any device_create() failure to -EINVAL. This loses the real errno returned by the driver core, such as -ENOMEM, and makes probe failures harder to diagnose correctly. Return PTR_ERR(dev) instead so callers receive the actual failure reason while preserving the existing cleanup path. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- drivers/staging/greybus/vibrator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 0ec4d317c..763c234fb 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -161,7 +161,7 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, dev = device_create(&vibrator_class, &bundle->dev, MKDEV(0, 0), vib, "vibrator%d", vib->minor); if (IS_ERR(dev)) { - retval = -EINVAL; + retval = PTR_ERR(dev); goto err_ida_remove; } vib->dev = dev; -- 2.54.0.windows.1

2 weeks

[PATCH] staging: greybus: power_supply: remove unnecessary parentheses in return

by Tomasz Unger

return is not a function, so the parentheses around the returned expressions are not needed. Remove them to follow kernel coding style and silence checkpatch.pl RETURN_PARENTHESES warnings. Verified with checkpatch.pl - no errors or warnings. Compiled the power_supply object successfully. Signed-off-by: Tomasz Unger <tomasz.unger(a)yahoo.pl> --- drivers/staging/greybus/power_supply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/power_supply.c b/drivers/staging/greybus/power_supply.c index 33b7a63979d6..44bd8a72fa50 100644 --- a/drivers/staging/greybus/power_supply.c +++ b/drivers/staging/greybus/power_supply.c @@ -336,7 +336,7 @@ static int is_psy_prop_writeable(struct gb_power_supply *gbpsy, static int is_prop_valint(enum power_supply_property psp) { - return ((psp < POWER_SUPPLY_PROP_MODEL_NAME) ? 1 : 0); + return (psp < POWER_SUPPLY_PROP_MODEL_NAME) ? 1 : 0; } static void next_interval(struct gb_power_supply *gbpsy) @@ -423,7 +423,7 @@ static void check_changed(struct gb_power_supply *gbpsy, static int total_props(struct gb_power_supply *gbpsy) { /* this return the intval plus the strval properties */ - return (gbpsy->properties_count + gbpsy->properties_count_str); + return gbpsy->properties_count + gbpsy->properties_count_str; } static void prop_append(struct gb_power_supply *gbpsy, --- base-commit: 7cb1c5b32a2bfde961fff8d5204526b609bcb30a change-id: 20260615-greybus-power-supply-return-parens-b3dd9f4a87c7 Best regards, -- Tomasz Unger <tomasz.unger(a)yahoo.pl>

2 weeks, 1 day

[PATCH] staging: greybus: uart: replace IDR with XArray

by Jack Lee

DEFINE_IDR is deprecated in favor of XArray. Convert tty_minors from IDR to XArray, replacing idr_alloc, idr_find, idr_remove and idr_destroy with their xa_alloc, xa_load, xa_erase and xa_destroy equivalents. Signed-off-by: Jack Lee <skunkolee(a)gmail.com> --- drivers/staging/greybus/uart.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..30afcd0caa14 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,13 +22,13 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> #include <linux/workqueue.h> #include <linux/completion.h> #include <linux/greybus.h> +#include <linux/xarray.h> #include "gbphy.h" @@ -67,7 +67,7 @@ struct gb_tty { }; static struct tty_driver *gb_tty_driver; -static DEFINE_IDR(tty_minors); +static DEFINE_XARRAY_ALLOC(tty_minors); static DEFINE_MUTEX(table_lock); static int gb_uart_receive_data_handler(struct gb_operation *op) @@ -342,7 +342,7 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) struct gb_tty *gb_tty; mutex_lock(&table_lock); - gb_tty = idr_find(&tty_minors, minor); + gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) { @@ -359,14 +359,13 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) static int alloc_minor(struct gb_tty *gb_tty) { - int minor; + int ret; - mutex_lock(&table_lock); - minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL); - mutex_unlock(&table_lock); - if (minor >= 0) - gb_tty->minor = minor; - return minor; + ret = xa_alloc(&tty_minors, &gb_tty->minor, gb_tty, + XA_LIMIT(0, GB_NUM_MINORS - 1), GFP_KERNEL); + if (ret) + return ret; + return gb_tty->minor; } static void release_minor(struct gb_tty *gb_tty) @@ -375,7 +374,7 @@ static void release_minor(struct gb_tty *gb_tty) gb_tty->minor = 0; /* Maybe should use an invalid value instead */ mutex_lock(&table_lock); - idr_remove(&tty_minors, minor); + xa_erase(&tty_minors, minor); mutex_unlock(&table_lock); } @@ -984,7 +983,7 @@ static void gb_tty_exit(void) { tty_unregister_driver(gb_tty_driver); tty_driver_kref_put(gb_tty_driver); - idr_destroy(&tty_minors); + xa_destroy(&tty_minors); } static const struct gbphy_device_id gb_uart_id_table[] = { -- 2.54.0

2 weeks, 1 day

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev