Re: [XEN][PATCH v5 07/17] xen/smmu: Add remove_device callback for smmu_iommu ops

[Michal Orzel <michal.orzel@amd.com>]

Hi Vikram,

On 11/04/2023 21:16, Vikram Garhwal wrote:
> 
> 
> Add remove_device callback for removing the device entry from smmu-master using
> following steps:
> 1. Find if SMMU master exists for the device node.
> 2. Remove the SMMU master
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 56 ++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 0a514821b3..14e15f1bc6 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -816,6 +816,19 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>         return 0;
>  }
> 
> +static int remove_smmu_master(struct arm_smmu_device *smmu,
> +                             struct arm_smmu_master *master)
> +{
> +       if (!smmu->masters.rb_node) {
> +               ASSERT_UNREACHABLE();
> +               return -ENOENT;
> +       }
> +
> +       rb_erase(&master->node, &smmu->masters);
> +
> +       return 0;
> +}
> +
>  static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
>                                          struct device *dev,
>                                          struct iommu_fwspec *fwspec)
> @@ -853,6 +866,32 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
>         return insert_smmu_master(smmu, master);
>  }
> 
> +static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
> +                                        struct device *dev)
> +{
> +       struct arm_smmu_master *master;
> +       struct device_node *dev_node = dev_get_dev_node(dev);
> +       int ret;
> +
> +       master = find_smmu_master(smmu, dev_node);
> +       if (master == NULL) {
> +               dev_err(dev,
> +                       "No registrations found for master device %s\n",
> +                       dev_node->name);
> +               return -EINVAL;
> +       }
> +
> +       ret = remove_smmu_master(smmu, master);
This patch looks good although I remember seeing Julien advising you that it would be beneficial
for the SMMU driver itself to check if device is not currently used before you remove it (even though
you have this check in iommu_remove_dt_device(). I could not find your answer to this.

NIT: No need for a blank line here if the next instruction is checking the ret value.

With the above things clarified:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal