[OpenWrt-Devel,1/3] b53: add b53_mac_array_to_u64() utility function

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(+)

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