Re: [PATCH v3 10/17] media: intel/ipu6: add input system driver

[Andreas Helbech Kleist <andreaskleist@gmail.com>]

Hi Bingbu,

Thank you for the patch series, I haven't had a chance to look at v3 in
detail yet, so this is just a small comment from my testing on v2 +
IPU4 hacks, which I can see is also here in v3.

 On Thu, 2024-01-11 at 14:55 +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> Input system driver do basic isys hardware setup and irq handling
> and work with fwnode and v4l2 to register the ISYS v4l2 devices.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 1353 ++++++++++++++++++++++
>  drivers/media/pci/intel/ipu6/ipu6-isys.h |  207 ++++
>  2 files changed, 1560 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.c
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.h
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c

...

> +static void isys_unregister_devices(struct ipu6_isys *isys)
> +{
> +	isys_unregister_video_devices(isys);
> +	isys_csi2_unregister_subdevices(isys);
> +	v4l2_device_unregister(&isys->v4l2_dev);
> +	media_device_unregister(&isys->media_dev);
> +	media_device_cleanup(&isys->media_dev);
> +}

...

> +static void isys_remove(struct auxiliary_device *auxdev)
> +{
> +	struct ipu6_bus_device *adev = auxdev_to_adev(auxdev);
> +	struct ipu6_isys *isys = dev_get_drvdata(&auxdev->dev);
> +	struct ipu6_device *isp = adev->isp;
> +	struct isys_fw_msgs *fwmsg, *safe;
> +	unsigned int i;
> +
> +	list_for_each_entry_safe(fwmsg, safe, &isys->framebuflist, head)
> +		dma_free_attrs(&auxdev->dev, sizeof(struct isys_fw_msgs),
> +			       fwmsg, fwmsg->dma_addr, 0);
> +
> +	list_for_each_entry_safe(fwmsg, safe, &isys->framebuflist_fw, head)
> +		dma_free_attrs(&auxdev->dev, sizeof(struct isys_fw_msgs),
> +			       fwmsg, fwmsg->dma_addr, 0);

I experienced a crash in ipu6_get_fw_msg_buf when unbinding the PCI
driver while streaming.

It happens because the above two lists are still used at this point. I
believe it is safe to free the fw msgs after the
isys_unregister_devices(isys) call below.

> +
> +	isys_unregister_devices(isys);
> +	isys_notifier_cleanup(isys);
> +
> +	cpu_latency_qos_remove_request(&isys->pm_qos);
> +
> +	if (!isp->secure_mode) {
> +		ipu6_cpd_free_pkg_dir(adev);
> +		ipu6_buttress_unmap_fw_image(adev, &adev->fw_sgt);
> +		release_firmware(adev->fw);
> +	}
> +
> +	for (i = 0; i < IPU6_ISYS_MAX_STREAMS; i++)
> +		mutex_destroy(&isys->streams[i].mutex);
> +
> +	isys_iwake_watermark_cleanup(isys);
> +	mutex_destroy(&isys->stream_mutex);
> +	mutex_destroy(&isys->mutex);
> +}

/Andreas