[PATCH] media: ipu6: Check firmware is started before close

[PATCH] media: ipu6: Check firmware is started before close

[Arpith Alexander]

Commit 70e3fac3ec78 ("media: ipu6: Move firmware init/cleanup to RPM
callbacks") moved firmware start to RPM resume and firmware close to RPM
suspend. The suspend callback closes the firmware unconditionally.

ipu6_pci_probe() wakes the PSYS. The PSYS's parent is the ISYS, so the
PM core wakes the ISYS first. At that point the ISYS driver isn't bound
yet, as isys_probe() returns -EPROBE_DEFER until the PCI probe has
finished. isys_runtime_pm_resume() is therefore not called and no
firmware is started.

The PSYS is released with pm_runtime_put(), so the ISYS is only
suspended asynchronously, and the driver core's pm_runtime_barrier()
cancels a pending suspend request before probe. The ISYS can therefore
still be runtime active when its driver binds, although its runtime
resume callback has never run. The next runtime suspend, such as the one
the driver core requests after probe, makes isys_runtime_pm_suspend()
close the firmware, and ipu6_fw_com_close() dereferences the NULL
isys->fwctx. On a Dell XPS 13 9340 this oopsed during boot:

BUG: kernel NULL pointer dereference, address: 000000000000004c
Workqueue: pm pm_runtime_work
RIP: 0010:ipu6_fw_com_close+0x9/0x50 [intel_ipu6]
Call Trace:
  ipu6_fw_isys_close+0x29/0x90 [intel_ipu6_isys]
  isys_runtime_pm_suspend+0x4c/0xf0 [intel_ipu6_isys]
  bus_pm_runtime_suspend+0x12/0x40 [intel_ipu6]
  rpm_callback+0x5e/0x80
  rpm_suspend+0xe7/0x620
  pm_runtime_work+0x98/0xb0

Only close the firmware if it was started, i.e. when isys->fwctx is set.
The check for a failed close is unchanged. A runtime suspend without a
preceding resume then does what the suspend callback did before that
commit.

Fixes: 70e3fac3ec78 ("media: ipu6: Move firmware init/cleanup to RPM callbacks")
Signed-off-by: Arpith Alexander <arpith99@gmail.com>
---
Tested on the XPS 13 9340 (Meteor Lake): no oops on the tested boots
(the unpatched kernel oopsed on its one boot that probed the IPU6, three
boots with this patch didn't), the ISYS runtime suspends when idle and
resumes for streaming, and the camera works.

This only fixes the suspend path. The ISYS being runtime active without
isys_runtime_pm_resume() having run also affects anything else that
relies on the runtime PM status before the first suspend, e.g. starting
a stream, or ipu6_isys_isr(), which v7.3 guarded with isys->power.
Making the runtime PM state consistent when the driver binds would cover
those as well.

 drivers/media/pci/intel/ipu6/ipu6-isys.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index 094a98e31c06..9c3818c904cd 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -821,10 +821,18 @@ static int isys_runtime_pm_suspend(struct device *dev)
 	struct ipu6_device *isp = adev->isp;
 	int ret = 0;
 
-	isys->adev->auxdrv_data->fw_ops->close(isys);
+	/*
+	 * The ISYS may have been runtime resumed as the parent of the PSYS
+	 * before this driver was bound. isys_runtime_pm_resume() hasn't run
+	 * then, and there is no firmware to close.
+	 */
 	if (isys->fwctx) {
-		dev_warn(&isys->adev->auxdev.dev, "failed to close fw isys\n");
-		ret = -EIO;
+		isys->adev->auxdrv_data->fw_ops->close(isys);
+		if (isys->fwctx) {
+			dev_warn(&isys->adev->auxdev.dev,
+				 "failed to close fw isys\n");
+			ret = -EIO;
+		}
 	}
 
 	mutex_lock(&isys->mutex);

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.55.0