greybus-dev

greybus-dev@lists.linaro.org

17 participants
766 discussions

[RFC] greybus: support combined Host + SVC

by Ayush Singh

Hello everyone, I’ve recently been experimenting with chaining multiple Greybus nodes to a single host over I²C (QWIIC port [0]). This general idea is not new (see MicroMod Qwiic Pro Kit [1]), although those setups do not use Greybus. Since I²C does not allow a slave to initiate transfers, it is not well suited for node-initiated events (e.g. interrupts, SVC-initiated control). However, for my current use case I am primarily interested in polling-based functionality, so this limitation is acceptable. In a typical Greybus topology, an Application Processor (AP), an SVC, and one or more modules are connected via UniPro. In practice, because most application processors lack a native UniPro interface, they connect through a bridge device that also implements the SVC. For the I²C-based setup described above, I have considered the following topologies: 1. Separate co-processor (SVC/Bridge) This approach is reasonable on SoCs such as TI AM6254, which include an M4F core that can serve as the SVC/bridge and control the I²C bus. However, on devices like TI AM62L, which lack such a core, introducing an additional processor solely for Greybus does not seem justified. Also, this would make the above much less portable, since it expects a hardware component, not all BeagleBoard.org boards have. ``` +----+ +---------------+ +----------+ | AP | <--- bus ----> | SVC / Bridge | <--- I2C ----> | Module | +----+ +---------------+ +----------+ | | +----------+ `----------- I2C ----> | Module | +----------+ ``` 2. SVC per node Each node implements its own SVC. Since an I²C slave cannot initiate communication, the AP must already know the address of each SVC/module. This also seems inefficient when chaining multiple nodes. ``` +----+ +----------------+ | AP | <--- I2C ---> | SVC / Module | +----+ +----------------+ | | +----------------+ `---- I2C ----> | SVC / Module | +----------------+ ``` 3. SVC/Bridge functionality inside the AP For this use case, this seems to be the most practical option. To clarify, I am not proposing any new data paths in the Greybus pipeline. The idea is to have a reusable an SVC/bridge implementation similar to what exists in greybus-zephyr [2][3], but hosted within the AP. ``` +-------------+ +-----------+ | AP / SVC | <--- I2C ---> | Module | +-------------+ +-----------+ | | +----------+ `-- I2C ---> | Module | +----------+ ``` Before proceeding further, I would appreciate feedback on this approach—particularly whether there are concerns with option 3, or if there are alternative designs I should consider. Best regards, Ayush Singh [0]: https://www.sparkfun.com/qwiic [1]: https://www.digikey.in/en/maker/projects/micromod-qwiic-pro-kit-project-gui… [2]: https://github.com/beagleboard/greybus-zephyr/blob/main/subsys/greybus/svc.… [3]: https://github.com/beagleboard/greybus-zephyr/blob/main/subsys/greybus/apbr…

1 month

[PATCH v4 1/2] greybus: raw: fix use-after-free on cdev close

by Damien Riégel

This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y): refcount_t: underflow; use-after-free. WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 ... Call Trace: <TASK> cdev_put+0x18/0x30 __fput+0x255/0x2a0 __x64_sys_close+0x3d/0x80 do_syscall_64+0xa4/0x290 entry_SYSCALL_64_after_hwframe+0x77/0x7f The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory. To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev have been released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback. Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Reviewed-by: Johan Hovold <johan(a)kernel.org> Signed-off-by: Damien Riégel <damien.riegel(a)silabs.com> --- Changes in v4: - rebase on v7.0-rc4 -> change kzalloc into kzalloc_obj Changes in v3: - move assignment of raw->dev.parent - add Reviewed-By: Johan Hovold Changes in v2: - trim down trace in commit message to keep only the essential part - rework error paths in probe function to ensure device is always freed (set device release callback before any call to put_device) - move ida_free to release callback drivers/staging/greybus/raw.c | 69 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 3027a2c25bcd..47a984554681 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; }; struct raw_data { @@ -148,6 +147,15 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; } +static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = container_of(dev, struct gb_raw, dev); + + ida_free(&minors, MINOR(raw->dev.devt)); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -164,15 +172,30 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV; - raw = kzalloc_obj(*raw); - if (!raw) + minor = ida_alloc(&minors, GFP_KERNEL); + if (minor < 0) + return minor; + + raw = kzalloc_obj(*raw, GFP_KERNEL); + if (!raw) { + ida_free(&minors, minor); return -ENOMEM; + } + + device_initialize(&raw->dev); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_put_device; connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; } INIT_LIST_HEAD(&raw->list); @@ -181,46 +204,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, raw->connection = connection; greybus_set_drvdata(bundle, raw); - minor = ida_alloc(&minors, GFP_KERNEL); - if (minor < 0) { - retval = minor; - goto error_connection_destroy; - } - - raw->dev = MKDEV(raw_major, minor); cdev_init(&raw->cdev, &raw_fops); retval = gb_connection_enable(connection); if (retval) - goto error_remove_ida; + goto error_connection_destroy; - retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable; - raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0; -error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection); -error_remove_ida: - ida_free(&minors, minor); - error_connection_destroy: gb_connection_destroy(connection); -error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; } @@ -231,11 +234,8 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp; - // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +244,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); } /* -- 2.52.0

1 month

[PATCH v3 1/2] greybus: raw: fix use-after-free on cdev close

by Damien Riégel

This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y): refcount_t: underflow; use-after-free. WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 ... Call Trace: <TASK> cdev_put+0x18/0x30 __fput+0x255/0x2a0 __x64_sys_close+0x3d/0x80 do_syscall_64+0xa4/0x290 entry_SYSCALL_64_after_hwframe+0x77/0x7f The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory. To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev have been released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback. Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Reviewed-by: Johan Hovold <johan(a)kernel.org> Signed-off-by: Damien Riégel <damien.riegel(a)silabs.com> --- Changes in v3: - move assignment of raw->dev.parent - add Reviewed-By: Johan Hovold Changes in v2: - trim down trace in commit message to keep only the essential part - rework error paths in probe function to ensure device is always freed (set device release callback before any call to put_device) - move ida_free to release callback drivers/staging/greybus/raw.c | 67 +++++++++++++++++------------------ 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..e668438e1a2 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; }; struct raw_data { @@ -148,6 +147,15 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; } +static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = container_of(dev, struct gb_raw, dev); + + ida_free(&minors, MINOR(raw->dev.devt)); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -164,15 +172,30 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV; + minor = ida_alloc(&minors, GFP_KERNEL); + if (minor < 0) + return minor; + raw = kzalloc(sizeof(*raw), GFP_KERNEL); - if (!raw) + if (!raw) { + ida_free(&minors, minor); return -ENOMEM; + } + + device_initialize(&raw->dev); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_put_device; connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; } INIT_LIST_HEAD(&raw->list); @@ -181,46 +204,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, raw->connection = connection; greybus_set_drvdata(bundle, raw); - minor = ida_alloc(&minors, GFP_KERNEL); - if (minor < 0) { - retval = minor; - goto error_connection_destroy; - } - - raw->dev = MKDEV(raw_major, minor); cdev_init(&raw->cdev, &raw_fops); retval = gb_connection_enable(connection); if (retval) - goto error_remove_ida; + goto error_connection_destroy; - retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable; - raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0; -error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection); -error_remove_ida: - ida_free(&minors, minor); - error_connection_destroy: gb_connection_destroy(connection); -error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; } @@ -231,11 +234,8 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp; - // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +244,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); } /* -- 2.52.0

1 month, 1 week

[PATCH v2 1/2] greybus: raw: fix use-after-free on cdev close

by Damien Riégel

This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y): refcount_t: underflow; use-after-free. WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 ... Call Trace: <TASK> cdev_put+0x18/0x30 __fput+0x255/0x2a0 __x64_sys_close+0x3d/0x80 do_syscall_64+0xa4/0x290 entry_SYSCALL_64_after_hwframe+0x77/0x7f The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory. To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev have been released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback. Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel <damien.riegel(a)silabs.com> --- Changes in v2: - trim down trace in commit message to keep only the essential part - rework error paths in probe function to ensure device is always freed (set device release callback before any call to put_device) - move ida_free to release callback drivers/staging/greybus/raw.c | 67 +++++++++++++++++------------------ 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..6da878e4339 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; }; struct raw_data { @@ -148,6 +147,15 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; } +static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = container_of(dev, struct gb_raw, dev); + + ida_free(&minors, MINOR(raw->dev.devt)); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -164,63 +172,58 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV; + minor = ida_alloc(&minors, GFP_KERNEL); + if (minor < 0) + return minor; + raw = kzalloc(sizeof(*raw), GFP_KERNEL); - if (!raw) + if (!raw) { + ida_free(&minors, minor); return -ENOMEM; + } + + device_initialize(&raw->dev); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_put_device; connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; } INIT_LIST_HEAD(&raw->list); mutex_init(&raw->list_lock); raw->connection = connection; + raw->dev.parent = &connection->bundle->dev; greybus_set_drvdata(bundle, raw); - minor = ida_alloc(&minors, GFP_KERNEL); - if (minor < 0) { - retval = minor; - goto error_connection_destroy; - } - - raw->dev = MKDEV(raw_major, minor); cdev_init(&raw->cdev, &raw_fops); retval = gb_connection_enable(connection); if (retval) - goto error_remove_ida; + goto error_connection_destroy; - retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable; - raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0; -error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection); -error_remove_ida: - ida_free(&minors, minor); - error_connection_destroy: gb_connection_destroy(connection); -error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; } @@ -231,11 +234,8 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp; - // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +244,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); } /* -- 2.52.0

1 month, 1 week

[PATCH] greybus: beagleplay: bound bootloader RX buffer copy

by Pengpeng Hou

When `flashing_mode` is set, `gb_tty_receive()` routes incoming bytes to `cc1352_bootloader_rx()`. That helper appends the new bytes to the shared `rx_buffer` with `memcpy()` but does not check that the chunk fits in the remaining space first. The normal HDLC receive path already enforces `MAX_RX_HDLC`, so do the same here before appending bootloader data. If a packet would overflow the receive buffer, drop it and reset the bootloader receive state instead of copying past the end of `rx_buffer`. Signed-off-by: Pengpeng Hou <pengpeng(a)iscas.ac.cn> --- drivers/greybus/gb-beagleplay.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c index 87186f891a6a..bca3132adacd 100644 --- a/drivers/greybus/gb-beagleplay.c +++ b/drivers/greybus/gb-beagleplay.c @@ -535,6 +535,12 @@ static size_t cc1352_bootloader_rx(struct gb_beagleplay *bg, const u8 *data, int ret; size_t off = 0; + if (count > sizeof(bg->rx_buffer) - bg->rx_buffer_len) { + dev_err_ratelimited(&bg->sd->dev, "Bootloader RX buffer overflow"); + bg->rx_buffer_len = 0; + return count; + } + memcpy(bg->rx_buffer + bg->rx_buffer_len, data, count); bg->rx_buffer_len += count; -- 2.50.1 (Apple Git-155)

1 month, 1 week

[PATCH 0/7] drivers: Simplify cleanup paths using __free

by Sanjay Chitroda

From: Sanjay Chitroda <sanjayembeddedse(a)gmail.com> Hi all, This patch series replaces manual cleanup and explicit kfree() calls with the __free attribute from <linux/cleanup.h>. This modernizes the memory management style and simplifies common error paths without altering any functional behavior. The __free attribute provides automatic scope-based cleanup, making resource management clearer and reducing the chances of missing cleanup on early returns. No functional changes are intended in this series. Testing: - Compiled with W=1 - Build-tested on i86_64 Based on: <linux-v7.0-rc2> Feel free to share your valuable input in context of the cleanup API. Thanks, Sanjay Chitroda Sanjay Chitroda (7): staging: greybus: simplify cleanup using __free iio: ssp_sensors: simplify cleanup using __free iio: st_sensors: simplify cleanup using __free media: mediatek: vcodec: simplify cleanup using __free media: chips-media: coda: simplify cleanup using __free media: allegro: simplify cleanup using __free staging: rtl8723bs: simplify cleanup using __free drivers/iio/common/ssp_sensors/ssp_spi.c | 9 +- .../iio/common/st_sensors/st_sensors_core.c | 7 +- .../media/platform/allegro-dvt/allegro-core.c | 95 +++++-------------- .../platform/chips-media/coda/coda-bit.c | 4 +- .../platform/chips-media/coda/coda-jpeg.c | 39 ++++---- .../mediatek/vcodec/common/mtk_vcodec_dbgfs.c | 3 +- drivers/staging/greybus/camera.c | 27 ++---- drivers/staging/greybus/loopback.c | 35 +++----- drivers/staging/greybus/raw.c | 6 +- .../staging/rtl8723bs/hal/rtl8723b_hal_init.c | 13 +-- drivers/staging/rtl8723bs/hal/sdio_ops.c | 37 ++------ 11 files changed, 78 insertions(+), 197 deletions(-) -- 2.34.1

1 month, 1 week

[PATCH] staging: greybus: audio: use sysfs_emit() in show functions

by Gabriel Rondon

Replace sprintf() with sysfs_emit() in all sysfs attribute show functions. sysfs_emit() is aware of the sysfs buffer page size limit and should be used instead of sprintf() for sysfs show callbacks to prevent potential buffer overflows. Also add the missing trailing newline to each output, which is the standard convention for sysfs attributes. Signed-off-by: Gabriel Rondon <grondon(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 e87b82ca6..f22ee73eb 100644 --- a/drivers/staging/greybus/audio_manager_module.c +++ b/drivers/staging/greybus/audio_manager_module.c @@ -76,7 +76,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 sprintf(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 = @@ -85,7 +85,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 sprintf(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 = @@ -94,7 +94,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 sprintf(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 = @@ -104,7 +104,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 sprintf(buf, "%d", module->desc.intf_id); + return sysfs_emit(buf, "%d\n", module->desc.intf_id); } static struct gb_audio_manager_module_attribute @@ -115,7 +115,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 sprintf(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 @@ -126,7 +126,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 sprintf(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.33.0

1 month, 1 week

[PATCH] staging: greybus: gpio: add comment to mutex definition

by Rahul Joshi

The irq_lock mutex is missing a comment describing what it protects, which is required by kernel coding style. Add a comment clarifying that it serializes IRQ bus lock/unlock operations used to defer and sync pending IRQ type and mask changes to hardware. Signed-off-by: Rahul Joshi <rj5547884(a)gmail.com> --- drivers/staging/greybus/gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c index 12185f7a982c..89c15b804b2a 100644 --- a/drivers/staging/greybus/gpio.c +++ b/drivers/staging/greybus/gpio.c @@ -39,7 +39,7 @@ struct gb_gpio_controller { struct gpio_chip chip; struct irq_chip irqc; - struct mutex irq_lock; + struct mutex irq_lock; /* protects irq bus operations */ }; static struct gpio_chip *irq_data_to_gpio_chip(struct irq_data *d) -- 2.34.1

1 month, 1 week

[PATCH] staging: greybus: replace DEFINE_IDR with DEFINE_XARRAY_ALLOC in uart.c

by Oskar Ray-Frayssinet

Replace deprecated DEFINE_IDR and idr_* functions with the modern DEFINE_XARRAY_ALLOC and xa_* equivalents in the greybus uart driver. Signed-off-by: Oskar Ray-Frayssinet <rayfraytech(a)gmail.com> --- drivers/staging/greybus/uart.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..6af0a3b4d2c4 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -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) { @@ -360,12 +360,15 @@ 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); + ret = xa_alloc(&tty_minors, &minor, gb_tty, + XA_LIMIT(0, GB_NUM_MINORS - 1), GFP_KERNEL); mutex_unlock(&table_lock); - if (minor >= 0) - gb_tty->minor = minor; + if (ret) + return ret; + gb_tty->minor = minor; return minor; } @@ -375,7 +378,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 +987,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.43.0

1 month, 1 week

[PATCH 1/2 RESEND] greybus: raw: fix use-after-free on cdev close

by Damien Riégel

This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y): [ 78.451062] refcount_t: underflow; use-after-free. [ 78.451352] WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 [ 78.451698] Modules linked in: gb_raw(C) [ 78.451881] CPU: 0 UID: 0 PID: 139 Comm: raw_chardev_tes Tainted: G WC 6.18.0-rc4 #212 PREEMPT(voluntary) [ 78.452386] Tainted: [W]=WARN, [C]=CRAP [ 78.452560] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 78.453049] RIP: 0010:refcount_warn_saturate+0xd0/0x130 [ 78.453311] Code: 0b 90 90 c3 cc cc cc cc 80 3d 4f ec 1d 01 00 0f 85 75 ff ff ff c6 05 42 ec 1d 01 01 90 48 c7 c7 e8 5b cb b4 e8 31f [ 78.453953] RSP: 0018:ffffaa0f80203ed0 EFLAGS: 00010282 [ 78.454251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.454472] RDX: 0000000000000000 RSI: ffffaa0f80203d68 RDI: 00000000ffffdfff [ 78.454690] RBP: 00000000040e001f R08: 00000000ffffdfff R09: ffffffffb510c008 [ 78.454899] R10: ffffffffb505c060 R11: 0000000063666572 R12: ffff938dc210b468 [ 78.455279] R13: ffff938dc1f5e1a0 R14: ffff938dc14710c0 R15: 0000000000000000 [ 78.455549] FS: 00007f2f22741740(0000) GS:ffff938e11fbc000(0000) knlGS:0000000000000000 [ 78.455806] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.456129] CR2: 00007f2f228c89c3 CR3: 00000000020d0000 CR4: 00000000000006f0 [ 78.456786] Call Trace: [ 78.456936] <TASK> [ 78.457069] cdev_put+0x18/0x30 [ 78.457230] __fput+0x255/0x2a0 [ 78.457372] __x64_sys_close+0x3d/0x80 [ 78.457544] do_syscall_64+0xa4/0x290 [ 78.457697] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 78.457883] RIP: 0033:0x7f2f227d1cc7 [ 78.458097] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 78.458692] RSP: 002b:00007fffab36fb50 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 78.459155] RAX: ffffffffffffffda RBX: 00007f2f22741740 RCX: 00007f2f227d1cc7 [ 78.459400] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003 [ 78.459648] RBP: 00007fffab36fba8 R08: 0000000000000000 R09: 0000000000000000 [ 78.459899] R10: 0000000000000000 R11: 0000000000000202 R12: 0000558298427128 [ 78.460212] R13: 00007f2f227416d0 R14: 00005582c72cf320 R15: 00005582c72cf320 [ 78.460470] </TASK> [ 78.460571] ---[ end trace 0000000000000000 ]--- The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory. To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev are not released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback. Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel <damien.riegel(a)silabs.com> --- resend: added linux-staging as Cc, this list was not part of the first submission. drivers/staging/greybus/raw.c | 49 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..b92214f97e3 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; }; struct raw_data { @@ -148,6 +147,13 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; } +static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = dev_get_drvdata(dev); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -168,11 +174,14 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (!raw) return -ENOMEM; + device_initialize(&raw->dev); + dev_set_drvdata(&raw->dev, raw); + connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; } INIT_LIST_HEAD(&raw->list); @@ -187,29 +196,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, goto error_connection_destroy; } - raw->dev = MKDEV(raw_major, minor); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &connection->bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_remove_ida; + cdev_init(&raw->cdev, &raw_fops); retval = gb_connection_enable(connection); if (retval) goto error_remove_ida; - retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable; - raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0; -error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection); @@ -219,8 +225,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, error_connection_destroy: gb_connection_destroy(connection); -error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; } @@ -231,11 +237,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp; - // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); + ida_free(&minors, MINOR(raw->dev.devt)); gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +248,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); } /* -- 2.52.0

1 month, 1 week

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev