>Subject: Re: [rdma-next 01/12] RDMA/bnxt_re: Use for_each_sg_dma_page
>iterator on umem SGL
>
>On Wed, Feb 13, 2019 at 01:12:37PM +0200, Gal Pressman wrote:
>> On 11-Feb-19 17:24, Shiraz Saleem wrote:
>> > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c
>> > b/drivers/infiniband/hw/bnxt_re/qplib_res.c
>> > index c8502c2..c8478c1 100644
>> > +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
>> > @@ -85,7 +85,7 @@ static void __free_pbl(struct pci_dev *pdev,
>> > struct bnxt_qplib_pbl *pbl, static int __alloc_pbl(struct pci_dev *pdev, struct
>bnxt_qplib_pbl *pbl,
>> > struct scatterlist *sghead, u32 pages, u32 pg_size) {
>> > - struct scatterlist *sg;
>> > + struct sg_dma_page_iter sg_iter;
>> > bool is_umem = false;
>> > int i;
>> >
>> > @@ -116,12 +116,13 @@ static int __alloc_pbl(struct pci_dev *pdev, struct
>bnxt_qplib_pbl *pbl,
>> > } else {
>> > i = 0;
>> > is_umem = true;
>> > - for_each_sg(sghead, sg, pages, i) {
>> > - pbl->pg_map_arr[i] = sg_dma_address(sg);
>> > - pbl->pg_arr[i] = sg_virt(sg);
>> > + for_each_sg_dma_page(sghead, &sg_iter, pages, 0) {
>> > + pbl->pg_map_arr[i] =
>sg_page_iter_dma_address(&sg_iter);
>> > + pbl->pg_arr[i] = NULL;
>> if (!pbl->pg_arr[i])
>>
>> Surely something went wrong here?
>
>That if does look wrong.
>
>I recall Shiraz removed the pg_arr as it wasn't being used, so maybe the if should
>go to?
Thanks Gal for catching this. Yes the reason pg_arr is set to NULL here is because it
appears used only in !umem case. But then the following if check is a bug. Duh!
bnxt_re folks - is this acceptable?
--- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
@@ -85,7 +85,7 @@ static void __free_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
struct scatterlist *sghead, u32 pages, u32 pg_size)
{
- struct scatterlist *sg;
+ struct sg_dma_page_iter sg_iter;
bool is_umem = false;
int i;
@@ -116,12 +116,9 @@ static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
} else {
i = 0;
is_umem = true;
- for_each_sg(sghead, sg, pages, i) {
- pbl->pg_map_arr[i] = sg_dma_address(sg);
- pbl->pg_arr[i] = sg_virt(sg);
- if (!pbl->pg_arr[i])
- goto fail;
-
+ for_each_sg_dma_page(sghead, &sg_iter, pages, 0) {
+ pbl->pg_map_arr[i] = sg_page_iter_dma_address(&sg_iter);
+ i++;
pbl->pg_count++;
}
}
Shiraz
