Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs

[Luis R. Rodriguez]

On Fri, Jun 10, 2016 at 10:16:00PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Jun 10, 2016 at 04:49:47PM +0200, Luis R. Rodriguez wrote:
> > On Fri, Jun 10, 2016 at 12:11:18PM +0200, Krzysztof Kozlowski wrote:
> > > The dma-mapping core and the implementations do not change the
> > > DMA attributes passed by pointer.  Thus the pointer can point to const
> > > data.  However the attributes do not have to be a bitfield. Instead
> > > unsigned long will do fine:
> > > 
> > > 1. This is just simpler.  Both in terms of reading the code and setting
> > >    attributes.  Instead of initializing local attributes on the stack
> > >    and passing pointer to it to dma_set_attr(), just set the bits.
> > > 
> > > 2. It brings safeness and checking for const correctness because the
> > >    attributes are passed by value.
> > 
> > Do we not expect the number of argument to grow ? This "cleanup" would
> > do away with such possibilities, and then require adding the API later,
> > and this requiring a full set of collateral evolutions again when this
> > is needed. What was the original motivation for using this instead of
> > the approach you are suggesting ?
> 
> What do you mean by "possibilities of argument to grow"? Something like
> adding new members to "struct dma_attrs" and changing its meaning?

Yup that.

> I think such growth is still constrained - you cannot put there anything
> without changing the meaning of the argument.

Obviously, however it would mean no needed collateral evolutions,
just an extension to the struct and drivers that use the new member
can make use of it.

> However you are right that "unsigned long" removes that possibility
> completely.

This is the concern.

> The dma-attrs in current form were added around 2008 in 74bc7ceebfa1
> ("dma: add dma_*map*_attrs() interfaces"), I think. Since that time, for
> example, the dma_map_*_attrs() did not change.

So we don't expect this to change either?

> > If the concern is the const data, why not require const struct dma_attr
> > for the APIs that we know can and should use const ?
> 
> The const is one concern. Complicated (more than expected) usage of dma
> attributes by the caller is second. 
> 
> Switching it to const would also reduce the possibilities of API
> extension.

My point was that const can be used for only APIs that we are sure of
that need it.

  Luis