Messages in this thread |  | | Subject | Re: [PATCH v4 12/22] iommu: introduce device fault report API | From | Jean-Philippe Brucker <> | Date | Mon, 30 Apr 2018 17:53:52 +0100 |
| |
Hi,
I noticed a couple issues when testing
On 16/04/18 22:49, Jacob Pan wrote: > +int iommu_register_device_fault_handler(struct device *dev, > + iommu_dev_fault_handler_t handler, > + void *data) > +{ > + struct iommu_param *param = dev->iommu_param; > + > + /* > + * Device iommu_param should have been allocated when device is > + * added to its iommu_group. > + */ > + if (!param) > + return -EINVAL; > + > + /* Only allow one fault handler registered for each device */ > + if (param->fault_param) > + return -EBUSY;
Should this be inside the param lock? We probably don't expect concurrent register/unregister but it seems cleaner
> + > + mutex_lock(¶m->lock); > + get_device(dev); > + param->fault_param = > + kzalloc(sizeof(struct iommu_fault_param), GFP_ATOMIC); > + if (!param->fault_param) { > + put_device(dev); > + mutex_unlock(¶m->lock); > + return -ENOMEM; > + } > + mutex_init(¶m->fault_param->lock); > + param->fault_param->handler = handler; > + param->fault_param->data = data; > + INIT_LIST_HEAD(¶m->fault_param->faults); > + > + mutex_unlock(¶m->lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler); > + > +/** > + * iommu_unregister_device_fault_handler() - Unregister the device fault handler > + * @dev: the device > + * > + * Remove the device fault handler installed with > + * iommu_register_device_fault_handler(). > + * > + * Return 0 on success, or an error. > + */ > +int iommu_unregister_device_fault_handler(struct device *dev) > +{ > + struct iommu_param *param = dev->iommu_param; > + int ret = 0; > + > + if (!param) > + return -EINVAL; > + > + mutex_lock(¶m->lock);
We should return EINVAL here, if fault_param is NULL. That way users can call unregister_fault_handler unconditionally in their cleanup paths
> + /* we cannot unregister handler if there are pending faults */ > + if (list_empty(¶m->fault_param->faults)) {
if (!list_empty(...))
> + ret = -EBUSY; > + goto unlock; > + } > + > + list_del(¶m->fault_param->faults);
faults is the list head, no need for list_del
> + kfree(param->fault_param); > + param->fault_param = NULL; > + put_device(dev); > + > +unlock: > + mutex_unlock(¶m->lock); > + > + return 0;
return ret
Thanks, Jean
|  |