2023-12-04 12:30:58

by Yi Liu

Subject: Re: [PATCH v7 05/12] iommu: Merge iopf_device_param into iommu_fault_param

On 2023/11/15 11:02, Lu Baolu wrote:
> The struct dev_iommu contains two pointers, fault_param and iopf_param.
> The fault_param pointer points to a data structure that is used to store
> pending faults that are awaiting responses. The iopf_param pointer points
> to a data structure that is used to store partial faults that are part of
> a Page Request Group.
>
> The fault_param and iopf_param pointers are essentially duplicate. This
> causes memory waste. Merge the iopf_device_param pointer into the
> iommu_fault_param pointer to consolidate the code and save memory. The
> consolidated pointer would be allocated on demand when the device driver
> enables the iopf on device, and would be freed after iopf is disabled.
>
> Signed-off-by: Lu Baolu <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Kevin Tian <[email protected]>
> Tested-by: Yan Zhao <[email protected]>
> ---
> include/linux/iommu.h | 18 ++++--
> drivers/iommu/io-pgfault.c | 113 ++++++++++++++++++-------------------
> drivers/iommu/iommu.c | 34 ++---------
> 3 files changed, 75 insertions(+), 90 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 79775859af42..108ab50da1ad 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -42,6 +42,7 @@ struct notifier_block;
> struct iommu_sva;
> struct iommu_fault_event;
> struct iommu_dma_cookie;
> +struct iopf_queue;
>
> #define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
> #define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
> @@ -590,21 +591,31 @@ struct iommu_fault_event {
> * struct iommu_fault_param - per-device IOMMU fault data
> * @handler: Callback function to handle IOMMU faults at device level
> * @data: handler private data
> - * @faults: holds the pending faults which needs response
> * @lock: protect pending faults list
> + * @dev: the device that owns this param
> + * @queue: IOPF queue
> + * @queue_list: index into queue->devices
> + * @partial: faults that are part of a Page Request Group for which the last
> + * request hasn't been submitted yet.
> + * @faults: holds the pending faults which needs response

since you already moved this line, maybe fix this typo as well.
s/needs/need/

> */
> struct iommu_fault_param {
> iommu_dev_fault_handler_t handler;
> void *data;
> + struct mutex lock;

can you share why move this line up? It results in a line move as well
in the above kdoc.

> +
> + struct device *dev;
> + struct iopf_queue *queue;
> + struct list_head queue_list;
> +
> + struct list_head partial;
> struct list_head faults;
> - struct mutex lock;
> };
>
> /**
> * struct dev_iommu - Collection of per-device IOMMU data
> *
> * @fault_param: IOMMU detected device fault reporting data
> - * @iopf_param: I/O Page Fault queue and data
> * @fwspec: IOMMU fwspec data
> * @iommu_dev: IOMMU device this device is linked to
> * @priv: IOMMU Driver private data
> @@ -620,7 +631,6 @@ struct iommu_fault_param {
> struct dev_iommu {
> struct mutex lock;
> struct iommu_fault_param *fault_param;
> - struct iopf_device_param *iopf_param;
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void *priv;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 24b5545352ae..b1cf28055525 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -25,21 +25,6 @@ struct iopf_queue {
> struct mutex lock;
> };
>
> -/**
> - * struct iopf_device_param - IO Page Fault data attached to a device
> - * @dev: the device that owns this param
> - * @queue: IOPF queue
> - * @queue_list: index into queue->devices
> - * @partial: faults that are part of a Page Request Group for which the last
> - * request hasn't been submitted yet.
> - */
> -struct iopf_device_param {
> - struct device *dev;
> - struct iopf_queue *queue;
> - struct list_head queue_list;
> - struct list_head partial;
> -};
> -
> struct iopf_fault {
> struct iommu_fault fault;
> struct list_head list;
> @@ -144,7 +129,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
> int ret;
> struct iopf_group *group;
> struct iopf_fault *iopf, *next;
> - struct iopf_device_param *iopf_param;
> + struct iommu_fault_param *iopf_param;
>
> struct device *dev = cookie;
> struct dev_iommu *param = dev->iommu;
> @@ -159,7 +144,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
> * As long as we're holding param->lock, the queue can't be unlinked
> * from the device and therefore cannot disappear.
> */
> - iopf_param = param->iopf_param;
> + iopf_param = param->fault_param;
> if (!iopf_param)
> return -ENODEV;
>
> @@ -229,14 +214,14 @@ EXPORT_SYMBOL_GPL(iommu_queue_iopf);
> int iopf_queue_flush_dev(struct device *dev)
> {
> int ret = 0;
> - struct iopf_device_param *iopf_param;
> + struct iommu_fault_param *iopf_param;
> struct dev_iommu *param = dev->iommu;
>
> if (!param)
> return -ENODEV;
>
> mutex_lock(&param->lock);
> - iopf_param = param->iopf_param;
> + iopf_param = param->fault_param;
> if (iopf_param)
> flush_workqueue(iopf_param->queue->wq);
> else
> @@ -260,7 +245,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
> int iopf_queue_discard_partial(struct iopf_queue *queue)
> {
> struct iopf_fault *iopf, *next;
> - struct iopf_device_param *iopf_param;
> + struct iommu_fault_param *iopf_param;
>
> if (!queue)
> return -EINVAL;
> @@ -287,34 +272,38 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
> */
> int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
> {
> - int ret = -EBUSY;
> - struct iopf_device_param *iopf_param;
> + int ret = 0;
> struct dev_iommu *param = dev->iommu;
> -
> - if (!param)
> - return -ENODEV;
> -
> - iopf_param = kzalloc(sizeof(*iopf_param), GFP_KERNEL);
> - if (!iopf_param)
> - return -ENOMEM;
> -
> - INIT_LIST_HEAD(&iopf_param->partial);
> - iopf_param->queue = queue;
> - iopf_param->dev = dev;
> + struct iommu_fault_param *fault_param;
>
> mutex_lock(&queue->lock);
> mutex_lock(&param->lock);
> - if (!param->iopf_param) {
> - list_add(&iopf_param->queue_list, &queue->devices);
> - param->iopf_param = iopf_param;
> - ret = 0;
> + if (param->fault_param) {
> + ret = -EBUSY;
> + goto done_unlock;
> }
> +
> + get_device(dev);

noticed the old code has this get as well. :) but still want to ask if
it is really need.

> + fault_param = kzalloc(sizeof(*fault_param), GFP_KERNEL);
> + if (!fault_param) {
> + put_device(dev);
> + ret = -ENOMEM;
> + goto done_unlock;
> + }
> +
> + mutex_init(&fault_param->lock);
> + INIT_LIST_HEAD(&fault_param->faults);
> + INIT_LIST_HEAD(&fault_param->partial);
> + fault_param->dev = dev;
> + list_add(&fault_param->queue_list, &queue->devices);
> + fault_param->queue = queue;
> +
> + param->fault_param = fault_param;
> +
> +done_unlock:
> mutex_unlock(&param->lock);
> mutex_unlock(&queue->lock);
>
> - if (ret)
> - kfree(iopf_param);
> -
> return ret;
> }
> EXPORT_SYMBOL_GPL(iopf_queue_add_device);
> @@ -330,34 +319,42 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
> */
> int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
> {
> - int ret = -EINVAL;
> + int ret = 0;
> struct iopf_fault *iopf, *next;
> - struct iopf_device_param *iopf_param;
> struct dev_iommu *param = dev->iommu;
> -
> - if (!param || !queue)
> - return -EINVAL;
> + struct iommu_fault_param *fault_param = param->fault_param;
>
> mutex_lock(&queue->lock);
> mutex_lock(&param->lock);
> - iopf_param = param->iopf_param;
> - if (iopf_param && iopf_param->queue == queue) {
> - list_del(&iopf_param->queue_list);
> - param->iopf_param = NULL;
> - ret = 0;
> + if (!fault_param) {
> + ret = -ENODEV;
> + goto unlock;
> }
> - mutex_unlock(&param->lock);
> - mutex_unlock(&queue->lock);
> - if (ret)
> - return ret;
> +
> + if (fault_param->queue != queue) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + if (!list_empty(&fault_param->faults)) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> + list_del(&fault_param->queue_list);
>
> /* Just in case some faults are still stuck */
> - list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
> + list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
> kfree(iopf);
>
> - kfree(iopf_param);
> + param->fault_param = NULL;
> + kfree(fault_param);
> + put_device(dev);
> +unlock:
> + mutex_unlock(&param->lock);
> + mutex_unlock(&queue->lock);
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(iopf_queue_remove_device);
>
> @@ -403,7 +400,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_alloc);
> */
> void iopf_queue_free(struct iopf_queue *queue)
> {
> - struct iopf_device_param *iopf_param, *next;
> + struct iommu_fault_param *iopf_param, *next;
>
> if (!queue)
> return;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f24513e2b025..9c9eacfa6761 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1326,27 +1326,18 @@ int iommu_register_device_fault_handler(struct device *dev,
> struct dev_iommu *param = dev->iommu;
> int ret = 0;
>
> - if (!param)
> + if (!param || !param->fault_param)
> return -EINVAL;
>
> mutex_lock(&param->lock);
> /* Only allow one fault handler registered for each device */
> - if (param->fault_param) {
> + if (param->fault_param->handler) {
> ret = -EBUSY;
> goto done_unlock;
> }
>
> - get_device(dev);
> - param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
> - if (!param->fault_param) {
> - put_device(dev);
> - ret = -ENOMEM;
> - goto done_unlock;
> - }
> param->fault_param->handler = handler;
> param->fault_param->data = data;
> - mutex_init(&param->fault_param->lock);
> - INIT_LIST_HEAD(&param->fault_param->faults);
>
> done_unlock:
> mutex_unlock(&param->lock);
> @@ -1367,29 +1358,16 @@ EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> int iommu_unregister_device_fault_handler(struct device *dev)
> {
> struct dev_iommu *param = dev->iommu;
> - int ret = 0;
>
> - if (!param)
> + if (!param || !param->fault_param)
> return -EINVAL;
>
> mutex_lock(&param->lock);
> -
> - if (!param->fault_param)
> - goto unlock;
> -
> - /* we cannot unregister handler if there are pending faults */
> - if (!list_empty(&param->fault_param->faults)) {
> - ret = -EBUSY;
> - goto unlock;
> - }
> -
> - kfree(param->fault_param);
> - param->fault_param = NULL;
> - put_device(dev);
> -unlock:
> + param->fault_param->handler = NULL;
> + param->fault_param->data = NULL;
> mutex_unlock(&param->lock);
>
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>

--
Regards,
Yi Liu

2023-12-05 12:01:47

by Baolu Lu

Subject: Re: [PATCH v7 05/12] iommu: Merge iopf_device_param into iommu_fault_param

On 2023/12/4 20:32, Yi Liu wrote:
> On 2023/11/15 11:02, Lu Baolu wrote:
>> The struct dev_iommu contains two pointers, fault_param and iopf_param.
>> The fault_param pointer points to a data structure that is used to store
>> pending faults that are awaiting responses. The iopf_param pointer points
>> to a data structure that is used to store partial faults that are part of
>> a Page Request Group.
>>
>> The fault_param and iopf_param pointers are essentially duplicate. This
>> causes memory waste. Merge the iopf_device_param pointer into the
>> iommu_fault_param pointer to consolidate the code and save memory. The
>> consolidated pointer would be allocated on demand when the device driver
>> enables the iopf on device, and would be freed after iopf is disabled.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> Reviewed-by: Jason Gunthorpe <[email protected]>
>> Reviewed-by: Kevin Tian <[email protected]>
>> Tested-by: Yan Zhao <[email protected]>
>> ---
>>   include/linux/iommu.h      |  18 ++++--
>>   drivers/iommu/io-pgfault.c | 113 ++++++++++++++++++-------------------
>>   drivers/iommu/iommu.c      |  34 ++---------
>>   3 files changed, 75 insertions(+), 90 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 79775859af42..108ab50da1ad 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -42,6 +42,7 @@ struct notifier_block;
>>   struct iommu_sva;
>>   struct iommu_fault_event;
>>   struct iommu_dma_cookie;
>> +struct iopf_queue;
>>   #define IOMMU_FAULT_PERM_READ    (1 << 0) /* read */
>>   #define IOMMU_FAULT_PERM_WRITE    (1 << 1) /* write */
>> @@ -590,21 +591,31 @@ struct iommu_fault_event {
>>    * struct iommu_fault_param - per-device IOMMU fault data
>>    * @handler: Callback function to handle IOMMU faults at device level
>>    * @data: handler private data
>> - * @faults: holds the pending faults which needs response
>>    * @lock: protect pending faults list
>> + * @dev: the device that owns this param
>> + * @queue: IOPF queue
>> + * @queue_list: index into queue->devices
>> + * @partial: faults that are part of a Page Request Group for which
>> the last
>> + *           request hasn't been submitted yet.
>> + * @faults: holds the pending faults which needs response
>
> since you already moved this line, maybe fix this typo as well.
> s/needs/need/
>
>>    */
>>   struct iommu_fault_param {
>>       iommu_dev_fault_handler_t handler;
>>       void *data;
>> +    struct mutex lock;
>
> can you share why move this line up? It results in a line move as well
> in the above kdoc.

This mutex protects the whole data structure. So I moved it up.

>
>> +
>> +    struct device *dev;
>> +    struct iopf_queue *queue;
>> +    struct list_head queue_list;
>> +
>> +    struct list_head partial;
>>       struct list_head faults;
>> -    struct mutex lock;
>>   };
>>   /**
>>    * struct dev_iommu - Collection of per-device IOMMU data
>>    *
>>    * @fault_param: IOMMU detected device fault reporting data
>> - * @iopf_param:     I/O Page Fault queue and data
>>    * @fwspec:     IOMMU fwspec data
>>    * @iommu_dev:     IOMMU device this device is linked to
>>    * @priv:     IOMMU Driver private data
>> @@ -620,7 +631,6 @@ struct iommu_fault_param {
>>   struct dev_iommu {
>>       struct mutex lock;
>>       struct iommu_fault_param    *fault_param;
>> -    struct iopf_device_param    *iopf_param;
>>       struct iommu_fwspec        *fwspec;
>>       struct iommu_device        *iommu_dev;
>>       void                *priv;
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 24b5545352ae..b1cf28055525 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -25,21 +25,6 @@ struct iopf_queue {
>>       struct mutex            lock;
>>   };
>> -/**
>> - * struct iopf_device_param - IO Page Fault data attached to a device
>> - * @dev: the device that owns this param
>> - * @queue: IOPF queue
>> - * @queue_list: index into queue->devices
>> - * @partial: faults that are part of a Page Request Group for which
>> the last
>> - *           request hasn't been submitted yet.
>> - */
>> -struct iopf_device_param {
>> -    struct device            *dev;
>> -    struct iopf_queue        *queue;
>> -    struct list_head        queue_list;
>> -    struct list_head        partial;
>> -};
>> -
>>   struct iopf_fault {
>>       struct iommu_fault        fault;
>>       struct list_head        list;
>> @@ -144,7 +129,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> void *cookie)
>>       int ret;
>>       struct iopf_group *group;
>>       struct iopf_fault *iopf, *next;
>> -    struct iopf_device_param *iopf_param;
>> +    struct iommu_fault_param *iopf_param;
>>       struct device *dev = cookie;
>>       struct dev_iommu *param = dev->iommu;
>> @@ -159,7 +144,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> void *cookie)
>>        * As long as we're holding param->lock, the queue can't be
>> unlinked
>>        * from the device and therefore cannot disappear.
>>        */
>> -    iopf_param = param->iopf_param;
>> +    iopf_param = param->fault_param;
>>       if (!iopf_param)
>>           return -ENODEV;
>> @@ -229,14 +214,14 @@ EXPORT_SYMBOL_GPL(iommu_queue_iopf);
>>   int iopf_queue_flush_dev(struct device *dev)
>>   {
>>       int ret = 0;
>> -    struct iopf_device_param *iopf_param;
>> +    struct iommu_fault_param *iopf_param;
>>       struct dev_iommu *param = dev->iommu;
>>       if (!param)
>>           return -ENODEV;
>>       mutex_lock(&param->lock);
>> -    iopf_param = param->iopf_param;
>> +    iopf_param = param->fault_param;
>>       if (iopf_param)
>>           flush_workqueue(iopf_param->queue->wq);
>>       else
>> @@ -260,7 +245,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
>>   int iopf_queue_discard_partial(struct iopf_queue *queue)
>>   {
>>       struct iopf_fault *iopf, *next;
>> -    struct iopf_device_param *iopf_param;
>> +    struct iommu_fault_param *iopf_param;
>>       if (!queue)
>>           return -EINVAL;
>> @@ -287,34 +272,38 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
>>    */
>>   int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
>>   {
>> -    int ret = -EBUSY;
>> -    struct iopf_device_param *iopf_param;
>> +    int ret = 0;
>>       struct dev_iommu *param = dev->iommu;
>> -
>> -    if (!param)
>> -        return -ENODEV;
>> -
>> -    iopf_param = kzalloc(sizeof(*iopf_param), GFP_KERNEL);
>> -    if (!iopf_param)
>> -        return -ENOMEM;
>> -
>> -    INIT_LIST_HEAD(&iopf_param->partial);
>> -    iopf_param->queue = queue;
>> -    iopf_param->dev = dev;
>> +    struct iommu_fault_param *fault_param;
>>       mutex_lock(&queue->lock);
>>       mutex_lock(&param->lock);
>> -    if (!param->iopf_param) {
>> -        list_add(&iopf_param->queue_list, &queue->devices);
>> -        param->iopf_param = iopf_param;
>> -        ret = 0;
>> +    if (param->fault_param) {
>> +        ret = -EBUSY;
>> +        goto done_unlock;
>>       }
>> +
>> +    get_device(dev);
>
> noticed the old code has this get as well. :) but still want to ask if
> it is really need.

It's not needed. It was part of iommu_register_device_fault_handler(),
which will be removed in the next patch.

>
>> +    fault_param = kzalloc(sizeof(*fault_param), GFP_KERNEL);
>> +    if (!fault_param) {
>> +        put_device(dev);
>> +        ret = -ENOMEM;
>> +        goto done_unlock;
>> +    }
>> +
>> +    mutex_init(&fault_param->lock);
>> +    INIT_LIST_HEAD(&fault_param->faults);
>> +    INIT_LIST_HEAD(&fault_param->partial);
>> +    fault_param->dev = dev;
>> +    list_add(&fault_param->queue_list, &queue->devices);
>> +    fault_param->queue = queue;
>> +
>> +    param->fault_param = fault_param;
>> +
>> +done_unlock:
>>       mutex_unlock(&param->lock);
>>       mutex_unlock(&queue->lock);
>> -    if (ret)
>> -        kfree(iopf_param);
>> -
>>       return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(iopf_queue_add_device);
>> @@ -330,34 +319,42 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
>>    */
>>   int iopf_queue_remove_device(struct iopf_queue *queue, struct device
>> *dev)
>>   {
>> -    int ret = -EINVAL;
>> +    int ret = 0;
>>       struct iopf_fault *iopf, *next;
>> -    struct iopf_device_param *iopf_param;
>>       struct dev_iommu *param = dev->iommu;
>> -
>> -    if (!param || !queue)
>> -        return -EINVAL;
>> +    struct iommu_fault_param *fault_param = param->fault_param;
>>       mutex_lock(&queue->lock);
>>       mutex_lock(&param->lock);
>> -    iopf_param = param->iopf_param;
>> -    if (iopf_param && iopf_param->queue == queue) {
>> -        list_del(&iopf_param->queue_list);
>> -        param->iopf_param = NULL;
>> -        ret = 0;
>> +    if (!fault_param) {
>> +        ret = -ENODEV;
>> +        goto unlock;
>>       }
>> -    mutex_unlock(&param->lock);
>> -    mutex_unlock(&queue->lock);
>> -    if (ret)
>> -        return ret;
>> +
>> +    if (fault_param->queue != queue) {
>> +        ret = -EINVAL;
>> +        goto unlock;
>> +    }
>> +
>> +    if (!list_empty(&fault_param->faults)) {
>> +        ret = -EBUSY;
>> +        goto unlock;
>> +    }
>> +
>> +    list_del(&fault_param->queue_list);
>>       /* Just in case some faults are still stuck */
>> -    list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
>> +    list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
>>           kfree(iopf);
>> -    kfree(iopf_param);
>> +    param->fault_param = NULL;
>> +    kfree(fault_param);
>> +    put_device(dev);
>> +unlock:
>> +    mutex_unlock(&param->lock);
>> +    mutex_unlock(&queue->lock);
>> -    return 0;
>> +    return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(iopf_queue_remove_device);
>> @@ -403,7 +400,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_alloc);
>>    */
>>   void iopf_queue_free(struct iopf_queue *queue)
>>   {
>> -    struct iopf_device_param *iopf_param, *next;
>> +    struct iommu_fault_param *iopf_param, *next;
>>       if (!queue)
>>           return;
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index f24513e2b025..9c9eacfa6761 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1326,27 +1326,18 @@ int iommu_register_device_fault_handler(struct
>> device *dev,
>>       struct dev_iommu *param = dev->iommu;
>>       int ret = 0;
>> -    if (!param)
>> +    if (!param || !param->fault_param)
>>           return -EINVAL;
>>       mutex_lock(&param->lock);
>>       /* Only allow one fault handler registered for each device */
>> -    if (param->fault_param) {
>> +    if (param->fault_param->handler) {
>>           ret = -EBUSY;
>>           goto done_unlock;
>>       }
>> -    get_device(dev);
>> -    param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
>> -    if (!param->fault_param) {
>> -        put_device(dev);
>> -        ret = -ENOMEM;
>> -        goto done_unlock;
>> -    }
>>       param->fault_param->handler = handler;
>>       param->fault_param->data = data;
>> -    mutex_init(&param->fault_param->lock);
>> -    INIT_LIST_HEAD(&param->fault_param->faults);
>>   done_unlock:
>>       mutex_unlock(&param->lock);
>> @@ -1367,29 +1358,16 @@
>> EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
>>   int iommu_unregister_device_fault_handler(struct device *dev)
>>   {
>>       struct dev_iommu *param = dev->iommu;
>> -    int ret = 0;
>> -    if (!param)
>> +    if (!param || !param->fault_param)
>>           return -EINVAL;
>>       mutex_lock(&param->lock);
>> -
>> -    if (!param->fault_param)
>> -        goto unlock;
>> -
>> -    /* we cannot unregister handler if there are pending faults */
>> -    if (!list_empty(&param->fault_param->faults)) {
>> -        ret = -EBUSY;
>> -        goto unlock;
>> -    }
>> -
>> -    kfree(param->fault_param);
>> -    param->fault_param = NULL;
>> -    put_device(dev);
>> -unlock:
>> +    param->fault_param->handler = NULL;
>> +    param->fault_param->data = NULL;
>>       mutex_unlock(&param->lock);
>> -    return ret;
>> +    return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>

Best regards,
baolu