| Message ID | 20210926061116.282-1-caihuoqing@baidu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/hns: Use dma_alloc_coherent() instead of kmalloc/dma_map_single() | expand |
On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: > Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() > with dma_alloc_coherent/dma_free_coherent() helps to reduce > code size, and simplify the code, and coherent DMA will not > clear the cache every time. > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > --- > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) Given I don't see any dma_sync_single calls for this mapping, isn't this a correctness fix too? Jason
On Mon, Sep 27, 2021 at 08:59:13AM -0300, Jason Gunthorpe wrote: > On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: > > Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() > > with dma_alloc_coherent/dma_free_coherent() helps to reduce > > code size, and simplify the code, and coherent DMA will not > > clear the cache every time. > > > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- > > 1 file changed, 5 insertions(+), 15 deletions(-) > > Given I don't see any dma_sync_single calls for this mapping, isn't > this a correctness fix too? HNS folks? Jason
On 2021/10/5 3:52, Jason Gunthorpe wrote: > On Mon, Sep 27, 2021 at 08:59:13AM -0300, Jason Gunthorpe wrote: >> On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: >>> Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() >>> with dma_alloc_coherent/dma_free_coherent() helps to reduce >>> code size, and simplify the code, and coherent DMA will not >>> clear the cache every time. >>> >>> Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> >>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- >>> 1 file changed, 5 insertions(+), 15 deletions(-) >> >> Given I don't see any dma_sync_single calls for this mapping, isn't >> this a correctness fix too? > > HNS folks? > > Jason > . > Our SoC can keep cache coherent, so there is no exception even if dma_sync_single* is not called, but the driver should not make assumptions about SoC. So using dma_alloc_coherent() instead of kmalloc/dma_map_single() can simplify the code and achieve the same purpose. Wenpeng Liang
On 09 10月 21 17:50:50, Wenpeng Liang wrote: > > > On 2021/10/5 3:52, Jason Gunthorpe wrote: > > On Mon, Sep 27, 2021 at 08:59:13AM -0300, Jason Gunthorpe wrote: > >> On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: > >>> Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() > >>> with dma_alloc_coherent/dma_free_coherent() helps to reduce > >>> code size, and simplify the code, and coherent DMA will not > >>> clear the cache every time. > >>> > >>> Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > >>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- > >>> 1 file changed, 5 insertions(+), 15 deletions(-) > >> > >> Given I don't see any dma_sync_single calls for this mapping, isn't > >> this a correctness fix too? > > > > HNS folks? > > > > Jason > > . > > > > Our SoC can keep cache coherent, so there is no exception even if > dma_sync_single* is not called, but the driver should not make > assumptions about SoC. > > So using dma_alloc_coherent() instead of kmalloc/dma_map_single() > can simplify the code and achieve the same purpose. > > Wenpeng Liang Hi Liang Thanks for your feedback. If you think my patch is correct, you can give a Reviewed-by: to it. You can also give a Tested-by: to it, if the test on hardware was made. Thanks Cai
On 2021/10/9 18:42, Cai Huoqing wrote: > On 09 10月 21 17:50:50, Wenpeng Liang wrote: >> >> >> On 2021/10/5 3:52, Jason Gunthorpe wrote: >>> On Mon, Sep 27, 2021 at 08:59:13AM -0300, Jason Gunthorpe wrote: >>>> On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: >>>>> Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() >>>>> with dma_alloc_coherent/dma_free_coherent() helps to reduce >>>>> code size, and simplify the code, and coherent DMA will not >>>>> clear the cache every time. >>>>> >>>>> Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> >>>>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- >>>>> 1 file changed, 5 insertions(+), 15 deletions(-) >>>> >>>> Given I don't see any dma_sync_single calls for this mapping, isn't >>>> this a correctness fix too? >>> >>> HNS folks? >>> >>> Jason >>> . >>> >> >> Our SoC can keep cache coherent, so there is no exception even if >> dma_sync_single* is not called, but the driver should not make >> assumptions about SoC. >> >> So using dma_alloc_coherent() instead of kmalloc/dma_map_single() >> can simplify the code and achieve the same purpose. >> >> Wenpeng Liang > > > Hi Liang > > Thanks for your feedback. > > If you think my patch is correct, you can give a Reviewed-by: to it. > You can also give a Tested-by: to it, if the test on hardware was made. > > Thanks > Cai > . > Hi, Cai After testing, it seems ok to me. Reviewed-by: Wenpeng Liang <liangwenpeng@huawei.com> Tested-by: Wenpeng Liang <liangwenpeng@huawei.com> Thanks, Wenpeng
On Sun, Sep 26, 2021 at 02:11:15PM +0800, Cai Huoqing wrote: > Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() > with dma_alloc_coherent/dma_free_coherent() helps to reduce > code size, and simplify the code, and coherent DMA will not > clear the cache every time. > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > Reviewed-by: Wenpeng Liang <liangwenpeng@huawei.com> > Tested-by: Wenpeng Liang <liangwenpeng@huawei.com> > --- > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c index 5b9953105752..380445f98a49 100644 --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c @@ -1165,32 +1165,22 @@ static int hns_roce_alloc_cmq_desc(struct hns_roce_dev *hr_dev, { int size = ring->desc_num * sizeof(struct hns_roce_cmq_desc); - ring->desc = kzalloc(size, GFP_KERNEL); + ring->desc = dma_alloc_coherent(hr_dev->dev, size, + &ring->desc_dma_addr, GFP_KERNEL); if (!ring->desc) return -ENOMEM; - ring->desc_dma_addr = dma_map_single(hr_dev->dev, ring->desc, size, - DMA_BIDIRECTIONAL); - if (dma_mapping_error(hr_dev->dev, ring->desc_dma_addr)) { - ring->desc_dma_addr = 0; - kfree(ring->desc); - ring->desc = NULL; - - return -ENOMEM; - } - return 0; } static void hns_roce_free_cmq_desc(struct hns_roce_dev *hr_dev, struct hns_roce_v2_cmq_ring *ring) { - dma_unmap_single(hr_dev->dev, ring->desc_dma_addr, - ring->desc_num * sizeof(struct hns_roce_cmq_desc), - DMA_BIDIRECTIONAL); + dma_free_coherent(hr_dev->dev, + ring->desc_num * sizeof(struct hns_roce_cmq_desc), + ring->desc, ring->desc_dma_addr); ring->desc_dma_addr = 0; - kfree(ring->desc); } static int init_csq(struct hns_roce_dev *hr_dev,
Replacing kmalloc/kfree/dma_map_single/dma_unmap_single() with dma_alloc_coherent/dma_free_coherent() helps to reduce code size, and simplify the code, and coherent DMA will not clear the cache every time. Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> --- drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)