Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... PS11, Line 231: acpi_device_path I should have thought to mention this earlier, but since |config->remote_name| is being modified I'm confused about the lifetime of the data and what, if anything, frees the string (and whether it should be freed).
As things are with this change, |remote_name| can come from 1) being specified in devicetree.cb, 2) coming here from acpi_device_path() (which is returning the address of a static local variable and likely unsafe to store w/o wrapping within strdup()), or 3) DEFAULT_REMOTE_NAME.
Offhand I don't see anyplace where #1 is ever freed (the root |dev_root| is apparently only walked), but if it did I don't know whether this would come from deallocating an entire region or calling free(config->remote_name) (the latter would be an issue since in cases #2 and #3 the underlying memory isn't a dynamic allocation).
If the device tree isn't ever freed, then I suppose it'd be safer to wrap acpi_device_path(cio2) within a strdup() call and never have to worry about freeing it (since the allocation is apparently no worse than if the string had been specified in devicetree.cb).
In the original code change I used a local variable, instead of modifying config->remote_name itself, as a way to avoid trying to find the answer to these questions/issues.