Hi Sinan, Sorry for the delay, thanks for reviewing this!
On 21/10/17 00:32, Sinan Kaya wrote: > few nits below. > >> +/* >> + * Allocate a iommu_process structure for the given task. >> + * >> + * Ideally we shouldn't need the domain parameter, since iommu_process is >> + * system-wide, but we use it to retrieve the driver's allocation ops and a >> + * PASID range. >> + */ >> +static struct iommu_process * >> +iommu_process_alloc(struct iommu_domain *domain, struct task_struct *task) >> +{ >> + int err; >> + int pasid; >> + struct iommu_process *process; >> + >> + if (WARN_ON(!domain->ops->process_alloc || !domain->ops->process_free)) >> + return ERR_PTR(-ENODEV); >> + >> + process = domain->ops->process_alloc(task); >> + if (IS_ERR(process)) >> + return process; >> + if (!process) >> + return ERR_PTR(-ENOMEM); >> + >> + process->pid = get_task_pid(task, PIDTYPE_PID); >> + process->release = domain->ops->process_free; >> + INIT_LIST_HEAD(&process->domains); >> + kref_init(&process->kref); >> + > nit, I think you should place this check right after the pid assignment. Sure >> + if (!process->pid) { >> + err = -EINVAL; >> + goto err_free_process; >> + } >> + >> + idr_preload(GFP_KERNEL); >> + spin_lock(&iommu_process_lock); >> + pasid = idr_alloc_cyclic(&iommu_process_idr, process, domain->min_pasid, >> + domain->max_pasid + 1, GFP_ATOMIC); >> + process->pasid = pasid; >> + spin_unlock(&iommu_process_lock); >> + idr_preload_end(); >> + > > nit, You can maybe return here if pasid is not negative. Ok >> + if (pasid < 0) { >> + err = pasid; >> + goto err_put_pid; >> + } >> + >> + return process; >> + >> +err_put_pid: >> + put_pid(process->pid); >> + >> +err_free_process: >> + domain->ops->process_free(process); >> + >> + return ERR_PTR(err); >> +} >> + >> +static void iommu_process_release(struct kref *kref) >> +{ >> + struct iommu_process *process; >> + void (*release)(struct iommu_process *); >> + >> + assert_spin_locked(&iommu_process_lock); >> + >> + process = container_of(kref, struct iommu_process, kref); > > if we are concerned about things going wrong (assert above), we should > also add some pointer check here (WARN) for process and release pointers as > well. We can probably get rid of this assert, as any external users releasing the process will have to go through iommu_process_put() which takes the lock. process_alloc() ensures that release isn't NULL, and process should always be valid here since we're being called from kref_put, but I should check the value in process_put. >> + release = process->release; >> + >> + WARN_ON(!list_empty(&process->domains)); >> + >> + idr_remove(&iommu_process_idr, process->pasid); >> + put_pid(process->pid); >> + release(process); >> +} >> + >> +/* >> + * Returns non-zero if a reference to the process was successfully taken. >> + * Returns zero if the process is being freed and should not be used. >> + */ >> +static int iommu_process_get_locked(struct iommu_process *process) >> +{ >> + assert_spin_locked(&iommu_process_lock); >> + >> + if (process) >> + return kref_get_unless_zero(&process->kref); >> + >> + return 0; >> +} >> + >> +static void iommu_process_put_locked(struct iommu_process *process) >> +{ >> + assert_spin_locked(&iommu_process_lock); >> + >> + kref_put(&process->kref, iommu_process_release); >> +} >> + >> +static int iommu_process_attach(struct iommu_domain *domain, struct device >> *dev, >> + struct iommu_process *process) >> +{ >> + int err; >> + int pasid = process->pasid; >> + struct iommu_context *context; >> + >> + if (WARN_ON(!domain->ops->process_attach || >> !domain->ops->process_detach)) >> + return -ENODEV; >> + >> + if (pasid > domain->max_pasid || pasid < domain->min_pasid) >> + return -ENOSPC; >> + >> + context = kzalloc(sizeof(*context), GFP_KERNEL); >> + if (!context) >> + return -ENOMEM; >> + > > devm_kzalloc maybe? I don't think we can ever leak contexts. Before the device is released, it has to detach() from the domain, which will unbind from any process (call unbind_dev_all()). Thanks, Jean _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu