On 21/10/17 16:47, Sinan Kaya wrote:
> Just some improvement suggestions.
>
> On 10/6/2017 9:31 AM, Jean-Philippe Brucker wrote:
>> + spin_lock(&iommu_process_lock);
>> + idr_for_each_entry(&iommu_process_idr, process, i) {
>> + if (process->pid != pid)
>> + continue;
> if you see this construct a lot, this could be a for_each_iommu_process.
>
>> +
>> + if (!iommu_process_get_locked(process)) {
>> + /* Process is defunct, create a new one */
>> + process = NULL;
>> + break;
>> + }
>> +
>> + /* Great, is it also bound to this domain? */
>> + list_for_each_entry(cur_context, &process->domains,
>> + process_head) {
>> + if (cur_context->domain != domain)
>> + continue;
> if you see this construct a lot, this could be a for_each_iommu_process_domain.
>
>> +
>> + context = cur_context;
>> + *pasid = process->pasid;
>> +
>> + /* Splendid, tell the driver and increase the ref */
>> + err = iommu_process_attach_locked(context, dev);
>> + if (err)
>> + iommu_process_put_locked(process);
>> +
>> + break;
>> + }
>> + break;
>> + }
>> + spin_unlock(&iommu_process_lock);
>> + put_pid(pid);
>> +
>> + if (context)
>> + return err;
>
> I think you should make the section above a independent function and return here when the
> context is found.
Hopefully this code will only be needed for bind(), but moving it to a
separate function should look better.
Thanks,
Jean
