Re: Modifying sg_dma_len(sg)?

Re: Modifying sg_dma_len(sg)?

[Vinod Koul]

On Fri, May 15, 2015 at 03:46:27PM +0200, Geert Uytterhoeven wrote:

am ccing LKML, perhaps this needs wider discussion..

> Several drivers reuse mapped scatterlists, and modify sg_dma_len(sg) to
> match the actual number of bytes they want to transfer.
> 
> Hence during driver shutdown, dma_unmap_sg() is called with a scatterlist
> that has modified lengths, compared to the original passed to dma_map_sg().
> If CONFIG_DMA_API_DEBUG=y, this causes warnings like:
> 
>     WARNING: CPU: 0 PID: 20 at lib/dma-debug.c:1103 check_unmap+0x24c/0x85c()
>     rcar-dmac e6700000.dma-controller: DMA-API: device driver frees
> DMA memory with different size [device address=0x000000006e15f000]
> [map size=4096 bytes] [unmap size=3 bytes]
> 
> The warning can be silenced by restoring the original sg_dma_len() just before
> calling dma_unmap_sg(), but it looks like no driver actually does that.
Right, but should the drivers map with one length and then modify the
length later on, why not map only the size that is required...

So driver should always unmap and remap with the right length whenever it is
required to change the length. Perhaps a dma_remap_sg() API to do so
properly.

I do not think modifying length directly should be encouraged...

-- 
~Vinod
> 
> Is it allowed to modify sg_dma_len(sg)?
> Is it OK to restore the original sg_dma_len()?
> 
> Thanks for your answers!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Re: Modifying sg_dma_len(sg)?

[Geert Uytterhoeven]

On Tue, May 19, 2015 at 6:01 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, May 15, 2015 at 03:46:27PM +0200, Geert Uytterhoeven wrote:
>
> am ccing LKML, perhaps this needs wider discussion..
>
>> Several drivers reuse mapped scatterlists, and modify sg_dma_len(sg) to
>> match the actual number of bytes they want to transfer.
>>
>> Hence during driver shutdown, dma_unmap_sg() is called with a scatterlist
>> that has modified lengths, compared to the original passed to dma_map_sg().
>> If CONFIG_DMA_API_DEBUG=y, this causes warnings like:
>>
>>     WARNING: CPU: 0 PID: 20 at lib/dma-debug.c:1103 check_unmap+0x24c/0x85c()
>>     rcar-dmac e6700000.dma-controller: DMA-API: device driver frees
>> DMA memory with different size [device address=0x000000006e15f000]
>> [map size=4096 bytes] [unmap size=3 bytes]
>>
>> The warning can be silenced by restoring the original sg_dma_len() just before
>> calling dma_unmap_sg(), but it looks like no driver actually does that.
> Right, but should the drivers map with one length and then modify the
> length later on, why not map only the size that is required...

This is mostly done in serial drivers, which set up an initial mapping, and
reuse it with different lengths, depending on the amount of data to transfer
at that time.

> So driver should always unmap and remap with the right length whenever it is
> required to change the length. Perhaps a dma_remap_sg() API to do so
> properly.
>
> I do not think modifying length directly should be encouraged...

In cases where there's only a single segment, I think it's better to use
dma_map_single() instead of dma_map_sg().
Then the actual length to transfer can easily be passed to
dmaengine_prep_slave_single(), right?

> --
> ~Vinod
>>
>> Is it allowed to modify sg_dma_len(sg)?
>> Is it OK to restore the original sg_dma_len()?
>>
>> Thanks for your answers!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Modifying sg_dma_len(sg)?

[Vinod Koul]

On Tue, May 19, 2015 at 08:51:54AM +0200, Geert Uytterhoeven wrote:
> On Tue, May 19, 2015 at 6:01 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Fri, May 15, 2015 at 03:46:27PM +0200, Geert Uytterhoeven wrote:
> >
> > am ccing LKML, perhaps this needs wider discussion..
> >
> >> Several drivers reuse mapped scatterlists, and modify sg_dma_len(sg) to
> >> match the actual number of bytes they want to transfer.
> >>
> >> Hence during driver shutdown, dma_unmap_sg() is called with a scatterlist
> >> that has modified lengths, compared to the original passed to dma_map_sg().
> >> If CONFIG_DMA_API_DEBUG=y, this causes warnings like:
> >>
> >>     WARNING: CPU: 0 PID: 20 at lib/dma-debug.c:1103 check_unmap+0x24c/0x85c()
> >>     rcar-dmac e6700000.dma-controller: DMA-API: device driver frees
> >> DMA memory with different size [device address=0x000000006e15f000]
> >> [map size=4096 bytes] [unmap size=3 bytes]
> >>
> >> The warning can be silenced by restoring the original sg_dma_len() just before
> >> calling dma_unmap_sg(), but it looks like no driver actually does that.
> > Right, but should the drivers map with one length and then modify the
> > length later on, why not map only the size that is required...
> 
> This is mostly done in serial drivers, which set up an initial mapping, and
> reuse it with different lengths, depending on the amount of data to transfer
> at that time.
> 
> > So driver should always unmap and remap with the right length whenever it is
> > required to change the length. Perhaps a dma_remap_sg() API to do so
> > properly.
> >
> > I do not think modifying length directly should be encouraged...
> 
> In cases where there's only a single segment, I think it's better to use
> dma_map_single() instead of dma_map_sg().
> Then the actual length to transfer can easily be passed to
> dmaengine_prep_slave_single(), right?
Sounds right to me

-- 
~Vinod