Re: [PATCH v9 5/6] iommu/dma: Allow a single FQ in addition to per-CPU FQs

Robin Murphy
2023-05-23T12:16:58Z

On 2023-05-23 13:02, Niklas Schnelle wrote:
[...]
>>> +static void fq_flush_single(struct iommu_dma_cookie *cookie)
>>> +{
>>> + struct iova_fq *fq = cookie->single_fq;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&fq->lock, flags);
>>> + fq_ring_free(cookie, fq);
>>> + spin_unlock_irqrestore(&fq->lock, flags)
>>
>> Nit: this should clearly just be a self-locked version of fq_ring_free()
>> that takes fq as an argument, then both the new case and the existing
>> loop body become trivial one-line calls.
>
> Sure will do. Just one question about names. As an example
> pci_reset_function_locked() means that the relevant lock is already
> taken with pci_reset_function() adding the lock/unlock. In your wording
> the implied function names sound the other way around. I can't find
> anything similar in drivers/iommu so would you mind going the PCI way
> and having:
>
> fq_ring_free_locked(): Called in queue_iova() with the lock held
> fr_ring_free(): Called in fq_flush_timeout() takes the lock itself
>
> Or maybe I'm just biased because I've used the PCI ..locked() functions
> before and there is a better convention.

Yes, that's the form that's most familiar to me too - sorry I failed to
express it clearly :)

Thanks,
Robin.