Re: [PATCH 01/15] media: intel/ipu6: add Intel IPU6 PCI device driver

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Andreas,

On 10/16/23 5:39 PM, Andreas Helbech Kleist wrote:
> On Tue, 2023-10-03 at 12:12 +0200, Andreas Helbech Kleist wrote:
>> On Thu, 2023-07-27 at 15:15 +0800, bingbu.cao@intel.com wrote:
>>> From: Bingbu Cao <bingbu.cao@intel.com>
> ...
>>> +static void ipu6_pci_remove(struct pci_dev *pdev)
>>> +{
>> ...
>>> +       ipu6_bus_del_devices(pdev);
>> ...
>>> +       ipu6_mmu_cleanup(isp->psys->mmu);
>>> +       ipu6_mmu_cleanup(isp->isys->mmu);
>>
>> I think ipu6_mmu_cleanup() should be done before
>> ipu6_bus_del_devices()
>> like in the ipu6_pci_probe() error path.
> 

Thank you for pointing out this issue.

> Scratch that, it also causes issues (because isys_remove frees stuff in
> the MMU).

What stuff in the mmu was freed in isys_remove()?

> 
> A fix that seems to work for me is to save the isp->isys->mmu pointer
> before calling ipu6_bus_del_devices, and then use that pointer when
> calling ipu6_mmu_cleanup.
> 
> diff --git
> a/drivers/media/pci/intel/ipu6/ipu6.cqb/drivers/media/pci/intel/ipu6/ip
> u6.c
> index 59ecefbcb56c..6333a4674d33 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6.c
> @@ -812,11 +812,13 @@ static int ipu6_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  static void ipu6_pci_remove(struct pci_dev *pdev)
>  {
>         struct ipu6_device *isp = pci_get_drvdata(pdev);
> +       struct ipu6_mmu *isys_mmu = isp->isys->mmu;
>  
>         ipu6_cpd_free_pkg_dir(isp->psys);
>  
>         ipu6_buttress_unmap_fw_image(isp->psys, &isp->psys->fw_sgt);
>  
> +
>         ipu6_bus_del_devices(pdev);
>  
>         pm_runtime_forbid(&pdev->dev);
> @@ -830,7 +832,7 @@ static void ipu6_pci_remove(struct pci_dev *pdev)
>         release_firmware(isp->cpd_fw);
>  
>         ipu6_mmu_cleanup(isp->psys->mmu);
> -       ipu6_mmu_cleanup(isp->isys->mmu);
> +       ipu6_mmu_cleanup(isys_mmu);
>  }
>  
>  static void ipu6_pci_reset_prepare(struct pci_dev *pdev)
> 

-- 
Best regards,
Bingbu Cao