| Message ID | 565DE4BA.1040703@sandisk.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> After dma_map_sg() has been called the return value of that function > must be used as the number of elements in the scatterlist instead of > scsi_sg_count(). Umm, but ib_map_mr_sg iterates on the sg list. Say you have sg_nents=3 and after mapping you got dma_nents=2 (2 entries are contig) and you pass that, ib_sg_to_pages will only iterate on 2 elements won't it? am I missing something? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2015 10:35 AM, Sagi Grimberg wrote: >> After dma_map_sg() has been called the return value of that function >> must be used as the number of elements in the scatterlist instead of >> scsi_sg_count(). > > Umm, but ib_map_mr_sg iterates on the sg list. Say you have sg_nents=3 > and after mapping you got dma_nents=2 (2 entries are contig) and you > pass that, ib_sg_to_pages will only iterate on 2 elements won't it? am > I missing something? Hello Sagi, From https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt: <quote> With scatterlists, you map a region gathered from several regions by: int i, count = dma_map_sg(dev, sglist, nents, direction); struct scatterlist *sg; for_each_sg(sglist, sg, count, i) { hw_address[i] = sg_dma_address(sg); hw_len[i] = sg_dma_len(sg); } where nents is the number of entries in the sglist. </quote> Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2015 20:39, Bart Van Assche wrote: > On 12/01/2015 10:35 AM, Sagi Grimberg wrote: >>> After dma_map_sg() has been called the return value of that function >>> must be used as the number of elements in the scatterlist instead of >>> scsi_sg_count(). >> >> Umm, but ib_map_mr_sg iterates on the sg list. Say you have sg_nents=3 >> and after mapping you got dma_nents=2 (2 entries are contig) and you >> pass that, ib_sg_to_pages will only iterate on 2 elements won't it? am >> I missing something? > > Hello Sagi, > > From https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt: > > <quote> > With scatterlists, you map a region gathered from several regions by: > > int i, count = dma_map_sg(dev, sglist, nents, direction); > struct scatterlist *sg; > > for_each_sg(sglist, sg, count, i) { > hw_address[i] = sg_dma_address(sg); > hw_len[i] = sg_dma_len(sg); > } > > where nents is the number of entries in the sglist. > </quote> From Documentation/DMA_API.txt <quote> int dma_map_sg(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction direction) Returns: the number of DMA address segments mapped (this may be shorter than <nents> passed in if some elements of the scatter/gather list are physically or virtually adjacent and an IOMMU maps them with a single entry). </quote> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 02, 2015 at 01:59:38PM +0200, Sagi Grimberg wrote: > >where nents is the number of entries in the sglist. > ></quote> > > From Documentation/DMA_API.txt > > <quote> > int > dma_map_sg(struct device *dev, struct scatterlist *sg, > int nents, enum dma_data_direction direction) > > Returns: the number of DMA address segments mapped (this may be shorter > than <nents> passed in if some elements of the scatter/gather list are > physically or virtually adjacent and an IOMMU maps them with a single > entry). > </quote> dma_map_sg returns the actual number of entries to iterate. At least historically some IOMMU implementations would do strange tricks like: If entries 2 and 3 could be merged dma_len for 2 would span 2 and 3, and then entry 3 would actually have the dma addr and len for entry 4. I'm not sure anyone still does that, but the first spot to check would be the Parisc IOMMU drivers. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> dma_map_sg returns the actual number of entries to iterate. At least > historically some IOMMU implementations would do strange tricks like: > > If entries 2 and 3 could be merged dma_len for 2 would span 2 and 3, > and then entry 3 would actually have the dma addr and len for entry 4. > > I'm not sure anyone still does that, but the first spot to check would > be the Parisc IOMMU drivers. Oh I see, I didn't know that. Thanks for clarifying this for me. AFAICT this patch is fine. Reviewed-by: Sagi Grimberg <sagig@mellanox.com> iser, nfs needs fixing too. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Replying to my own email, >> dma_map_sg returns the actual number of entries to iterate. At least >> historically some IOMMU implementations would do strange tricks like: >> >> If entries 2 and 3 could be merged dma_len for 2 would span 2 and 3, >> and then entry 3 would actually have the dma addr and len for entry 4. So what would be in the last entry {dma_addr, dma_len}? zeros? >> I'm not sure anyone still does that, but the first spot to check would >> be the Parisc IOMMU drivers. So how does that sit with the fact that dma_unmap requires the same sg_nents as in dma_map and not the actual value of dma entries? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 03, 2015 at 10:46:10AM +0200, Sagi Grimberg wrote: > >> If entries 2 and 3 could be merged dma_len for 2 would span 2 and 3, > >> and then entry 3 would actually have the dma addr and len for entry 4. > > So what would be in the last entry {dma_addr, dma_len}? zeros? > > >>I'm not sure anyone still does that, but the first spot to check would > >>be the Parisc IOMMU drivers. > > So how does that sit with the fact that dma_unmap requires the > same sg_nents as in dma_map and not the actual value of dma entries? Take a look at drivers/parisc/iommu-helpers.h:iommu_coalesce_chunks() and drivers/parisc/sba_iommu.c:sba_unmap_sg() for example. The first fills out the sglist, and zeroes all unused entries past what it fills in. The unmap side than simply exits the loop if the entries are zeroed. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index fac1423..3db9a65 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1313,7 +1313,7 @@ reset_state: } static int srp_map_finish_fr(struct srp_map_state *state, - struct srp_rdma_ch *ch) + struct srp_rdma_ch *ch, int sg_nents) { struct srp_target_port *target = ch->target; struct srp_device *dev = target->srp_host->srp_dev; @@ -1328,10 +1328,10 @@ static int srp_map_finish_fr(struct srp_map_state *state, WARN_ON_ONCE(!dev->use_fast_reg); - if (state->sg_nents == 0) + if (sg_nents == 0) return 0; - if (state->sg_nents == 1 && target->global_mr) { + if (sg_nents == 1 && target->global_mr) { srp_map_desc(state, sg_dma_address(state->sg), sg_dma_len(state->sg), target->global_mr->rkey); @@ -1345,8 +1345,7 @@ static int srp_map_finish_fr(struct srp_map_state *state, rkey = ib_inc_rkey(desc->mr->rkey); ib_update_fast_reg_key(desc->mr, rkey); - n = ib_map_mr_sg(desc->mr, state->sg, state->sg_nents, - dev->mr_page_size); + n = ib_map_mr_sg(desc->mr, state->sg, sg_nents, dev->mr_page_size); if (unlikely(n < 0)) return n; @@ -1452,16 +1451,15 @@ static int srp_map_sg_fr(struct srp_map_state *state, struct srp_rdma_ch *ch, state->fr.next = req->fr_list; state->fr.end = req->fr_list + ch->target->cmd_sg_cnt; state->sg = scat; - state->sg_nents = scsi_sg_count(req->scmnd); - while (state->sg_nents) { + while (count) { int i, n; - n = srp_map_finish_fr(state, ch); + n = srp_map_finish_fr(state, ch, count); if (unlikely(n < 0)) return n; - state->sg_nents -= n; + count -= n; for (i = 0; i < n; i++) state->sg = sg_next(state->sg); } @@ -1521,13 +1519,12 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req, if (dev->use_fast_reg) { state.sg = idb_sg; - state.sg_nents = 1; sg_set_buf(idb_sg, req->indirect_desc, idb_len); idb_sg->dma_address = req->indirect_dma_addr; /* hack! */ #ifdef CONFIG_NEED_SG_DMA_LENGTH idb_sg->dma_length = idb_sg->length; /* hack^2 */ #endif - ret = srp_map_finish_fr(&state, ch); + ret = srp_map_finish_fr(&state, ch, 1); if (ret < 0) return ret; } else if (dev->use_fmr) { diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h index 87a2a91..f6af531 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.h +++ b/drivers/infiniband/ulp/srp/ib_srp.h @@ -300,10 +300,7 @@ struct srp_map_state { dma_addr_t base_dma_addr; u32 dma_len; u32 total_len; - union { - unsigned int npages; - int sg_nents; - }; + unsigned int npages; unsigned int nmdesc; unsigned int ndesc; };
After dma_map_sg() has been called the return value of that function must be used as the number of elements in the scatterlist instead of scsi_sg_count(). Fixes: commit f7f7aab1a5c0 ("IB/srp: Convert to new registration API") Reported-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: stable <stable@vger.kernel.org> # v4.4+ Cc: Sagi Grimberg <sagig@mellanox.com> Cc: Sebastian Parschauer <sebastian.riemer@profitbricks.com> --- drivers/infiniband/ulp/srp/ib_srp.c | 19 ++++++++----------- drivers/infiniband/ulp/srp/ib_srp.h | 5 +---- 2 files changed, 9 insertions(+), 15 deletions(-)