| Message ID | 20230208212712.3184953-3-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand |
Hi Sakari,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/ACPI-property-Parse-data-node-string-references-in-properties/20230209-053017
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20230208212712.3184953-3-sakari.ailus%40linux.intel.com
patch subject: [PATCH v5 2/8] ACPI: property: Parse _CRS CSI-2 descriptor
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20230209/202302091722.YCT7d1kl-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202302091722.YCT7d1kl-lkp@intel.com
smatch warnings:
drivers/acpi/mipi.c:241 acpi_crs_csi2_alloc_fill_swnodes() warn: variable dereferenced before check 'ads' (see line 237)
vim +/ads +241 drivers/acpi/mipi.c
06167f6b99bad9 Sakari Ailus 2023-02-08 206 void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle handle)
06167f6b99bad9 Sakari Ailus 2023-02-08 207 {
06167f6b99bad9 Sakari Ailus 2023-02-08 208 struct acpi_device_software_nodes *ads;
06167f6b99bad9 Sakari Ailus 2023-02-08 209 struct crs_csi2_swnodes *swnodes;
06167f6b99bad9 Sakari Ailus 2023-02-08 210 size_t alloc_size;
06167f6b99bad9 Sakari Ailus 2023-02-08 211 unsigned int i;
06167f6b99bad9 Sakari Ailus 2023-02-08 212 bool overflow;
06167f6b99bad9 Sakari Ailus 2023-02-08 213 void *end;
06167f6b99bad9 Sakari Ailus 2023-02-08 214
06167f6b99bad9 Sakari Ailus 2023-02-08 215 /*
06167f6b99bad9 Sakari Ailus 2023-02-08 216 * Allocate memory for ports, node pointers (number of nodes +
06167f6b99bad9 Sakari Ailus 2023-02-08 217 * 1 (guardian), nodes (root + number of ports * 2 (for for
06167f6b99bad9 Sakari Ailus 2023-02-08 218 * every port there is an endpoint)).
06167f6b99bad9 Sakari Ailus 2023-02-08 219 */
06167f6b99bad9 Sakari Ailus 2023-02-08 220 overflow = check_mul_overflow(sizeof(*ads->ports) +
06167f6b99bad9 Sakari Ailus 2023-02-08 221 sizeof(*ads->nodes) * 2 +
06167f6b99bad9 Sakari Ailus 2023-02-08 222 sizeof(*ads->nodeptrs) * 2,
06167f6b99bad9 Sakari Ailus 2023-02-08 223 ports_count, &alloc_size);
06167f6b99bad9 Sakari Ailus 2023-02-08 224 overflow = overflow ||
06167f6b99bad9 Sakari Ailus 2023-02-08 225 check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
06167f6b99bad9 Sakari Ailus 2023-02-08 226 sizeof(*ads->nodeptrs) * 2,
06167f6b99bad9 Sakari Ailus 2023-02-08 227 alloc_size, &alloc_size);
06167f6b99bad9 Sakari Ailus 2023-02-08 228 if (overflow) {
06167f6b99bad9 Sakari Ailus 2023-02-08 229 acpi_handle_warn(handle,
06167f6b99bad9 Sakari Ailus 2023-02-08 230 "too many _CRS CSI2 resource handles (%zu)",
06167f6b99bad9 Sakari Ailus 2023-02-08 231 ports_count);
06167f6b99bad9 Sakari Ailus 2023-02-08 232 return;
06167f6b99bad9 Sakari Ailus 2023-02-08 233 }
06167f6b99bad9 Sakari Ailus 2023-02-08 234
06167f6b99bad9 Sakari Ailus 2023-02-08 235 swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL);
06167f6b99bad9 Sakari Ailus 2023-02-08 236 ads = kzalloc(alloc_size, GFP_KERNEL);
06167f6b99bad9 Sakari Ailus 2023-02-08 @237 ads->ports = (void *)(ads + 1);
^^^^^^^^^^
Derference
06167f6b99bad9 Sakari Ailus 2023-02-08 238 ads->nodes = (void *)(ads->ports + ports_count);
06167f6b99bad9 Sakari Ailus 2023-02-08 239 ads->nodeptrs = (void *)(ads->nodes + ports_count * 2 + 1);
06167f6b99bad9 Sakari Ailus 2023-02-08 240 end = ads->nodeptrs + ports_count * 2 + 2;
06167f6b99bad9 Sakari Ailus 2023-02-08 @241 if (!swnodes || !ads || WARN_ON((void *)ads + alloc_size != end)) {
^^^^
Checked to late.
06167f6b99bad9 Sakari Ailus 2023-02-08 242 kfree(swnodes);
06167f6b99bad9 Sakari Ailus 2023-02-08 243 kfree(ads);
06167f6b99bad9 Sakari Ailus 2023-02-08 244 acpi_handle_debug(handle,
06167f6b99bad9 Sakari Ailus 2023-02-08 245 "cannot allocate for %zu software nodes\n",
06167f6b99bad9 Sakari Ailus 2023-02-08 246 ports_count);
06167f6b99bad9 Sakari Ailus 2023-02-08 247 return;
06167f6b99bad9 Sakari Ailus 2023-02-08 248 }
06167f6b99bad9 Sakari Ailus 2023-02-08 249
06167f6b99bad9 Sakari Ailus 2023-02-08 250 ads->num_ports = ports_count;
06167f6b99bad9 Sakari Ailus 2023-02-08 251 for (i = 0; i < ports_count * 2 + 1; i++)
06167f6b99bad9 Sakari Ailus 2023-02-08 252 ads->nodeptrs[i] = &ads->nodes[i];
06167f6b99bad9 Sakari Ailus 2023-02-08 253 ads->nodeptrs[i] = NULL;
06167f6b99bad9 Sakari Ailus 2023-02-08 254 for (i = 0; i < ports_count; i++)
06167f6b99bad9 Sakari Ailus 2023-02-08 255 ads->ports[i].port_nr = NO_CSI2_PORT;
06167f6b99bad9 Sakari Ailus 2023-02-08 256 swnodes->handle = handle;
06167f6b99bad9 Sakari Ailus 2023-02-08 257 swnodes->ads = ads;
06167f6b99bad9 Sakari Ailus 2023-02-08 258 list_add(&swnodes->list, &crs_csi2_swnodes);
06167f6b99bad9 Sakari Ailus 2023-02-08 259 }
Note: The report from Dan Carpenter has not been addressed. On Wed, Feb 8, 2023 at 10:27 PM Sakari Ailus <sakari.ailus@linux.intel.com> 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. This should contain (a) pointer(s) to the relevant specification material. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/acpi/Makefile | 2 +- > drivers/acpi/internal.h | 7 + > drivers/acpi/mipi.c | 358 ++++++++++++++++++++++++++++++++++++++++ > drivers/acpi/scan.c | 16 +- > include/acpi/acpi_bus.h | 11 ++ > 5 files changed, 389 insertions(+), 5 deletions(-) > create mode 100644 drivers/acpi/mipi.c > > 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..33d50df831933 > --- /dev/null > +++ b/drivers/acpi/mipi.c > @@ -0,0 +1,358 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * MIPI DisCo for Imaging support. I do realize that the last patch in the series contains the description of this code, but it would be really helpful to put at least some of it here and update it in the following patches as the code gets modified. > + * > + * Copyright (C) 2023 Intel Corporation > + */ > + > +#include <linux/acpi.h> > +#include <linux/limits.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" > + > +/* Temporary ACPI device handle to software node data structure mapping */ > +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); > +} > + > +/* > + * Description of a _CRS CSI2 resource descriptor before software node creation > + */ > +struct crs_csi2_instance { > + struct list_head list; > + struct acpi_resource_csi2_serialbus csi2; > + acpi_handle remote_handle; > + char remote_name[]; > +}; > + > +/* Temporary list of ACPI device nodes with _CRS CSI2 resource descriptors */ > +struct crs_csi2 { > + struct list_head list; > + acpi_handle handle; > + struct list_head buses; > +}; > + > +/* > + * Context for collecting _CRS CSI2 resource descriptors during ACPI namespace > + * walk > + */ > +struct scan_check_crs_csi2_context { > + struct list_head res_head; > + acpi_handle handle; > + size_t handle_count; > +}; > + > +/* Context for scanning ACPI device nodes for _CRS CSI2 resource descriptors */ > +struct crs_csi2_all { > + struct list_head crs_csi2_head; > + size_t handle_count; > +}; > + > +/* Scan a single CSI2 resource descriptor in _CRS. */ > +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; > + > + 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; > + } > + > + if (ACPI_FAILURE(acpi_get_handle(NULL, csi2->resource_source.string_ptr, > + &remote_handle))) { > + acpi_handle_warn(inst_context->handle, > + "cannot get handle for %s\n", > + csi2->resource_source.string_ptr); > + return AE_OK; > + } > + > + /* > + * Allocate space to store the _CRS CSI2 entry and its data and add it > + * to the list. > + */ > + inst = kmalloc(struct_size(inst, remote_name, csi2->resource_source.string_length), > + GFP_KERNEL); > + if (!inst) > + return AE_OK; > + > + inst->csi2 = *csi2; > + strscpy(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_head); > + > + inst_context->handle_count++; > + > + return AE_OK; > +} > + > +/* > + * Find all devices with _CRS CSI2 resource descriptors and collect them > + * into a list for later use. > + */ > +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_head = LIST_HEAD_INIT(inst_context.res_head), > + }; > + struct crs_csi2_all *csi2_all = context; > + struct crs_csi2 *csi2; > + > + acpi_walk_resources(handle, METHOD_NAME__CRS, > + scan_check_crs_csi2_instance, &inst_context); > + > + if (list_empty(&inst_context.res_head)) > + return AE_OK; > + > + /* > + * Found entry, so allocate memory for it, fill it and add it to the > + * list. > + */ > + csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL); > + if (!csi2) > + return AE_OK; > + > + csi2->handle = handle; > + list_replace(&inst_context.res_head, &csi2->buses); > + list_add(&csi2->list, &csi2_all->crs_csi2_head); > + > + /* This handle plus remote handles in _CRS CSI2 resource descriptors */ > + csi2_all->handle_count += 1 + inst_context.handle_count; > + > + return AE_OK; > +} > + > +struct acpi_handle_ref { > + acpi_handle handle; > + unsigned int count; > +}; > + > +#define NO_CSI2_PORT (UINT_MAX - 1) > + > +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; > +} > + > +/* > + * Release entries in temporary storage of ACPI device nodes with _CRS CSI2 > + * resource descriptors. > + */ > +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); > + } > +} > + > +/* > + * Allocate memory and set up software nodes for an ACPI device with given > + * number of CSI-2 ports. > + */ > +void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle handle) > +{ > + struct acpi_device_software_nodes *ads; > + struct crs_csi2_swnodes *swnodes; > + size_t alloc_size; > + unsigned int i; > + bool overflow; > + void *end; > + > + /* > + * 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)). > + */ > + overflow = check_mul_overflow(sizeof(*ads->ports) + > + sizeof(*ads->nodes) * 2 + > + sizeof(*ads->nodeptrs) * 2, > + ports_count, &alloc_size); > + overflow = overflow || > + check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) + > + sizeof(*ads->nodeptrs) * 2, > + alloc_size, &alloc_size); > + if (overflow) { > + acpi_handle_warn(handle, > + "too many _CRS CSI2 resource handles (%zu)", > + ports_count); > + return; > + } > + > + swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL); > + ads = kzalloc(alloc_size, GFP_KERNEL); > + ads->ports = (void *)(ads + 1); > + ads->nodes = (void *)(ads->ports + ports_count); > + ads->nodeptrs = (void *)(ads->nodes + ports_count * 2 + 1); > + end = ads->nodeptrs + ports_count * 2 + 2; > + if (!swnodes || !ads || WARN_ON((void *)ads + alloc_size != end)) { > + kfree(swnodes); > + kfree(ads); > + acpi_handle_debug(handle, > + "cannot allocate for %zu software nodes\n", > + ports_count); > + return; > + } > + > + ads->num_ports = ports_count; > + for (i = 0; i < ports_count * 2 + 1; i++) > + ads->nodeptrs[i] = &ads->nodes[i]; > + ads->nodeptrs[i] = NULL; > + for (i = 0; i < ports_count; i++) > + ads->ports[i].port_nr = NO_CSI2_PORT; > + swnodes->handle = handle; > + swnodes->ads = ads; > + list_add(&swnodes->list, &crs_csi2_swnodes); > +} > + > +/** > + * acpi_bus_scan_crs_csi2 - Scan a device and its child devices for _CRS CSI-2 > + * > + * @handle: Root of the ACPI Namespace branch to scan > + * > + * This function does a number of things: > + * > + * 1. Iteratively scan all ACPI device nodes starting from a handle 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; > + struct crs_csi2_all csi2_all = { > + .crs_csi2_head = LIST_HEAD_INIT(csi2_all.crs_csi2_head), > + }; > + size_t this_count; > + unsigned int curr = 0; > + struct crs_csi2 *csi2; > + > + /* > + * Collect the devices that have a _CRS CSI-2 resource. Each CSI-2 TX > + * port has one. > + */ > + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX, > + scan_check_crs_csi2, NULL, &csi2_all, NULL); I still don't like this walk and I still think that it is avoidable. At least I don't see why it isn't avoidable ATM. > + > + /* No handles? Bail out here. */ > + if (!csi2_all.handle_count) > + return; > + > + handle_refs = kcalloc(csi2_all.handle_count + 1, sizeof(*handle_refs), > + GFP_KERNEL); > + if (!handle_refs) { > + acpi_handle_debug(handle, "no memory for %zu handle refs\n", > + csi2_all.handle_count + 1); > + return; > + } > + > + /* Associate handles to the number of references. */ > + list_for_each_entry(csi2, &csi2_all.crs_csi2_head, 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 the handles by remote so duplicates canbe easily found. */ > + sort(handle_refs, csi2_all.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 < csi2_all.handle_count + 1; curr++) { > + /* Weed out duplicate receiver devices. */ > + if (this->handle == handle_refs[curr].handle) { > + this_count += handle_refs[curr].count; > + continue; > + } > + > + acpi_crs_csi2_alloc_fill_swnodes(this_count, this->handle); > + > + this = &handle_refs[curr]; > + this_count = this->count; > + } > + > + kfree(handle_refs); > + > + crs_csi2_release(&csi2_all.crs_csi2_head); > +} > 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; > -- > 2.30.2 >
Hi Rafael, On Tue, Mar 07, 2023 at 02:38:43PM +0100, Rafael J. Wysocki wrote: > Note: The report from Dan Carpenter has not been addressed. I'll reply to Dan --- it's a false positive. > > On Wed, Feb 8, 2023 at 10:27 PM Sakari Ailus > <sakari.ailus@linux.intel.com> 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. > > This should contain (a) pointer(s) to the relevant specification material. I can add that for v6. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/acpi/Makefile | 2 +- > > drivers/acpi/internal.h | 7 + > > drivers/acpi/mipi.c | 358 ++++++++++++++++++++++++++++++++++++++++ > > drivers/acpi/scan.c | 16 +- > > include/acpi/acpi_bus.h | 11 ++ > > 5 files changed, 389 insertions(+), 5 deletions(-) > > create mode 100644 drivers/acpi/mipi.c > > > > 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..33d50df831933 > > --- /dev/null > > +++ b/drivers/acpi/mipi.c > > @@ -0,0 +1,358 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * MIPI DisCo for Imaging support. > > I do realize that the last patch in the series contains the > description of this code, but it would be really helpful to put at > least some of it here and update it in the following patches as the > code gets modified. I'll see if it could be meaningfully split, with some going to this patch. > > > + * > > + * Copyright (C) 2023 Intel Corporation > > + */ ... > > +void acpi_bus_scan_crs_csi2(acpi_handle handle) > > +{ > > + struct acpi_handle_ref *handle_refs; > > + struct acpi_handle_ref *this = NULL; > > + struct crs_csi2_all csi2_all = { > > + .crs_csi2_head = LIST_HEAD_INIT(csi2_all.crs_csi2_head), > > + }; > > + size_t this_count; > > + unsigned int curr = 0; > > + struct crs_csi2 *csi2; > > + > > + /* > > + * Collect the devices that have a _CRS CSI-2 resource. Each CSI-2 TX > > + * port has one. > > + */ > > + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX, > > + scan_check_crs_csi2, NULL, &csi2_all, NULL); > > I still don't like this walk and I still think that it is avoidable. > > At least I don't see why it isn't avoidable ATM. In acpi_scan_init(), the ACPI tables has been just obtained (via acpi_get_table()), and the second table traversal in acpi_bus_scan(), acpi_bus_check_add_1() will, in turn, create devices that further can be probed. I guess we could look at the "mipi-img-port-*" nodes right under the device to see whether the device is expected to be a CSI-2 receiver. I'm not sure if this would bring a performance improvement or further degradation. Another option could be to opportunistically initialise a device, and once encountering a "mipi-img-port-*" node in _DSD property parsing, bail out and postpone initialisation of the device on first namespace traversal. This would mean adding some extra error handling in e.g. acpi_init_device_object() callers. As the proportion of devices with CSI-2 connections compared to all devices is expected to be rather small, the second option would likely bring a performance improvement compared to the current implementation but it would be somewhat ugly to involve property parsing with this IMO.
On Tue, Mar 07, 2023 at 09:00:30PM +0200, Sakari Ailus wrote: > Another option could be to opportunistically initialise a device, and once > encountering a "mipi-img-port-*" node in _DSD property parsing, bail out > and postpone initialisation of the device on first namespace traversal. > This would mean adding some extra error handling in e.g. > acpi_init_device_object() callers. What I think could be also done is to collect the handles with these nodes to a list (requires struct list_head in struct acpi_device) in order to create the devices after tree traversal. This approach might be usable for even getting rid of the second pass altogether, with the tradeoff of requiring a little bit of extra memory.
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..33d50df831933 --- /dev/null +++ b/drivers/acpi/mipi.c @@ -0,0 +1,358 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * MIPI DisCo for Imaging support. + * + * Copyright (C) 2023 Intel Corporation + */ + +#include <linux/acpi.h> +#include <linux/limits.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" + +/* Temporary ACPI device handle to software node data structure mapping */ +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); +} + +/* + * Description of a _CRS CSI2 resource descriptor before software node creation + */ +struct crs_csi2_instance { + struct list_head list; + struct acpi_resource_csi2_serialbus csi2; + acpi_handle remote_handle; + char remote_name[]; +}; + +/* Temporary list of ACPI device nodes with _CRS CSI2 resource descriptors */ +struct crs_csi2 { + struct list_head list; + acpi_handle handle; + struct list_head buses; +}; + +/* + * Context for collecting _CRS CSI2 resource descriptors during ACPI namespace + * walk + */ +struct scan_check_crs_csi2_context { + struct list_head res_head; + acpi_handle handle; + size_t handle_count; +}; + +/* Context for scanning ACPI device nodes for _CRS CSI2 resource descriptors */ +struct crs_csi2_all { + struct list_head crs_csi2_head; + size_t handle_count; +}; + +/* Scan a single CSI2 resource descriptor in _CRS. */ +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; + + 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; + } + + if (ACPI_FAILURE(acpi_get_handle(NULL, csi2->resource_source.string_ptr, + &remote_handle))) { + acpi_handle_warn(inst_context->handle, + "cannot get handle for %s\n", + csi2->resource_source.string_ptr); + return AE_OK; + } + + /* + * Allocate space to store the _CRS CSI2 entry and its data and add it + * to the list. + */ + inst = kmalloc(struct_size(inst, remote_name, csi2->resource_source.string_length), + GFP_KERNEL); + if (!inst) + return AE_OK; + + inst->csi2 = *csi2; + strscpy(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_head); + + inst_context->handle_count++; + + return AE_OK; +} + +/* + * Find all devices with _CRS CSI2 resource descriptors and collect them + * into a list for later use. + */ +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_head = LIST_HEAD_INIT(inst_context.res_head), + }; + struct crs_csi2_all *csi2_all = context; + struct crs_csi2 *csi2; + + acpi_walk_resources(handle, METHOD_NAME__CRS, + scan_check_crs_csi2_instance, &inst_context); + + if (list_empty(&inst_context.res_head)) + return AE_OK; + + /* + * Found entry, so allocate memory for it, fill it and add it to the + * list. + */ + csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL); + if (!csi2) + return AE_OK; + + csi2->handle = handle; + list_replace(&inst_context.res_head, &csi2->buses); + list_add(&csi2->list, &csi2_all->crs_csi2_head); + + /* This handle plus remote handles in _CRS CSI2 resource descriptors */ + csi2_all->handle_count += 1 + inst_context.handle_count; + + return AE_OK; +} + +struct acpi_handle_ref { + acpi_handle handle; + unsigned int count; +}; + +#define NO_CSI2_PORT (UINT_MAX - 1) + +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; +} + +/* + * Release entries in temporary storage of ACPI device nodes with _CRS CSI2 + * resource descriptors. + */ +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); + } +} + +/* + * Allocate memory and set up software nodes for an ACPI device with given + * number of CSI-2 ports. + */ +void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle handle) +{ + struct acpi_device_software_nodes *ads; + struct crs_csi2_swnodes *swnodes; + size_t alloc_size; + unsigned int i; + bool overflow; + void *end; + + /* + * 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)). + */ + overflow = check_mul_overflow(sizeof(*ads->ports) + + sizeof(*ads->nodes) * 2 + + sizeof(*ads->nodeptrs) * 2, + ports_count, &alloc_size); + overflow = overflow || + check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) + + sizeof(*ads->nodeptrs) * 2, + alloc_size, &alloc_size); + if (overflow) { + acpi_handle_warn(handle, + "too many _CRS CSI2 resource handles (%zu)", + ports_count); + return; + } + + swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL); + ads = kzalloc(alloc_size, GFP_KERNEL); + ads->ports = (void *)(ads + 1); + ads->nodes = (void *)(ads->ports + ports_count); + ads->nodeptrs = (void *)(ads->nodes + ports_count * 2 + 1); + end = ads->nodeptrs + ports_count * 2 + 2; + if (!swnodes || !ads || WARN_ON((void *)ads + alloc_size != end)) { + kfree(swnodes); + kfree(ads); + acpi_handle_debug(handle, + "cannot allocate for %zu software nodes\n", + ports_count); + return; + } + + ads->num_ports = ports_count; + for (i = 0; i < ports_count * 2 + 1; i++) + ads->nodeptrs[i] = &ads->nodes[i]; + ads->nodeptrs[i] = NULL; + for (i = 0; i < ports_count; i++) + ads->ports[i].port_nr = NO_CSI2_PORT; + swnodes->handle = handle; + swnodes->ads = ads; + list_add(&swnodes->list, &crs_csi2_swnodes); +} + +/** + * acpi_bus_scan_crs_csi2 - Scan a device and its child devices for _CRS CSI-2 + * + * @handle: Root of the ACPI Namespace branch to scan + * + * This function does a number of things: + * + * 1. Iteratively scan all ACPI device nodes starting from a handle 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; + struct crs_csi2_all csi2_all = { + .crs_csi2_head = LIST_HEAD_INIT(csi2_all.crs_csi2_head), + }; + size_t this_count; + unsigned int curr = 0; + struct crs_csi2 *csi2; + + /* + * Collect the devices that have a _CRS CSI-2 resource. Each CSI-2 TX + * port has one. + */ + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX, + scan_check_crs_csi2, NULL, &csi2_all, NULL); + + /* No handles? Bail out here. */ + if (!csi2_all.handle_count) + return; + + handle_refs = kcalloc(csi2_all.handle_count + 1, sizeof(*handle_refs), + GFP_KERNEL); + if (!handle_refs) { + acpi_handle_debug(handle, "no memory for %zu handle refs\n", + csi2_all.handle_count + 1); + return; + } + + /* Associate handles to the number of references. */ + list_for_each_entry(csi2, &csi2_all.crs_csi2_head, 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 the handles by remote so duplicates canbe easily found. */ + sort(handle_refs, csi2_all.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 < csi2_all.handle_count + 1; curr++) { + /* Weed out duplicate receiver devices. */ + if (this->handle == handle_refs[curr].handle) { + this_count += handle_refs[curr].count; + continue; + } + + acpi_crs_csi2_alloc_fill_swnodes(this_count, this->handle); + + this = &handle_refs[curr]; + this_count = this->count; + } + + kfree(handle_refs); + + crs_csi2_release(&csi2_all.crs_csi2_head); +} 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 | 358 ++++++++++++++++++++++++++++++++++++++++ drivers/acpi/scan.c | 16 +- include/acpi/acpi_bus.h | 11 ++ 5 files changed, 389 insertions(+), 5 deletions(-) create mode 100644 drivers/acpi/mipi.c