lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 16 Aug 2007 22:06:22 -0700 From: Roland Dreier <rdreier@...co.com> To: akepner@....com Cc: linux-kernel@...r.kernel.org, tony.luck@...el.com, jes@....com Subject: Re: [PATCH] sn-ia64: allow drivers to flush in-flight DMA The overall approach looks fine to me, although I'm not the arbiter of taste for the DMA API. However, I think this wants to be split into at least three parts for merging: adding the dmaflush flags stuff into the DMA API; adding the dmaflush parameter to the ib_umem_get() API (and fixing every caller); and using that API to fix mthca on SGI boxes. (And also there should probably be a 4th patch to fix the same issue with mlx4 at least, and possibly further patches for other drivers such as the cxgb3 RDMA driver, etc) Also further comments below: > struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, > - size_t size, int access) > + size_t size, int access, int dmaflush) This change means every caller of ib_umem_get needs to be fixed up as part of the patch. > @@ -1027,7 +1029,14 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length, > if (!mr) > return ERR_PTR(-ENOMEM); > > - mr->umem = ib_umem_get(pd->uobject->context, start, length, acc); > + if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd)) { > + err = -EFAULT; > + goto err; > + } trivial, but you might as well do the copy_from_udata before allocating mr, so that you can just return ERR_PTR(-EFAULT) without having to free mr on the error path. > + dmaflush = (int) ucmd.mr_attrs & MTHCA_MR_DMAFLUSH; > + > + mr->umem = ib_umem_get(pd->uobject->context, start, length, acc, > + dmaflush); I think the dmaflush temporary variable isn't adding anything, and certainly the cast to int is unnecessary. Anyway, it looks better to me to write this simply as mr->umem = ib_umem_get(pd->uobject->context, start, length, acc, ucmd.mr_attrs & MTHCA_MR_DMAFLUSH); > +#define MTHCA_MR_DMAFLUSH 0x1 /* flush in-flight DMA on a write to > + * memory region (IA64_SGI_SN2 only) */ I would leave out the commentary about this being SN2-only -- the whole point is to use a generic API to hide this detail. - R. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists