Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2/24/22 2:00 AM, Robin Murphy wrote:
> On 2022-02-18 00:55, Lu Baolu wrote:
> [...]
>> +/**
>> + * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>> + * @group: The group.
>> + * @owner: Caller specified pointer. Used for exclusive ownership.
>> + *
>> + * This is to support backward compatibility for vfio which manages
>> + * the dma ownership in iommu_group level. New invocations on this
>> + * interface should be prohibited.
>> + */
>> +int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&group->mutex);
>> +    if (group->owner_cnt) {
> 
> To clarify the comment buried in the other thread, I really think we 
> should just unconditionally flag the error here...
> 
>> +        if (group->owner != owner) {
>> +            ret = -EPERM;
>> +            goto unlock_out;
>> +        }
>> +    } else {
>> +        if (group->domain && group->domain != group->default_domain) {
>> +            ret = -EBUSY;
>> +            goto unlock_out;
>> +        }
>> +
>> +        group->owner = owner;
>> +        if (group->domain)
>> +            __iommu_detach_group(group->domain, group);
>> +    }
>> +
>> +    group->owner_cnt++;
>> +unlock_out:
>> +    mutex_unlock(&group->mutex);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>> +
>> +/**
>> + * iommu_group_release_dma_owner() - Release DMA ownership of a group
>> + * @group: The group.
>> + *
>> + * Release the DMA ownership claimed by iommu_group_claim_dma_owner().
>> + */
>> +void iommu_group_release_dma_owner(struct iommu_group *group)
>> +{
>> +    mutex_lock(&group->mutex);
>> +    if (WARN_ON(!group->owner_cnt || !group->owner))
>> +        goto unlock_out;
>> +
>> +    if (--group->owner_cnt > 0)
>> +        goto unlock_out;
> 
> ...and equivalently just set owner_cnt directly to 0 here. I don't see a 
> realistic use-case for any driver to claim the same group more than 
> once, and allowing it in the API just feels like opening up various 
> potential corners for things to get out of sync.

Yeah! Both make sense to me. I will also drop the owner token in the API
as it's unnecessary anymore after the change.

> I think that's the only significant concern I have left with the series 
> as a whole - you can consider my other grumbles non-blocking :)

Thank you and very appreciated for your time!

Best regards,
baolu