Messages in this thread | | | Date | Mon, 30 Apr 2018 11:54:56 -0700 | From | Jacob Pan <> | Subject | Re: [PATCH v4 12/22] iommu: introduce device fault report API |
| |
On Mon, 30 Apr 2018 17:53:52 +0100 Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:
> 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 agreed, same as corrections below. Thanks! > > [...] > > 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
[Jacob Pan]
| |