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: Wed, 19 Mar 2008 17:26:37 -0700 From: akepner@....com To: Andrew Morton <akpm@...ux-foundation.org> Cc: Tony Luck <tony.luck@...el.com>, Jesse Barnes <jbarnes@...tuousgeek.org>, Jes Sorensen <jes@....com>, Randy Dunlap <randy.dunlap@...cle.com>, Roland Dreier <rdreier@...co.com>, James Bottomley <James.Bottomley@...senPartnership.com>, David Miller <davem@...emloft.net>, Benjamin Herrenschmidt <benh@...nel.crashing.org>, Grant Grundler <grundler@...isc-linux.org>, Michael Ellerman <michael@...erman.id.au>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 0/4 v4] dma: dma_{un}map_{single|sg}_attrs() interface On Thu, Mar 13, 2008 at 01:41:06AM -0700, Andrew Morton wrote: Thanks for the review. I have a new patchset which will address most of your comments. > On Wed, 12 Mar 2008 21:00:22 -0700 akepner@....com wrote: > > > ... > > > > diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h > > index e69de29..05b6e91 100644 > > --- a/include/linux/dma-attrs.h > > +++ b/include/linux/dma-attrs.h > > @@ -0,0 +1,55 @@ > > +#ifndef _DMA_ATTR_H > > +#define _DMA_ATTR_H > > + > > +enum dma_attr { > > + DMA_ATTR_BARRIER, > > + DMA_ATTR_MAX, > > +}; > > + > > +struct dma_attrs { > > + unsigned flags; > > +}; > > Please comment the above. Describe the semantic meaning of the enums and > how they are mapped into `struct dma_attrs' (which is what I assume they > do). I see there's a documentation update here so if you think it's best > to direct the code reader to that file then fine, add a suitable reference. OK, I've added a reference to the Documentation/DMA-attributes.txt file. > > Why did we need to implement a struct for a plain old flags word? To make the type that carries the attributes opaque, so that it could be expanded. It's a bitmap in the latest version. > > > +#define __DMA_ATTRS_INIT() { .flags = 0, } > > Does this need to exist? No, it's gone from the next patchset. > > > +#define DECLARE_DMA_ATTRS(x) struct dma_attrs x = __DMA_ATTRS_INIT() > > This is a definition, not a declaration. Please rename it to > DEFINE_DMA_ATTRS(). > You're right. Done. > > +#define INIT_DMA_ATTRS(x) (x)->flags = 0; > > So if I do > > if (expr) > INIT_DMA_ATTRS(bar); > else > something_else(); > > it won't compile. > > Golden rule: implement not in cpp that which can be implemented in C. > > static inline void init_dma_attrs(struct dma_attrs *attrs) > { > attrs->flags = 0; > } > > is much nicer, no? > Yes ;-) > > +#ifdef ARCH_USES_DMA_ATTRS > > There is no precedent for ARCH_USES_*. > > There is a little bit of precedent for ARCH_HAVE_* > > There is lots of precendence for ARCH_HAS_*. > > We don't like ARCH_HAS_* anyway ;) What can we do to get rid of this? > Ideally, make it available on all architectures at zero cost to those which > don't need it. If that is impractical (why?) then it is preferable to do > this in Kconfig. > OK, I've changed this to CONFIG_HAVE_DMA_ATTRS, selected in Kconfig. > > +/* > > + * dma_set_attr - set a specific attribute > > + * may be called with a null attrs > > + */ > > +static inline int dma_set_attr(struct dma_attrs *attrs, enum dma_attr attr) > > +{ > > + if (!attrs) > > + return 0; > > + if (attr < DMA_ATTR_MAX) { > > + attrs->flags |= (1 << attr); > > + return 0; > > + } > > + return -EINVAL; > > +} > > Is there any non-buggy reason why code would pass an out-of-range attribute > into this function? If not, BUG_ON() would be appropriate treatment. Latest version uses BUG_ON(). > > .... > We have a dma_set_attr() stub but no dma_get_attr() stub? > I put a stub in, but dma_get_attr() is only used by arch-specific code, and only code that has CONFIG_HAVE_DMA_ATTRS set. dma_set_attr() is used by arch-independent code, so it has to be there whether CONFIG_HAVE_DMA_ATTRS is set or no. -- Arthur -- 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