Message ID | 1424702463-10012-1-git-send-email-aa@ocedo.com |
---|---|
State | Changes Requested |
Delegated to: | Jonas Gorski |
Headers | show |
Hi, On Mon, Feb 23, 2015 at 3:41 PM, Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > Converts an MAC array of u8 to a u64 value. > > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > --- > target/linux/generic/files/drivers/net/phy/b53/b53_regs.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > index ba50915..4379c58 100644 > --- a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > @@ -20,6 +20,16 @@ > #ifndef __B53_REGS_H > #define __B53_REGS_H > > +/* Utility function for converting u8 arrays into u64 values to be written with b53_write */ You only use this in b53_common.c, so why not just have it in there? And maybe merge it into the patch atually adding a user. > +static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) { > + u64 mac = (*(const u64 *)u8_arr); Also this will likely produce alignment issues on e.g. mips, which doesn't allow unaligned accesses. > +#ifdef __BIG_ENDIAN > + return (mac >> 16); > +#else > + return (mac << 16); > +#endif > +} > + > /* Management Port (SMP) Page offsets */ > #define B53_CTRL_PAGE 0x00 /* Control */ > #define B53_STAT_PAGE 0x01 /* Status */ > -- Jonas
So, on a powerpc system this works. static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) { u64 mac = 0; u8 *cmac = (u8 *)&mac; memcpy(&cmac[2], u8_arr, 6); return mac; } I've done this approach initially, but there were some concerns afterwards regarding endianness. On my x86_64 system it looks ok, but I'm hoping you'd validate that this is endian-correct and would work on little endian targets. And then I'll move this in the port mirroring patch. Thanks On Fri, Feb 27, 2015 at 8:36 PM, Jonas Gorski <jogo@openwrt.org> wrote: > Hi, > > On Mon, Feb 23, 2015 at 3:41 PM, Alexandru Ardelean > <ardeleanalex@gmail.com> wrote: > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > > > Converts an MAC array of u8 to a u64 value. > > > > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > > --- > > target/linux/generic/files/drivers/net/phy/b53/b53_regs.h | 10 > ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > > index ba50915..4379c58 100644 > > --- a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > > +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h > > @@ -20,6 +20,16 @@ > > #ifndef __B53_REGS_H > > #define __B53_REGS_H > > > > +/* Utility function for converting u8 arrays into u64 values to be > written with b53_write */ > > You only use this in b53_common.c, so why not just have it in there? > And maybe merge it into the patch atually adding a user. > > > +static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) { > > + u64 mac = (*(const u64 *)u8_arr); > > Also this will likely produce alignment issues on e.g. mips, which > doesn't allow unaligned accesses. > > > +#ifdef __BIG_ENDIAN > > + return (mac >> 16); > > +#else > > + return (mac << 16); > > +#endif > > +} > > + > > /* Management Port (SMP) Page offsets */ > > #define B53_CTRL_PAGE 0x00 /* Control */ > > #define B53_STAT_PAGE 0x01 /* Status */ > > -- > > Jonas >
On Mon, Mar 2, 2015 at 10:44 AM, Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > So, on a powerpc system this works. > > static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) { > u64 mac = 0; > u8 *cmac = (u8 *)&mac; > memcpy(&cmac[2], u8_arr, 6); > return mac; > } > > I've done this approach initially, but there were some concerns afterwards > regarding endianness. > On my x86_64 system it looks ok, but I'm hoping you'd validate that this is > endian-correct and would work on little endian targets. > And then I'll move this in the port mirroring patch. Please don't top post. Hm, looking a the original patch, did you test this in little endian? the shift looks wrong for there. Same for the memcpy, you need to copy to cmac[0] in case of little endian. Maybe it would be easier to just make the source mac u16 aligned, and then use ether_addr_copy(). Jonas
diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h index ba50915..4379c58 100644 --- a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h @@ -20,6 +20,16 @@ #ifndef __B53_REGS_H #define __B53_REGS_H +/* Utility function for converting u8 arrays into u64 values to be written with b53_write */ +static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) { + u64 mac = (*(const u64 *)u8_arr); +#ifdef __BIG_ENDIAN + return (mac >> 16); +#else + return (mac << 16); +#endif +} + /* Management Port (SMP) Page offsets */ #define B53_CTRL_PAGE 0x00 /* Control */ #define B53_STAT_PAGE 0x01 /* Status */