From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sinan Kaya Subject: Re: [PATCH V3 3/4] dmaselftest: add memcpy selftest support functions Date: Sun, 8 Nov 2015 22:07:00 -0500 Message-ID: <56400DD4.5080506@codeaurora.org> References: <1446958380-23298-1-git-send-email-okaya@codeaurora.org> <1446958380-23298-4-git-send-email-okaya@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:37081 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752350AbbKIDHG (ORCPT ); Sun, 8 Nov 2015 22:07:06 -0500 In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Andy Shevchenko Cc: dmaengine , timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com, Andy Gross , linux-arm-msm@vger.kernel.org, linux-arm Mailing List , Vinod Koul , Dan Williams , "linux-kernel@vger.kernel.org" On 11/8/2015 3:09 PM, Andy Shevchenko wrote: > On Sun, Nov 8, 2015 at 6:52 AM, Sinan Kaya wro= te: >> This patch adds supporting utility functions >> for selftest. The intention is to share the self >> test code between different drivers. >> >> Supported test cases include: >> 1. dma_map_single >> 2. streaming DMA >> 3. coherent DMA >> 4. scatter-gather DMA > > All below comments about entire file, please check and update. > >> +struct test_result { >> + atomic_t counter; >> + wait_queue_head_t wq; >> + struct dma_device *dmadev; > > dmadev -> dma. > Done. >> +}; >> + >> +static void dma_selftest_complete(void *arg) >> +{ >> + struct test_result *result =3D arg; >> + struct dma_device *dmadev =3D result->dmadev; >> + >> + atomic_inc(&result->counter); >> + wake_up(&result->wq); >> + dev_dbg(dmadev->dev, "self test transfer complete :%d\n", >> + atomic_read(&result->counter)); >> +} >> + >> +/* >> + * Perform a transaction to verify the HW works. >> + */ >> +static int dma_selftest_sg(struct dma_device *dmadev, > > dmdev -> dma > ok >> + struct dma_chan *dma_chanptr, u64 size, > > dma_chanptr -> chan ok > >> + unsigned long flags) >> +{ >> + dma_addr_t src_dma, dest_dma, dest_dma_it; > > src_dma -> src, dest_dma_it -> dst ? ok > >> + u8 *dest_buf; > > Perhaps put nearby src_buf definition? ok > >> + u32 i, j =3D 0; > > unsigned int why? > >> + dma_cookie_t cookie; >> + struct dma_async_tx_descriptor *tx; > >> + int err =3D 0; >> + int ret; > > Any reason to have two instead of one of similar meaning? > removed ret >> + struct sg_table sg_table; >> + struct scatterlist *sg; >> + int nents =3D 10, count; >> + bool free_channel =3D 1; >> + u8 *src_buf; >> + int map_count; >> + struct test_result result; > > Hmm=E2=80=A6 Maybe make names shorter? > >> + >> + init_waitqueue_head(&result.wq); >> + atomic_set(&result.counter, 0); >> + result.dmadev =3D dmadev; >> + >> + if (!dma_chanptr) >> + return -ENOMEM; >> + >> + if (dmadev->device_alloc_chan_resources(dma_chanptr) < 1) > > >> + return -ENODEV; >> + >> + if (!dma_chanptr->device || !dmadev->dev) { >> + dmadev->device_free_chan_resources(dma_chanptr); >> + return -ENODEV; >> + } >> + >> + ret =3D sg_alloc_table(&sg_table, nents, GFP_KERNEL); >> + if (ret) { >> + err =3D ret; >> + goto sg_table_alloc_failed; >> + } >> + >> + for_each_sg(sg_table.sgl, sg, nents, i) { >> + u64 alloc_sz; >> + void *cpu_addr; >> + >> + alloc_sz =3D round_up(size, nents); >> + do_div(alloc_sz, nents); >> + cpu_addr =3D kmalloc(alloc_sz, GFP_KERNEL); >> + >> + if (!cpu_addr) { >> + err =3D -ENOMEM; >> + goto sg_buf_alloc_failed; >> + } >> + >> + dev_dbg(dmadev->dev, "set sg buf[%d] :%p\n", i, cpu_= addr); >> + sg_set_buf(sg, cpu_addr, alloc_sz); >> + } >> + >> + dest_buf =3D kmalloc(round_up(size, nents), GFP_KERNEL); >> + if (!dest_buf) { >> + err =3D -ENOMEM; >> + goto dst_alloc_failed; >> + } >> + dev_dbg(dmadev->dev, "dest:%p\n", dest_buf); >> + >> + /* Fill in src buffer */ >> + count =3D 0; >> + for_each_sg(sg_table.sgl, sg, nents, i) { >> + src_buf =3D sg_virt(sg); >> + dev_dbg(dmadev->dev, >> + "set src[%d, %d, %p] =3D %d\n", i, j, src_bu= f, count); >> + >> + for (j =3D 0; j < sg_dma_len(sg); j++) >> + src_buf[j] =3D count++; >> + } >> + >> + /* dma_map_sg cleans and invalidates the cache in arm64 when >> + * DMA_TO_DEVICE is selected for src. That's why, we need to= do >> + * the mapping after the data is copied. >> + */ >> + map_count =3D dma_map_sg(dmadev->dev, sg_table.sgl, nents, >> + DMA_TO_DEVICE); >> + if (!map_count) { >> + err =3D -EINVAL; >> + goto src_map_failed; >> + } >> + >> + dest_dma =3D dma_map_single(dmadev->dev, dest_buf, >> + size, DMA_FROM_DEVICE); >> + >> + err =3D dma_mapping_error(dmadev->dev, dest_dma); >> + if (err) >> + goto dest_map_failed; >> + >> + /* check scatter gather list contents */ >> + for_each_sg(sg_table.sgl, sg, map_count, i) >> + dev_dbg(dmadev->dev, >> + "[%d/%d] src va=3D%p, iova =3D %pa len:%d\n"= , >> + i, map_count, sg_virt(sg), &sg_dma_address(s= g), >> + sg_dma_len(sg)); >> + >> + dest_dma_it =3D dest_dma; >> + for_each_sg(sg_table.sgl, sg, map_count, i) { >> + src_buf =3D sg_virt(sg); >> + src_dma =3D sg_dma_address(sg); >> + dev_dbg(dmadev->dev, "src_dma: %pad dest_dma:%pad\n"= , >> + &src_dma, &dest_dma_it); >> + >> + tx =3D dmadev->device_prep_dma_memcpy(dma_chanptr, d= est_dma_it, >> + src_dma, sg_dma_len(sg), flags); >> + if (!tx) { >> + dev_err(dmadev->dev, >> + "Self-test sg failed, disabling\n"); >> + err =3D -ENODEV; >> + goto prep_memcpy_failed; >> + } >> + >> + tx->callback_param =3D &result; >> + tx->callback =3D dma_selftest_complete; >> + cookie =3D tx->tx_submit(tx); >> + dest_dma_it +=3D sg_dma_len(sg); >> + } >> + >> + dmadev->device_issue_pending(dma_chanptr); >> + >> + /* >> + * It is assumed that the hardware can move the data within = 1s >> + * and signal the OS of the completion >> + */ >> + ret =3D wait_event_timeout(result.wq, >> + atomic_read(&result.counter) =3D=3D (map_count), >> + msecs_to_jiffies(10000)); >> + >> + if (ret <=3D 0) { >> + dev_err(dmadev->dev, >> + "Self-test sg copy timed out, disabling\n"); >> + err =3D -ENODEV; >> + goto tx_status; >> + } >> + dev_dbg(dmadev->dev, >> + "Self-test complete signal received\n"); >> + >> + if (dmadev->device_tx_status(dma_chanptr, cookie, NULL) !=3D >> + DMA_COMPLETE) { >> + dev_err(dmadev->dev, >> + "Self-test sg status not complete, disabling= \n"); >> + err =3D -ENODEV; >> + goto tx_status; >> + } >> + >> + dma_sync_single_for_cpu(dmadev->dev, dest_dma, size, >> + DMA_FROM_DEVICE); >> + >> + count =3D 0; >> + for_each_sg(sg_table.sgl, sg, map_count, i) { >> + src_buf =3D sg_virt(sg); >> + if (memcmp(src_buf, &dest_buf[count], sg_dma_len(sg)= ) =3D=3D 0) { >> + count +=3D sg_dma_len(sg); >> + continue; >> + } >> + >> + for (j =3D 0; j < sg_dma_len(sg); j++) { >> + if (src_buf[j] !=3D dest_buf[count]) { >> + dev_dbg(dmadev->dev, >> + "[%d, %d] (%p) src :%x dest (%p):%x = cnt:%d\n", >> + i, j, &src_buf[j], src_buf[j= ], >> + &dest_buf[count], dest_buf[c= ount], >> + count); >> + dev_err(dmadev->dev, >> + "Self-test copy failed compare, dis= abling\n"); >> + err =3D -EFAULT; >> + return err; >> + goto compare_failed; > > Here something wrong. removed the return. > >> + } >> + count++; >> + } >> + } >> + thanks --=20 Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, In= c. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a=20 Linux Foundation Collaborative Project