Re: [RFC/PATCH 0/6] DSPBRIDGE: fix mem+cache API issues

[Ohad Ben-Cohen <ohad@wizery.com>]

Hi Felipe,

On Sun, May 16, 2010 at 8:35 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Fri, May 14, 2010 at 10:27 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> I spent some time looking deeper into this patch series, and I have some doubts.
>>
>> On Sun, May 2, 2010 at 8:47 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> Basically you're right, but the patches currently silently allow
>>> today's user space
>>> to somehow work (because if you don't use dma bounce buffers and you feel lucky
>>> about speculative prefetching then you might get away with not calling
>>> dma unmap).
>>> I did that on purpose, to ease testing & migration, but didn't
>>> explicitely said it because
>>>  frankly it's just wrong.
>>
>> I looked into the dma code (I guess it's in arch/arm/mm/dma-mapping.c)
>> and I don't understand what dma_unmap_sg is supposed to do. I see that
>> first some "safe buffer" is searched, and if there isn't any... then
>> it depends on the direction whether something is actually done or not.
>>
>> I guess it depends whether our arch has dmabounce or not, which I have
>> no idea, but if we do, wouldn't skiping dma_unmap calls leak some
>> massive amount of "safe buffers"?
>
> Now I understand better; arch/arm/mm/dma-mapping.c will not be used
> unless CONFIG_DMABOUNCE=y, which is not the case for OMAP3.
>
> So, as you can see in arch/arm/include/asm/dma-mapping.h, dma_unmap_sg
> is a noop.

This has changed since 2.6.34, when support was added
to ARM speculative prefetching. dma_unmap_* API is now
responsible to invalidate caches when data was moved in from the device
(regardless of CONFIG_DMABOUNCE).

Dspbridge really must support this, and applications should start
using it.

Whether to deprecate the old API ? Eventually I think we should,
but probably not anytime soon. Let's take the kernel approach of minimizing
user space pain: keep the old API around, mark it as candidate for
deprecation, and rethink in the future.

Thanks,
Ohad.

>
> static inline void dma_unmap_single(struct device *dev, dma_addr_t handle,
>                size_t size, enum dma_data_direction dir)
> {
>        /* nothing to do */
> }
>
> So, in the end, PROC_BEGINDMATODSP and PROC_BEGINDMAFROMDSP would do
> exactly the same as PROC_FLUSHMEMORY and PROC_INVALIDATEMEMORY
> (dmac_op_range/outer_io_range). And
> PROC_ENDDMATODSP/PROC_ENDDMAFROMDSP don't do anything. Therefore even
> if user-space updates to the new API, there's no change.
>
> I don't think it makes sense to push for this new API since there will
> be absolutely no gain.
>
>>> What do you say about the following plan then:
>>> - I'll add a single pair of begin_dma and end_dma commands that will
>>> take the additional
>>> direction parameter as described above. I'll then covert the existing
>>> flush & invalidate commands to use this begin_dma command supplying it
>>> the appropriate direction argument.
>>
>> Sounds perfect, but I wonder if the deprecated flush & invalidate
>> would really work. See previous comments.
>
> Actually it would work. I like this approach because it doesn't break
> ABI, and doesn't change the semantics unless the new ioctls are used.
>
>>> - In a separate patch, I'll deprecate flush & invalidate entirely
>>> (usage of this deprecated
>>> API will fail, resulting in a guiding error message).
>
> I don't think there's any need for deprecation.
>
>>> We get a sane and versatile (and platform-independent) implementation
>>> that always work,
>>> but breaks user space. If anyone would need to work with current user space,
>>> the deprecating patch can always be reverted locally to get back a
>>> flush & invalidate
>>> implementations that (somehow) work.
>
> I still would like the new API for the reason I mentioned before: so
> that user-space can clean/invalidate/flush.
>
> Cheers.
>
> --
> Felipe Contreras
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html