On 10/12/2014 18:22, Yann Droneaud wrote: > Hi, > > Le mardi 11 novembre 2014 à 18:36 +0200, Haggai Eran a écrit : >> In some drivers there's a need to read data from a user space area that >> was pinned using ib_umem, when running from a different process context. >> >> The ib_umem_copy_from function allows reading data from the physical pages >> pinned in the ib_umem struct. >> >> Signed-off-by: Haggai Eran <haggaie@xxxxxxxxxxxx> >> --- >> drivers/infiniband/core/umem.c | 26 ++++++++++++++++++++++++++ >> include/rdma/ib_umem.h | 2 ++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c >> index e0f883292374..77bec75963e7 100644 >> --- a/drivers/infiniband/core/umem.c >> +++ b/drivers/infiniband/core/umem.c >> @@ -292,3 +292,29 @@ int ib_umem_page_count(struct ib_umem *umem) >> return n; >> } >> EXPORT_SYMBOL(ib_umem_page_count); >> + >> +/* >> + * Copy from the given ib_umem's pages to the given buffer. >> + * >> + * umem - the umem to copy from >> + * offset - offset to start copying from >> + * dst - destination buffer >> + * length - buffer length >> + * >> + * Returns the number of copied bytes, or an error code. >> + */ >> +int ib_umem_copy_from(struct ib_umem *umem, size_t offset, void *dst, >> + size_t length) > > I would prefer the arguments in the same order as ib_copy_from_udata() > > int ib_umem_copy_from(void *dst, > struct ib_umem *umem, size_t umem_offset, > size_t length); No problem. >> +{ >> + size_t end = offset + length; >> + >> + if (offset > umem->length || end > umem->length || end < offset) { >> + pr_err("ib_umem_copy_from not in range. offset: %zd umem length: %zd end: %zd\n", >> + offset, umem->length, end); >> + return -EINVAL; >> + } >> + >> + return sg_pcopy_to_buffer(umem->sg_head.sgl, umem->nmap, dst, length, >> + offset + ib_umem_offset(umem)); >> +} >> +EXPORT_SYMBOL(ib_umem_copy_from); > > As the function return a "int", no more than INT_MAX bytes (likely 2^31 > - 1) can be copied. Perhaps changing the return type to to ssize_t would > be better (and a check to enfore ssize_t maximum value). Or change the > function could return 0 in case of success or a error code, just like > ib_copy_from_udata(). > Okay. I'll change it to match ib_copy_from_udata. We're checking the umem size in the call site of this function anyway, and the only reason I see sg_pcopy_to_buffer would return less than *length* bytes is when reaching the end of the scatterlist. Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html