Hi Jacob, Just two minor things below, that I noticed while using fault handlers for SVA. From my perspective the series is fine otherwise
On 11/05/2018 21:54, Jacob Pan wrote: > +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); Could we check that param->fault_param isn't NULL here, so that the driver can call this function unconditionally in a cleanup path? > + /* we cannot unregister handler if there are pending faults */ > + if (!list_empty(¶m->fault_param->faults)) { > + ret = -EBUSY; > + goto unlock; > + } > + > + kfree(param->fault_param); > + param->fault_param = NULL; > + put_device(dev); > +unlock: > + mutex_unlock(¶m->lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler); > + > + > +/** > + * iommu_report_device_fault() - Report fault event to device > + * @dev: the device > + * @evt: fault event data > + * > + * Called by IOMMU model specific drivers when fault is detected, typically > + * in a threaded IRQ handler. > + * > + * Return 0 on success, or an error. > + */ > +int iommu_report_device_fault(struct device *dev, struct > iommu_fault_event *evt) > +{ > + int ret = 0; > + struct iommu_fault_event *evt_pending; > + struct iommu_fault_param *fparam; > + > + /* iommu_param is allocated when device is added to group */ > + if (!dev->iommu_param | !evt) Should probably be || Thanks, Jean _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu