Re: Question: partial transfers of DMABUFs

[Christian König]

Am 15.02.23 um 14:52 schrieb Paul Cercueil:
> Le mercredi 15 février 2023 à 14:46 +0100, Christian König a écrit :
> > Am 15.02.23 um 14:24 schrieb Paul Cercueil:
> > > Hi Christian,
> > >
> > > Le mercredi 15 février 2023 à 13:58 +0100, Christian König a
> > > écrit :
> > > > Hi Paul,
> > > >
> > > > Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> > > > > Hi,
> > > > >
> > > > > I am working on adding support for DMABUFs in the IIO
> > > > > subsystem.
> > > > >
> > > > > One thing we want there, is the ability to specify the number
> > > > > of
> > > > > bytes
> > > > > to transfer (while still defaulting to the DMABUF size).
> > > > >
> > > > > Since dma_buf_map_attachment() returns a sg_table,
> > > > Please don't assume that this is an sg_table. We just used it as
> > > > container for DMA addresses, but this has proven to be a mistake.
> > > TL/DR, why was it a mistake? Just curious.
> > The sg_table should have just contained DMA addresses, but we had
> > multiple people who tried to use the pages instead.
> >
> > This works to some extend, but goes boom as soon as somebody messes
> > with
> > the pages reference counts or tries to map it into an address space
> > or
> > something like that.
> >
> > We got so far that we now intentionally mangle the page addresses in
> > the
> > sg_table to prevent people from using it:
> > https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763
> Isn't that breaking the chains though? I'd expect page_link to be
> mangled only if !sg_is_chain(sg).

Those are filtered out by for_each_sgtable_sg if I'm not completely 
mistaken.

> > > > There is work underway to replace the sg_table with (for example)
> > > > just
> > > > an array of DMA addresses.
> > > Ok, so I believe at some point we will need an equivalent of
> > > dmaengine_prep_slave_sg() which takes an array of DMA addresses.
> > Well we will probably come up with a new container for this, but
> > yeah.
> Understood.
>
> You said there was work underway, could you point me to the
> corresponding mailing list threads and/or code?

That's not really released yet. We just discussed it a bit when Daniel 
added the sg_table mangling after this went boom for the third time so :)

Just use git blame to find the patch of the mangling and read up on the 
mailing list discussion around that.

Regards,
Christian.

> > Regards,
> > Christian.
> Cheers,
> -Paul
>
> > > > > I basically have two options, and I can't decide which one is
> > > > > the
> > > > > best (or the less ugly):
> > > > >
> > > > > - Either I add a new API function similar to
> > > > > dmaengine_prep_slave_sg(),
> > > > > which still takes a scatterlist as argument but also takes the
> > > > > number
> > > > > of bytes as argument;
> > > > >
> > > > > - Or I add a function to duplicate the scatterlist and then
> > > > > shrink
> > > > > it
> > > > > manually, which doesn't sound like a good idea either.
> > > > >
> > > > > What would be the recommended way?
> > > > I strongly recommend to come up with a new function which only
> > > > takes
> > > > DMA
> > > > addresses and separate segment length.
> > > Alright, thanks for your input.
> > >
> > > So I would add a new dma_device.dma_prep_slave_dma_array() callback
> > > with a corresponding API function, and then the drivers can be
> > > converted from using .dma_prep_slave_sg() to this new function in
> > > due
> > > time.
> > >
> > > Vinod, that works for you?
> > >
> > > Cheers,
> > > -Paul