load_le16 is a synonym for the existing le16_to_cpup and is added to be
symmetric with the load_le16_noalign API. On arches where unaligned
access is OK, the unaligned calls are replaced with aligned calls.
store_le16 is a new API and is added to be symmetric with the unaligned
functions. It is implemented as a macro to allow compile-time
byteswapping when the value is a constant. This will also allow use in
many places currently that are of the form:
*(__le16 *)ptr = cpu_to_le16(foo);
In addition, some drivers/filesystems/arches already provide this API
privately, which will allow them to be consolidated into this common code.
Signed-off-by: Harvey Harrison <[email protected]>
---
Linus,
I'd like you to consider this for 2.6.29 as it just adds the new API using
the old API as the implementation. I've had multiple requests to get this
in so that the typesafe unaligned bits can be used in 2.6.30 without waiting
on the API additions to go in.
For 2.6.30 I have a series of patches to begin replacing users of the existing
bits (aligned and unaligned) and make the tangle of headers more sane, but getting
the API exposed would be great to avoid ordering headaches during the merge window.
include/linux/byteorder/generic.h | 25 +++++++++++++++++++------
1 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
index 0846e6b..621a506 100644
--- a/include/linux/byteorder/generic.h
+++ b/include/linux/byteorder/generic.h
@@ -119,6 +119,19 @@
#define cpu_to_be16s __cpu_to_be16s
#define be16_to_cpus __be16_to_cpus
+#define load_le16 __le16_to_cpup
+#define load_le32 __le32_to_cpup
+#define load_le64 __le64_to_cpup
+#define load_be16 __be16_to_cpup
+#define load_be32 __be32_to_cpup
+#define load_be64 __be64_to_cpup
+#define store_le16(p, val) (*(__le16 *)(p) = cpu_to_le16(val))
+#define store_le32(p, val) (*(__le32 *)(p) = cpu_to_le32(val))
+#define store_le64(p, val) (*(__le64 *)(p) = cpu_to_le64(val))
+#define store_be16(p, val) (*(__be16 *)(p) = cpu_to_be16(val))
+#define store_be32(p, val) (*(__be32 *)(p) = cpu_to_be32(val))
+#define store_be64(p, val) (*(__be64 *)(p) = cpu_to_be64(val))
+
/*
* They have to be macros in order to do the constant folding
* correctly - if the argument passed into a inline function
@@ -142,32 +155,32 @@
static inline void le16_add_cpu(__le16 *var, u16 val)
{
- *var = cpu_to_le16(le16_to_cpu(*var) + val);
+ store_le16(var, load_le16(var) + val);
}
static inline void le32_add_cpu(__le32 *var, u32 val)
{
- *var = cpu_to_le32(le32_to_cpu(*var) + val);
+ store_le32(var, load_le32(var) + val);
}
static inline void le64_add_cpu(__le64 *var, u64 val)
{
- *var = cpu_to_le64(le64_to_cpu(*var) + val);
+ store_le64(var, load_le64(var) + val);
}
static inline void be16_add_cpu(__be16 *var, u16 val)
{
- *var = cpu_to_be16(be16_to_cpu(*var) + val);
+ store_be16(var, load_be16(var) + val);
}
static inline void be32_add_cpu(__be32 *var, u32 val)
{
- *var = cpu_to_be32(be32_to_cpu(*var) + val);
+ store_be32(var, load_be32(var) + val);
}
static inline void be64_add_cpu(__be64 *var, u64 val)
{
- *var = cpu_to_be64(be64_to_cpu(*var) + val);
+ store_be64(var, load_be64(var) + val);
}
#endif /* _LINUX_BYTEORDER_GENERIC_H */
--
1.6.2.rc2.289.g2fa25
On Mon, 2 Mar 2009, Harvey Harrison wrote:
>
> store_le16 is a new API and is added to be symmetric with the unaligned
> functions.
This seems to be expressly designed to be unsafe, in that it casts the
thing to the right type, making it impossible for sparse to warn about
bad byteorder use.
Linus
On Mon, 2009-03-02 at 16:15 -0800, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Harvey Harrison wrote:
> >
> > store_le16 is a new API and is added to be symmetric with the unaligned
> > functions.
>
> This seems to be expressly designed to be unsafe, in that it casts the
> thing to the right type, making it impossible for sparse to warn about
> bad byteorder use.
>
Unfortunately yes, hopefully you have a solution for the problem I ran
into with this part:
This was added to be symmetric with the unaligned store API, and replace
code doing
*(__le16 *)ptr = cpu_to_le16(val);
So existing code is casting already in most of the places this could be
used. And although this could be made a static inline and get the sparse
checking, we lose one of the big advantages of the open-coding - constants
are byteswapped at compile time. Although gcc (4.4) grew support for __builtin_constant_p
in static inlines, older gcc's don't, so we would lose that with essentially
all current compilers.
So the option was to make it a static inline and get the sparse checking, or
add the cast in the macro and lose sparse checking but preserve the compile-time
swapping...maybe I chose poorly.
Thoughts?
Harvey
On Mon, 2 Mar 2009, Harvey Harrison wrote:
>
> Although gcc (4.4) grew support for __builtin_constant_p in static
> inlines, older gcc's don't, so we would lose that with essentially all
> current compilers.
We've used __builtin_constant_p in inline functions for a long time.
Look at kmalloc() in <linux/slab_dev.h>.
I do agree that it's a "new" feature, but I don't think it's _that_ new. I
think it goes back to something like 3.2 rather than 4.4.
Although I also would not be surprised if this is one of those "gcc
version of the day" things, where some versions do it, others don't.
So it's quite possible that it depends a bit on just how many dead cats
you have offered, and whether you ran widdershins or turnwise around the
computer when you turned it on.
Linus
On Mon, 2009-03-02 at 16:37 -0800, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Harvey Harrison wrote:
> >
> > Although gcc (4.4) grew support for __builtin_constant_p in static
> > inlines, older gcc's don't, so we would lose that with essentially all
> > current compilers.
>
> We've used __builtin_constant_p in inline functions for a long time.
>
> Look at kmalloc() in <linux/slab_dev.h>.
>
> I do agree that it's a "new" feature, but I don't think it's _that_ new. I
> think it goes back to something like 3.2 rather than 4.4.
>
> Although I also would not be surprised if this is one of those "gcc
> version of the day" things, where some versions do it, others don't.
>
> So it's quite possible that it depends a bit on just how many dead cats
> you have offered, and whether you ran widdershins or turnwise around the
> computer when you turned it on.
>
OK, static inline it is then. Would you be opposed to an API like:
get_le16
put_le16
to match with
get_unaligned_le16
put_unaligned_le16
And make the existing unaligned helpers typesafe?
Harvey
On Mon, 2 Mar 2009, Harvey Harrison wrote:
>
> OK, static inline it is then. Would you be opposed to an API like:
>
> get_le16
> put_le16
>
> to match with
>
> get_unaligned_le16
> put_unaligned_le16
>
> And make the existing unaligned helpers typesafe?
That sounds much better to me. That said, I'm also wondering what the
upside is of this all?
Linus
On Mon, 2009-03-02 at 17:51 -0800, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Harvey Harrison wrote:
> >
> > OK, static inline it is then. Would you be opposed to an API like:
> >
> > get_le16
> > put_le16
> >
> > to match with
> >
> > get_unaligned_le16
> > put_unaligned_le16
> >
> > And make the existing unaligned helpers typesafe?
>
> That sounds much better to me. That said, I'm also wondering what the
> upside is of this all?
>
1) Recognize that some drivers/subsystems want this, or already
implement it privately
2) [micro-optimization] Allow arches that do provide load/store swapped
instructions to be used more often.
3) make it more likely that people will actually use the unaligned
helpers rather than open-coding the byteswapping, allowing arches that
have no alignment constraints to just do regular loads if possible.
Disadvantages:
1) existing users of the get_unaligned bits may/will produce sparse
warnings on the flag day
2) the existing argument ordering of put_unaligned is opposite what
is usually expected in such a helper
3) [nitpicky] get/put almost always means reference taking/releasing,
load/store is the usual verb used for such an API.
Harvey