Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Mon, Jun 13, 2016 at 09:00:41AM -0700, Christoph Hellwig wrote: > On Fri, Jun 10, 2016 at 04:49:47PM +0200, Luis R. Rodriguez wrote: > > 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 ? > > We still got plenty of space for attrs. If you're worried about running > out of 32 flags we could do a dma_attrs_t typedef that we could swich > to a u64. That would have another advantage in that we could add a > __bitwise sparse annotation to avoid people passing the wrong kind of > flags. History shows only sparse use of extensions of these these attributes so I think the approach with a typedef is definitely more suitable for this API, and should last long enough. The added gain of the annotation is a nice bonus. Luis
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Mon, Jun 13, 2016 at 09:00:41AM -0700, Christoph Hellwig wrote: > On Fri, Jun 10, 2016 at 04:49:47PM +0200, Luis R. Rodriguez wrote: > > 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 ? > > We still got plenty of space for attrs. If you're worried about running > out of 32 flags we could do a dma_attrs_t typedef that we could swich > to a u64. That would have another advantage in that we could add a > __bitwise sparse annotation to avoid people passing the wrong kind of > flags. History shows only sparse use of extensions of these these attributes so I think the approach with a typedef is definitely more suitable for this API, and should last long enough. The added gain of the annotation is a nice bonus. Luis
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 04:49:47PM +0200, Luis R. Rodriguez wrote: > 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 ? We still got plenty of space for attrs. If you're worried about running out of 32 flags we could do a dma_attrs_t typedef that we could swich to a u64. That would have another advantage in that we could add a __bitwise sparse annotation to avoid people passing the wrong kind of flags.
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 04:49:47PM +0200, Luis R. Rodriguez wrote: > 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 ? We still got plenty of space for attrs. If you're worried about running out of 32 flags we could do a dma_attrs_t typedef that we could swich to a u64. That would have another advantage in that we could add a __bitwise sparse annotation to avoid people passing the wrong kind of flags.
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 10:44:19PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Jun 10, 2016 at 10:23:47PM +0200, Luis R. Rodriguez wrote:
> > On Fri, Jun 10, 2016 at 10:16:00PM +0200, Krzysztof Kozlowski wrote:
> > > 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?
>
> I do not know, I am not aware of planned changes to that.
If this will not change then I think this change is good.
> > > > 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.
>
> As of now, dma_attrs should be const everywhere. That would be almost
> the same patchset as current one. If you consider extending the
> dma_attrs to something new and not yet known, then how will
> differentiate between cases when 'const' is needed for sure?
Depends on the use case, but if it known it should always be const
then great.
> I understand your concern. Sticking to current API for that reason might
> be a good defensive API programming... or might be way of keeping this
> function prototype for long...
Since this hasn't changed for years at all I think your change
is reasonable.
Luis
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 10:44:19PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Jun 10, 2016 at 10:23:47PM +0200, Luis R. Rodriguez wrote:
> > On Fri, Jun 10, 2016 at 10:16:00PM +0200, Krzysztof Kozlowski wrote:
> > > 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?
>
> I do not know, I am not aware of planned changes to that.
If this will not change then I think this change is good.
> > > > 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.
>
> As of now, dma_attrs should be const everywhere. That would be almost
> the same patchset as current one. If you consider extending the
> dma_attrs to something new and not yet known, then how will
> differentiate between cases when 'const' is needed for sure?
Depends on the use case, but if it known it should always be const
then great.
> I understand your concern. Sticking to current API for that reason might
> be a good defensive API programming... or might be way of keeping this
> function prototype for long...
Since this hasn't changed for years at all I think your change
is reasonable.
Luis
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 10:23:47PM +0200, Luis R. Rodriguez wrote:
> 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.
For parts of the API, there is still possibility of adding new layer of
wrapping, just like it was done with dma_map_single_attrs():
#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL)
For the dma_map_ops not...
> > 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?
I do not know, I am not aware of planned changes to that.
>
> > > 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.
As of now, dma_attrs should be const everywhere. That would be almost
the same patchset as current one. If you consider extending the
dma_attrs to something new and not yet known, then how will
differentiate between cases when 'const' is needed for sure?
I understand your concern. Sticking to current API for that reason might
be a good defensive API programming... or might be way of keeping this
function prototype for long...
Best regards,
Krzysztof
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
On Fri, Jun 10, 2016 at 10:23:47PM +0200, Luis R. Rodriguez wrote:
> 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.
For parts of the API, there is still possibility of adding new layer of
wrapping, just like it was done with dma_map_single_attrs():
#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL)
For the dma_map_ops not...
> > 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?
I do not know, I am not aware of planned changes to that.
>
> > > 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.
As of now, dma_attrs should be const everywhere. That would be almost
the same patchset as current one. If you consider extending the
dma_attrs to something new and not yet known, then how will
differentiate between cases when 'const' is needed for sure?
I understand your concern. Sticking to current API for that reason might
be a good defensive API programming... or might be way of keeping this
function prototype for long...
Best regards,
Krzysztof
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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?
I think such growth is still constrained - you cannot put there anything
without changing the meaning of the argument.
However you are right that "unsigned long" removes that possibility
completely.
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.
> 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.
Best regards,
Krzysztof
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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?
I think such growth is still constrained - you cannot put there anything
without changing the meaning of the argument.
However you are right that "unsigned long" removes that possibility
completely.
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.
> 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.
Best regards,
Krzysztof
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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 ? 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 ? Luis
Re: [PATCH v4 01/44] dma-mapping: Use unsigned long for dma_attrs
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 ? 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 ? Luis
