CPU Cache and busdma usage in USB

[Hans Petter Selasky]

On Tuesday 23 June 2009 10:35:42 Piotr Zięcik wrote:
> --- a/sys/dev/usb/usb_busdma.c
> +++ b/sys/dev/usb/usb_busdma.c
> @@ -658,8 +658,7 @@ usb_pc_cpu_invalidate(struct usb_page_cache *pc)
>                 /* nothing has been loaded into this page cache! */
>                 return;
>         }
> -       bus_dmamap_sync(pc->tag, pc->map,
> -           BUS_DMASYNC_POSTWRITE | BUS_DMASYNC_POSTREAD);
> +       bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
>  }
>
>  /*------------------------------------------------------------------------
>* @@ -672,8 +671,7 @@ usb_pc_cpu_flush(struct usb_page_cache *pc)
>                 /* nothing has been loaded into this page cache! */
>                 return;
>         }
> -       bus_dmamap_sync(pc->tag, pc->map,
> -           BUS_DMASYNC_PREWRITE | BUS_DMASYNC_PREREAD);
> +       bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
>  }

Hi,

Let's restart the discussion with the initial patch.

You want to change the flags passed to bus_dmamap_sync() so that the 
flush/invalidate mapping gets right. I understand why your patch makes it 
work. That's not the problem.

In "src/sys/arm/arm/busdma_machdep.c" there is a function called 
"_bus_dmamap_sync_bp()". If you look at that function you see that it only 
triggers on the "BUS_DMASYNC_PREWRITE" and "BUS_DMASYNC_POSTREAD" flags. After 
your patching only the PREXXXX flags are used, so if bouce pages are used on 
ARM and x86 and amd64 +++, then only BUS_DMASYNC_PREWRITE will do anything. 
This indicates that your patch is not fully correct.

Grepping through the source code for ARM, I found a line like this:

/*XXX*/ arm9_dcache_wbinv_range,        /* dcache_inv_range     */

which is not correct. If we are only invalidating, then it is not correct to 
do a flush first.

Summed up:

static void
bus_dmamap_sync_buf(void *buf, int len, bus_dmasync_op_t op)
{
        char _tmp_cl[arm_dcache_align], _tmp_clend[arm_dcache_align];

        if ((op & BUS_DMASYNC_PREWRITE) && !(op & BUS_DMASYNC_PREREAD)) {
                cpu_dcache_wb_range((vm_offset_t)buf, len);
                cpu_l2cache_wb_range((vm_offset_t)buf, len);
        }
        if (op & BUS_DMASYNC_PREREAD) {
                if (!(op & BUS_DMASYNC_PREWRITE) &&
                    ((((vm_offset_t)(buf) | len) & arm_dcache_align_mask) == 
0)) {
                        cpu_dcache_inv_range((vm_offset_t)buf, len);
                        cpu_l2cache_inv_range((vm_offset_t)buf, len);
                } else {

Because the USB code specifies both PREREAD and PREWRITE we end up in the 
following case, which is not implemented correctly. The function name 
indicates write back first, then invalidate, but when looking at the 
implementation:

<example>

        arm8_cache_purgeID,             /* idcache_wbinv_all    */
        (void *)arm8_cache_purgeID,     /* idcache_wbinv_range  */

</example>

You see that it only performs purge and no prior flush. This is what needs 
fixing! If semihalf could go through the "arm/arm/cpufunc.c" file and fix 
those flush and invalidate functions then many people would become happy!

Again, it is not a problem in USB firstly, it is a problem in "arm/xxx".

                        cpu_dcache_wbinv_range((vm_offset_t)buf, len);
                        cpu_l2cache_wbinv_range((vm_offset_t)buf, len);
                }
        }
        if (op & BUS_DMASYNC_POSTREAD) {
                if ((vm_offset_t)buf & arm_dcache_align_mask) {
                        memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~
                            arm_dcache_align_mask),
                            (vm_offset_t)buf & arm_dcache_align_mask);
                }
                if (((vm_offset_t)buf + len) & arm_dcache_align_mask) {
                        memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len),
                            arm_dcache_align - (((vm_offset_t)(buf) + len) &
                           arm_dcache_align_mask));
                }
                cpu_dcache_inv_range((vm_offset_t)buf, len);
                cpu_l2cache_inv_range((vm_offset_t)buf, len);

                if ((vm_offset_t)buf & arm_dcache_align_mask)
                        memcpy((void *)((vm_offset_t)buf &
                            ~arm_dcache_align_mask), _tmp_cl, 
                            (vm_offset_t)buf & arm_dcache_align_mask);
                if (((vm_offset_t)buf + len) & arm_dcache_align_mask)
                        memcpy((void *)((vm_offset_t)buf + len), _tmp_clend,
                            arm_dcache_align - (((vm_offset_t)(buf) + len) &
                           arm_dcache_align_mask));
        }
}

--HPS