| Message ID | 20230123134617.265382-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand |
On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote: > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera > configuration. For now, only figure out where the descriptor is present in > order to allow adding information from it to related devices. ... > + memcpy(inst->remote_name, csi2->resource_source.string_ptr, > + csi2->resource_source.string_length); Why don't we use strscpy()? Is it really strings? Or is it some abuse of the ACPI object type? ... > +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level, > + void *context, void **ret) > +{ > + struct scan_check_crs_csi2_context inst_context = { > + .handle = handle, > + .res_list = LIST_HEAD_INIT(inst_context.res_list), > + }; > + struct list_head *list = context; > + struct crs_csi2 *csi2; > + INIT_LIST_HEAD(&inst_context.res_list); Why do you need this? I don't see that variable is static... > + acpi_walk_resources(handle, METHOD_NAME__CRS, > + scan_check_crs_csi2_instance, &inst_context); > + > + if (list_empty(&inst_context.res_list)) > + return AE_OK; > + > + csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL); > + if (!csi2) > + return AE_OK; > + > + csi2->handle = handle; > + list_replace(&inst_context.res_list, &csi2->buses); > + list_add(&csi2->list, list); Hmm... Can list_swap() be used here? > + return AE_OK; > +} ... > + /* > + * Figure out how much temporary storage we need for counting > + * connections in each device. > + */ > + list_for_each_entry(csi2, &crs_csi2_handles, list) { > + struct crs_csi2_instance *inst; > + > + handle_count++; > + list_for_each_entry(inst, &csi2->buses, list) > + handle_count++; list_count_nodes()? > + } ... > + sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp, > + NULL); Yes, I would leave it on one line. ... > + if (check_mul_overflow(sizeof(*ads->ports) + > + sizeof(*ads->nodes) * 2 + > + sizeof(*ads->nodeptrs) * 2, > + (size_t)this_count, &alloc_size) || Can this_count be of size_t type from the beginning? > + check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) + > + sizeof(*ads->nodeptrs) * 2, > + alloc_size, &alloc_size)) { > + acpi_handle_warn(handle, "too many handles (%u)", > + this_count); > + continue; > + } ... > + ads->nodeptrs = (void *)(ads->nodes + > + this_count * 2 + 1); Why this is not on one line? (I have got less than 80).
Hi Andy, On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote: > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote: > > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera > > configuration. For now, only figure out where the descriptor is present in > > order to allow adding information from it to related devices. > > ... > > > + memcpy(inst->remote_name, csi2->resource_source.string_ptr, > > + csi2->resource_source.string_length); > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of > the ACPI object type? I didn't find a guarantee it would be nil terminated. Albeit I'm fine switching to strscpy() if there's such a guarantee. > > ... > > > +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level, > > + void *context, void **ret) > > +{ > > + struct scan_check_crs_csi2_context inst_context = { > > + .handle = handle, > > + .res_list = LIST_HEAD_INIT(inst_context.res_list), > > + }; > > + struct list_head *list = context; > > + struct crs_csi2 *csi2; > > > + INIT_LIST_HEAD(&inst_context.res_list); > > Why do you need this? I don't see that variable is static... Ah. It's not static. But this is a leftover from development time and can be removed, it's initialised in variable declaration. > > > + acpi_walk_resources(handle, METHOD_NAME__CRS, > > + scan_check_crs_csi2_instance, &inst_context); > > + > > + if (list_empty(&inst_context.res_list)) > > + return AE_OK; > > + > > + csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL); > > + if (!csi2) > > + return AE_OK; > > + > > + csi2->handle = handle; > > + list_replace(&inst_context.res_list, &csi2->buses); > > + list_add(&csi2->list, list); > > Hmm... Can list_swap() be used here? We're replacing an entry in a list and then adding an entry to another. How would you use list_swap() here? > > > + return AE_OK; > > +} > > ... > > > + /* > > + * Figure out how much temporary storage we need for counting > > + * connections in each device. > > + */ > > + list_for_each_entry(csi2, &crs_csi2_handles, list) { > > + struct crs_csi2_instance *inst; > > + > > + handle_count++; > > > + list_for_each_entry(inst, &csi2->buses, list) > > + handle_count++; > > list_count_nodes()? Are you suggesting adding a new list API function or using one that's not in the linux-acpi/testing branch yet? > > > + } > > ... > > > + sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp, > > + NULL); > > Yes, I would leave it on one line. Works for me. > > ... > > > + if (check_mul_overflow(sizeof(*ads->ports) + > > + sizeof(*ads->nodes) * 2 + > > + sizeof(*ads->nodeptrs) * 2, > > + (size_t)this_count, &alloc_size) || > > Can this_count be of size_t type from the beginning? I think so. > > > + check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) + > > + sizeof(*ads->nodeptrs) * 2, > > + alloc_size, &alloc_size)) { > > + acpi_handle_warn(handle, "too many handles (%u)", > > + this_count); > > + continue; > > + } > > ... > > > + ads->nodeptrs = (void *)(ads->nodes + > > + this_count * 2 + 1); > > Why this is not on one line? (I have got less than 80). Probably there was more on that line but I forgot to unwrap when removing whatever was there. I'll address this for v3.
On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote: > On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote: ... > > > + memcpy(inst->remote_name, csi2->resource_source.string_ptr, > > > + csi2->resource_source.string_length); > > > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of > > the ACPI object type? > > I didn't find a guarantee it would be nil terminated. Albeit I'm fine > switching to strscpy() if there's such a guarantee. Following this logic neither memcpy() (and especially memcpy()!) call guarantees that. But hold on, have you actually read strscpy() documentation? ... > > > + list_replace(&inst_context.res_list, &csi2->buses); > > > + list_add(&csi2->list, list); > > > > Hmm... Can list_swap() be used here? > > We're replacing an entry in a list and then adding an entry to another. How > would you use list_swap() here? I see, so it is about two different lists. ... > > > + /* > > > + * Figure out how much temporary storage we need for counting > > > + * connections in each device. > > > + */ > > > + list_for_each_entry(csi2, &crs_csi2_handles, list) { > > > + struct crs_csi2_instance *inst; > > > + > > > + handle_count++; > > > > > + list_for_each_entry(inst, &csi2->buses, list) > > > + handle_count++; > > > > list_count_nodes()? > > Are you suggesting adding a new list API function or using one that's not > in the linux-acpi/testing branch yet? It's in USB tree. I'm fine if you switch your code later on. > > > + }
Hi Andy, On Mon, Jan 23, 2023 at 07:25:25PM +0200, Andy Shevchenko wrote: > On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote: > > On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote: > > > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote: > > ... > > > > > + memcpy(inst->remote_name, csi2->resource_source.string_ptr, > > > > + csi2->resource_source.string_length); > > > > > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of > > > the ACPI object type? > > > > I didn't find a guarantee it would be nil terminated. Albeit I'm fine > > switching to strscpy() if there's such a guarantee. > > Following this logic neither memcpy() (and especially memcpy()!) call > guarantees that. But hold on, have you actually read strscpy() documentation? Yes. And there is such a guarantee, too. The string_length contains the length of the string, including the terminating nil character. I have no problem with strscpy() but it won't affect the end result in any way. :-) > > ... > > > > > + list_replace(&inst_context.res_list, &csi2->buses); > > > > + list_add(&csi2->list, list); > > > > > > Hmm... Can list_swap() be used here? > > > > We're replacing an entry in a list and then adding an entry to another. How > > would you use list_swap() here? > > I see, so it is about two different lists. > > ... > > > > > + /* > > > > + * Figure out how much temporary storage we need for counting > > > > + * connections in each device. > > > > + */ > > > > + list_for_each_entry(csi2, &crs_csi2_handles, list) { > > > > + struct crs_csi2_instance *inst; > > > > + > > > > + handle_count++; > > > > > > > + list_for_each_entry(inst, &csi2->buses, list) > > > > + handle_count++; > > > > > > list_count_nodes()? > > > > Are you suggesting adding a new list API function or using one that's not > > in the linux-acpi/testing branch yet? > > It's in USB tree. > I'm fine if you switch your code later on. Ack, I'll post a patch once this hits the linux-acpi tree. > > > > > + } >
On Tue, Jan 24, 2023 at 05:52:08PM +0200, Sakari Ailus wrote: > On Mon, Jan 23, 2023 at 07:25:25PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote: > > > On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote: > > > > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote: ... > > > > > + memcpy(inst->remote_name, csi2->resource_source.string_ptr, > > > > > + csi2->resource_source.string_length); > > > > > > > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of > > > > the ACPI object type? > > > > > > I didn't find a guarantee it would be nil terminated. Albeit I'm fine > > > switching to strscpy() if there's such a guarantee. > > > > Following this logic neither memcpy() (and especially memcpy()!) call > > guarantees that. But hold on, have you actually read strscpy() documentation? > > Yes. And there is such a guarantee, too. The string_length contains the > length of the string, including the terminating nil character. I have no > problem with strscpy() but it won't affect the end result in any way. :-) At run time won't be any differences, at reading and maintaining it has a lot. It gets rid of the confusion: "is this actually string or not?"
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index a5b649e71ab1b..a98fa1bc15548 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -37,7 +37,7 @@ acpi-$(CONFIG_ACPI_SLEEP) += proc.o # ACPI Bus and Device Drivers # acpi-y += bus.o glue.o -acpi-y += scan.o +acpi-y += scan.o mipi.o acpi-y += resource.o acpi-y += acpi_processor.o acpi-y += processor_core.o diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index ec584442fb298..1ec4aa92bf17a 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -282,4 +282,11 @@ void acpi_init_lpit(void); static inline void acpi_init_lpit(void) { } #endif +/*-------------------------------------------------------------------------- + ACPI and MIPI DisCo for Imaging conversion + -------------------------------------------------------------------------- */ + +void acpi_crs_csi2_swnodes_del_free(void); +void acpi_bus_scan_crs_csi2(acpi_handle handle); + #endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c new file mode 100644 index 0000000000000..2a8cd8ee074e3 --- /dev/null +++ b/drivers/acpi/mipi.c @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * MIPI DisCo for Imaging support. + * + * Copyright (C) 2023 Intel Corporation + */ + +#include <linux/acpi.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/overflow.h> +#include <linux/types.h> +#include <linux/slab.h> +#include <linux/sort.h> +#include <linux/string.h> + +#include "internal.h" + +struct crs_csi2_swnodes { + struct list_head list; + acpi_handle handle; + struct acpi_device_software_nodes *ads; +}; + +static LIST_HEAD(crs_csi2_swnodes); + +static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes) +{ + list_del(&swnodes->list); + kfree(swnodes); +} + +void acpi_crs_csi2_swnodes_del_free(void) +{ + struct crs_csi2_swnodes *swnodes, *swnodes_tmp; + + list_for_each_entry_safe(swnodes, swnodes_tmp, &crs_csi2_swnodes, list) + crs_csi2_swnode_del_free(swnodes); +} + +struct crs_csi2_instance { + struct list_head list; + struct acpi_resource_csi2_serialbus csi2; + acpi_handle remote_handle; + char remote_name[]; +}; + +struct crs_csi2 { + struct list_head list; + acpi_handle handle; + struct list_head buses; +}; + +struct scan_check_crs_csi2_context { + struct list_head res_list; + acpi_handle handle; +}; + +static acpi_status scan_check_crs_csi2_instance(struct acpi_resource *res, + void *context) +{ + struct scan_check_crs_csi2_context *inst_context = context; + struct acpi_resource_csi2_serialbus *csi2; + struct crs_csi2_instance *inst; + acpi_handle remote_handle; + acpi_status status; + + if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) + return AE_OK; + + csi2 = &res->data.csi2_serial_bus; + + if (csi2->type != ACPI_RESOURCE_SERIAL_TYPE_CSI2) + return AE_OK; + + if (!csi2->resource_source.string_length) { + acpi_handle_debug(inst_context->handle, + "invalid resource source string length\n"); + return AE_OK; + } + + status = acpi_get_handle(NULL, csi2->resource_source.string_ptr, + &remote_handle); + if (status != AE_OK) { + acpi_handle_warn(inst_context->handle, + "cannot get handle for %s\n", + csi2->resource_source.string_ptr); + return AE_OK; + } + + inst = kmalloc(struct_size(inst, remote_name, + csi2->resource_source.string_length), + GFP_KERNEL); + if (!inst) + return AE_OK; + + inst->csi2 = *csi2; + memcpy(inst->remote_name, csi2->resource_source.string_ptr, + csi2->resource_source.string_length); + inst->csi2.resource_source.string_ptr = inst->remote_name; + inst->remote_handle = remote_handle; + + list_add(&inst->list, &inst_context->res_list); + + return AE_OK; +} + +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level, + void *context, void **ret) +{ + struct scan_check_crs_csi2_context inst_context = { + .handle = handle, + .res_list = LIST_HEAD_INIT(inst_context.res_list), + }; + struct list_head *list = context; + struct crs_csi2 *csi2; + + INIT_LIST_HEAD(&inst_context.res_list); + + acpi_walk_resources(handle, METHOD_NAME__CRS, + scan_check_crs_csi2_instance, &inst_context); + + if (list_empty(&inst_context.res_list)) + return AE_OK; + + csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL); + if (!csi2) + return AE_OK; + + csi2->handle = handle; + list_replace(&inst_context.res_list, &csi2->buses); + list_add(&csi2->list, list); + + return AE_OK; +} + +struct acpi_handle_ref { + acpi_handle handle; + unsigned int count; +}; + +#define NO_CSI2_PORT (~1U) + +static int crs_handle_cmp(const void *__a, const void *__b) +{ + const struct acpi_handle_ref *a = __a, *b = __b; + + return a->handle < b->handle ? -1 : a->handle > b->handle; +} + +static void crs_csi2_release(struct list_head *crs_csi2_handles) +{ + struct crs_csi2 *csi2, *csi2_tmp; + + list_for_each_entry_safe(csi2, csi2_tmp, crs_csi2_handles, list) { + struct crs_csi2_instance *inst, *inst_tmp; + + list_for_each_entry_safe(inst, inst_tmp, &csi2->buses, list) { + list_del(&inst->list); + kfree(inst); + } + + list_del(&csi2->list); + kfree(csi2); + } +} + +/** + * acpi_bus_scan_crs_csi2 - Scan a device and its child devices for _CRS CSI-2 + * + * @handle: ACPI handle to scan + * + * This function does a number of things: + * + * 1. Scan an ACPI device and its children for _CRS CSI-2 instances. The + * instances are stored for later use. + * + * 2. Count how many references to other devices _CRS CSI-2 instances have in + * total. + * + * 3. Count the number of references to other devices for each _CRS CSI-2 + * instance. + * + * 4. Allocate memory for swnodes each ACPI device requires later on, and + * generate a list of such allocations. + * + * Note that struct acpi_device isn't available yet at this time. + * + * acpi_scan_lock in scan.c must be held when calling this function. + */ +void acpi_bus_scan_crs_csi2(acpi_handle handle) +{ + struct acpi_handle_ref *handle_refs; + struct acpi_handle_ref *this = NULL; + LIST_HEAD(crs_csi2_handles); + unsigned int handle_count = 0, this_count; + unsigned int curr = 0; + struct crs_csi2 *csi2; + + /* Collect the devices that have a _CRS CSI-2 resource */ + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX, + scan_check_crs_csi2, NULL, &crs_csi2_handles, NULL); + + /* + * Figure out how much temporary storage we need for counting + * connections in each device. + */ + list_for_each_entry(csi2, &crs_csi2_handles, list) { + struct crs_csi2_instance *inst; + + handle_count++; + + list_for_each_entry(inst, &csi2->buses, list) + handle_count++; + } + + /* No handles? Bail out here. */ + if (!handle_count) + return; + + handle_refs = kcalloc(handle_count + 1, sizeof(*handle_refs), + GFP_KERNEL); + if (!handle_refs) { + acpi_handle_debug(handle, "no memory for %u handle refs\n", + handle_count + 1); + return; + } + + /* Associate handles to the number of references. */ + list_for_each_entry(csi2, &crs_csi2_handles, list) { + struct crs_csi2_instance *inst; + struct acpi_handle_ref *handle_ref; + + handle_ref = &handle_refs[curr++]; + handle_ref->handle = csi2->handle; + + list_for_each_entry(inst, &csi2->buses, list) { + handle_refs[curr].handle = inst->remote_handle; + handle_refs[curr].count = 1; + handle_ref->count++; + curr++; + } + } + + handle_refs[curr].handle = NULL; + + sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp, + NULL); + + /* + * Finally count references in each handle, allocate space for device + * specific ports, properties and fill the _CRS CSI2 descriptor + * originated data. + */ + this = handle_refs; + this_count = this->count; + for (curr = 1; curr < handle_count + 1; curr++) { + struct acpi_device_software_nodes *ads; + struct crs_csi2_swnodes *swnodes; + size_t alloc_size; + void *end; + + if (this->handle == handle_refs[curr].handle) { + this_count += handle_refs[curr].count; + continue; + } + + /* + * Allocate memory for ports, node pointers (number of nodes + + * 1 (guardian), nodes (root + number of ports * 2 (for for + * every port there is an endpoint)). + */ + if (check_mul_overflow(sizeof(*ads->ports) + + sizeof(*ads->nodes) * 2 + + sizeof(*ads->nodeptrs) * 2, + (size_t)this_count, &alloc_size) || + check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) + + sizeof(*ads->nodeptrs) * 2, + alloc_size, &alloc_size)) { + acpi_handle_warn(handle, "too many handles (%u)", + this_count); + continue; + } + + swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL); + ads = kzalloc(alloc_size, GFP_KERNEL); + ads->ports = (void *)(ads + 1); + ads->nodes = (void *)(ads->ports + this_count); + ads->nodeptrs = (void *)(ads->nodes + + this_count * 2 + 1); + end = ads->nodeptrs + this_count * 2 + 2; + if (!swnodes || !ads || + WARN_ON((void *)ads + alloc_size != end)) { + kfree(swnodes); + kfree(ads); + acpi_handle_debug(handle, + "cannot allocate for %u swnodes\n", + this_count); + } else { + unsigned int i; + + ads->num_ports = this_count; + for (i = 0; i < this_count * 2 + 1; i++) + ads->nodeptrs[i] = &ads->nodes[i]; + ads->nodeptrs[i] = NULL; + for (i = 0; i < this_count; i++) + ads->ports[i].port_nr = NO_CSI2_PORT; + swnodes->handle = this->handle; + swnodes->ads = ads; + list_add(&swnodes->list, &crs_csi2_swnodes); + } + + this = &handle_refs[curr]; + this_count = this->count; + } + + kfree(handle_refs); + + crs_csi2_release(&crs_csi2_handles); +} diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 0c6f06abe3f47..50de874b8f208 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2423,9 +2423,12 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); int acpi_bus_scan(acpi_handle handle) { struct acpi_device *device = NULL; + int ret = 0; acpi_bus_scan_second_pass = false; + acpi_bus_scan_crs_csi2(handle); + /* Pass 1: Avoid enumerating devices with missing dependencies. */ if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device))) @@ -2433,13 +2436,15 @@ int acpi_bus_scan(acpi_handle handle) acpi_bus_check_add_1, NULL, NULL, (void **)&device); - if (!device) - return -ENODEV; + if (!device) { + ret = -ENODEV; + goto out_release; + } acpi_bus_attach(device, (void *)true); if (!acpi_bus_scan_second_pass) - return 0; + goto out_release; /* Pass 2: Enumerate all of the remaining devices. */ @@ -2452,7 +2457,10 @@ int acpi_bus_scan(acpi_handle handle) acpi_bus_attach(device, NULL); - return 0; +out_release: + acpi_crs_csi2_swnodes_del_free(); + + return ret; } EXPORT_SYMBOL(acpi_bus_scan); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index e44be31115a67..a05fe22c1175c 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -360,6 +360,17 @@ struct acpi_device_data { struct acpi_gpio_mapping; +struct acpi_device_software_node_port { + unsigned int port_nr; +}; + +struct acpi_device_software_nodes { + struct acpi_device_software_node_port *ports; + struct software_node *nodes; + const struct software_node **nodeptrs; + unsigned int num_ports; +}; + /* Device */ struct acpi_device { u32 pld_crc;
Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera configuration. For now, only figure out where the descriptor is present in order to allow adding information from it to related devices. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/Makefile | 2 +- drivers/acpi/internal.h | 7 + drivers/acpi/mipi.c | 320 ++++++++++++++++++++++++++++++++++++++++ drivers/acpi/scan.c | 16 +- include/acpi/acpi_bus.h | 11 ++ 5 files changed, 351 insertions(+), 5 deletions(-) create mode 100644 drivers/acpi/mipi.c