From 9cd8d095f9317b460ee0d4d003863cb26b892f60 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 5 Mar 2024 16:59:05 +0100 Subject: [PATCH 1/3] ipu6-isys: Add error logging to various probe error paths Add error logging to various probe() error paths to make it easier to debug probe() failures. Signed-off-by: Hans de Goede --- drivers/media/pci/intel/ipu-isys-csi2.c | 4 +++- drivers/media/pci/intel/ipu-isys-subdev.c | 9 +++++++-- drivers/media/pci/intel/ipu-isys.c | 16 ++++++++++++---- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/drivers/media/pci/intel/ipu-isys-csi2.c b/drivers/media/pci/intel/ipu-isys-csi2.c index 1530f50baf51..093d436b3143 100644 --- a/drivers/media/pci/intel/ipu-isys-csi2.c +++ b/drivers/media/pci/intel/ipu-isys-csi2.c @@ -532,8 +532,10 @@ int ipu_isys_csi2_init(struct ipu_isys_csi2 *csi2, NR_OF_CSI2_SINK_PADS, 0, CSI2_PAD_SOURCE, CSI2_PAD_SINK); - if (rval) + if (rval) { + dev_err(&isys->adev->dev, "ipu_isys_subdev_init() err %d\n", rval); goto fail; + } csi2->asd.pad[CSI2_PAD_SINK].flags |= MEDIA_PAD_FL_MUST_CONNECT; diff --git a/drivers/media/pci/intel/ipu-isys-subdev.c b/drivers/media/pci/intel/ipu-isys-subdev.c index 27c1c4066eb9..84fd58c42453 100644 --- a/drivers/media/pci/intel/ipu-isys-subdev.c +++ b/drivers/media/pci/intel/ipu-isys-subdev.c @@ -849,17 +849,22 @@ int ipu_isys_subdev_init(struct ipu_isys_subdev *asd, return -ENOMEM; rval = media_entity_pads_init(&asd->sd.entity, num_pads, asd->pad); - if (rval) + if (rval) { + dev_err(&asd->isys->adev->dev, "%s: media_entity_pads_init(%d) err %d\n", __func__, num_pads, rval); goto out_mutex_destroy; + } if (asd->ctrl_init) { rval = v4l2_ctrl_handler_init(&asd->ctrl_handler, nr_ctrls); - if (rval) + if (rval) { + dev_err(&asd->isys->adev->dev, "%s: v4l2_ctrl_handler_init() err %d\n", __func__, rval); goto out_media_entity_cleanup; + } asd->ctrl_init(&asd->sd); if (asd->ctrl_handler.error) { rval = asd->ctrl_handler.error; + dev_err(&asd->isys->adev->dev, "%s: ctrl_handler.error %d\n", __func__, rval); goto out_v4l2_ctrl_handler_free; } diff --git a/drivers/media/pci/intel/ipu-isys.c b/drivers/media/pci/intel/ipu-isys.c index 16ee40bd6bf5..de9652566d49 100644 --- a/drivers/media/pci/intel/ipu-isys.c +++ b/drivers/media/pci/intel/ipu-isys.c @@ -396,8 +396,10 @@ static int isys_register_subdevices(struct ipu_isys *isys) rval = ipu_isys_csi2_init(&isys->csi2[i], isys, isys->pdata->base + csi2->offsets[i], i); - if (rval) + if (rval) { + dev_err(&isys->adev->dev, "ipu_isys_csi2_init() err %d\n", rval); goto fail; + } isys->isr_csi2_bits |= IPU_ISYS_UNISPART_IRQ_CSI2(i); } @@ -1009,12 +1011,16 @@ static int isys_register_devices(struct ipu_isys *isys) goto out_v4l2_device_unregister; rval = isys_notifier_init(isys); - if (rval) + if (rval) { + dev_err(&isys->adev->dev, "isys_notifier_init() err %d\n", rval); goto out_isys_unregister_subdevices; + } rval = v4l2_device_register_subdev_nodes(&isys->v4l2_dev); - if (rval) + if (rval) { + dev_err(&isys->adev->dev, "error registering subdev nodes %d\n", rval); goto out_isys_notifier_cleanup; + } return 0; @@ -1561,8 +1567,10 @@ static int isys_probe(struct ipu_bus_device *adev) if (rval) goto out_remove_pkg_dir_shared_buffer; rval = isys_iwake_watermark_init(isys); - if (rval) + if (rval) { + dev_err(&adev->dev, "isys_iwake_watermark_init() err %d\n", rval); goto out_unregister_devices; + } ipu_mmu_hw_cleanup(adev->mmu); From e1206472e2c4886a0071ed4e70b5b11be5e08900 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 5 Mar 2024 17:05:57 +0100 Subject: [PATCH 2/3] ipu6-isys: Fix crash when ipu_isys_csi2_init() fails Calling isys_unregister_subdevices() on errors in isys_register_subdevices() leads to not yet registered subdevs getting unregistered resulting in a NULL pointer deref: [ 143.100638] BUG: kernel NULL pointer dereference, address: 0000000000000000 ... [ 143.100655] RIP: 0010:__wake_up_common+0x2a/0xa0 ... [ 143.100672] Call Trace: [ 143.100674] [ 143.100676] ? __die+0x23/0x70 [ 143.100680] ? page_fault_oops+0x171/0x4e0 [ 143.100684] ? exc_page_fault+0x7f/0x180 [ 143.100687] ? asm_exc_page_fault+0x26/0x30 [ 143.100690] ? __wake_up_common+0x2a/0xa0 [ 143.100692] __wake_up+0x36/0x60 [ 143.100697] __vb2_queue_cancel+0xd2/0x290 [videobuf2_common] [ 143.100706] vb2_core_queue_release+0x22/0x50 [videobuf2_common] [ 143.100714] isys_unregister_subdevices+0x49/0xb0 [intel_ipu6_isys] [ 143.100723] isys_probe+0x59c/0xa30 [intel_ipu6_isys] Fix this by only cleaning up subdevs which have actually been registered. Signed-off-by: Hans de Goede --- drivers/media/pci/intel/ipu-isys.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/media/pci/intel/ipu-isys.c b/drivers/media/pci/intel/ipu-isys.c index de9652566d49..24c0af786a49 100644 --- a/drivers/media/pci/intel/ipu-isys.c +++ b/drivers/media/pci/intel/ipu-isys.c @@ -382,15 +382,12 @@ static int isys_register_subdevices(struct ipu_isys *isys) const struct ipu_isys_internal_csi2_pdata *csi2 = &isys->pdata->ipdata->csi2; struct ipu_isys_csi2_be_soc *csi2_be_soc; - unsigned int i, k; - int rval; + int i = 0, k = 0, rval; isys->csi2 = devm_kcalloc(&isys->adev->dev, csi2->nports, sizeof(*isys->csi2), GFP_KERNEL); - if (!isys->csi2) { - rval = -ENOMEM; - goto fail; - } + if (!isys->csi2) + return -ENOMEM; for (i = 0; i < csi2->nports; i++) { rval = ipu_isys_csi2_init(&isys->csi2[i], isys, @@ -425,7 +422,8 @@ static int isys_register_subdevices(struct ipu_isys *isys) if (rval) { dev_info(&isys->adev->dev, "can't create link csi2->be_soc\n"); - goto fail; + isys_unregister_subdevices(isys); + return rval; } } } @@ -433,7 +431,16 @@ static int isys_register_subdevices(struct ipu_isys *isys) return 0; fail: - isys_unregister_subdevices(isys); + while (--k >= 0) { + dev_info(&isys->adev->dev, "foo %d\n", k); + ipu_isys_csi2_be_soc_cleanup(&isys->csi2_be_soc[k]); + } + + while (--i >= 0) { + dev_info(&isys->adev->dev, "bar %d\n", k); + ipu_isys_csi2_cleanup(&isys->csi2[i]); + } + return rval; } From ab8b8bea5c510342e008cae0dc8cc1b7b8c30c99 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 5 Mar 2024 18:14:50 +0100 Subject: [PATCH 3/3] ipu6-isys: Fix ipu_isys_probe() error exit resource cleanup Add a bunch of missing resource cleanups to ipu_isys_probe()'s error exit path. Add a missing mutex_destroy() to isys_remove() and drop the always true "if (isp->ipu_dir)" check before calling debugfs_remove_recursive(). This likely should have checked the debugfs_remove_recursive() argument is not NULL but that is not necessary, debugfs_remove_recursive() checks this itself. Signed-off-by: Hans de Goede --- drivers/media/pci/intel/ipu-isys.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/media/pci/intel/ipu-isys.c b/drivers/media/pci/intel/ipu-isys.c index 24c0af786a49..eb4fba3a0a80 100644 --- a/drivers/media/pci/intel/ipu-isys.c +++ b/drivers/media/pci/intel/ipu-isys.c @@ -1170,8 +1170,7 @@ static void isys_remove(struct ipu_bus_device *adev) dev_info(&adev->dev, "removed\n"); #ifdef CONFIG_DEBUG_FS - if (isp->ipu_dir) - debugfs_remove_recursive(isys->debugfsdir); + debugfs_remove_recursive(isys->debugfsdir); #endif list_for_each_entry_safe(fwmsg, safe, &isys->framebuflist, head) { @@ -1217,6 +1216,7 @@ static void isys_remove(struct ipu_bus_device *adev) mutex_destroy(&isys->stream_mutex); mutex_destroy(&isys->mutex); + mutex_destroy(&isys->lib_mutex); if (isys->short_packet_source == IPU_ISYS_SHORT_PACKET_FROM_TUNIT) { u32 trace_size = IPU_ISYS_SHORT_PACKET_TRACE_BUFFER_SIZE; @@ -1587,7 +1587,15 @@ static int isys_probe(struct ipu_bus_device *adev) isys_iwake_watermark_cleanup(isys); isys_unregister_devices(isys); out_remove_pkg_dir_shared_buffer: +#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 9, 0) cpu_latency_qos_remove_request(&isys->pm_qos); +#else + pm_qos_remove_request(&isys->pm_qos); +#endif + ipu_trace_uninit(&adev->dev); +#ifdef CONFIG_DEBUG_FS + debugfs_remove_recursive(isys->debugfsdir); +#endif if (!isp->secure_mode) ipu_cpd_free_pkg_dir(adev, isys->pkg_dir, isys->pkg_dir_dma_addr, @@ -1598,10 +1606,10 @@ static int isys_probe(struct ipu_bus_device *adev) release_firmware: if (!isp->secure_mode) release_firmware(isys->fw); - ipu_trace_uninit(&adev->dev); mutex_destroy(&isys->mutex); mutex_destroy(&isys->stream_mutex); + mutex_destroy(&isys->lib_mutex); if (isys->short_packet_source == IPU_ISYS_SHORT_PACKET_FROM_TUNIT) mutex_destroy(&isys->short_packet_tracing_mutex);