Re: No need to second guess SXIE_ROUNDUP

[Artturi Alm]

On Thu, Jun 09, 2016 at 12:04:02AM +0100, Tom Cosgrove wrote:
> If (pktlen & 3) == 0, SXIE_ROUNDUP returns pktlen anyway (that's its job):
> it's defined as
> 
> #define SXIE_ROUNDUP(size, unit) (((size) + (unit) - 1) & ~((unit) - 1))
> 
> Thanks
> 
> Tom
> 

Hi,

starting w/bikeshed i'd go for removing rlen and just do pktlen+3, because
of >> 2, but seriously... does this possibly mean you think there was
something wrong with the diff i sent less than 24hrs ago that your diff does
nothing but stomp on? if so, do write your concerns out loud. i do care about
feedback. and in case you honestly were unaware of my diff, apologies, but i
really think leaving rlen and using SXIE_ROUNDUP at all is unnecessary.
and you missed unused variable in the other function using SXIE_ROUNDUP that
you certainly did check i hope, heh.

-Artturi

http://marc.info/?l=openbsd-tech=146537914916112=2

> 
> Index: sys/arch/armv7/sunxi/sxie.c
> ===
> RCS file: /home/OpenBSD/cvs/src/sys/arch/armv7/sunxi/sxie.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 sxie.c
> --- sys/arch/armv7/sunxi/sxie.c   13 Apr 2016 11:34:00 -  1.14
> +++ sys/arch/armv7/sunxi/sxie.c   8 Jun 2016 23:03:46 -
> @@ -606,10 +606,7 @@ trynext:
>   m_adj(m, ETHER_ALIGN);
>  
>   /* read the actual packet from fifo XXX through 'align buffer'.. */
> - if (pktlen & 3)
> - rlen = SXIE_ROUNDUP(pktlen, 4);
> - else
> - rlen = pktlen;
> + rlen = SXIE_ROUNDUP(pktlen, 4);
>   bus_space_read_multi_4(sc->sc_iot, sc->sc_ioh,
>   SXIE_RXIO, (uint32_t *)[0], rlen >> 2);
>   memcpy(mtod(m, char *), (char *)[0], pktlen);
> 

Re: No need to second guess SXIE_ROUNDUP

[David Gwynne]

why not roundup() from src/sys/sys/param.h?

> On 9 Jun 2016, at 09:04, Tom Cosgrove  
> wrote:
> 
> If (pktlen & 3) == 0, SXIE_ROUNDUP returns pktlen anyway (that's its job):
> it's defined as
> 
>#define SXIE_ROUNDUP(size, unit) (((size) + (unit) - 1) & ~((unit) - 1))
> 
> Thanks
> 
> Tom
> 
> 
> Index: sys/arch/armv7/sunxi/sxie.c
> ===
> RCS file: /home/OpenBSD/cvs/src/sys/arch/armv7/sunxi/sxie.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 sxie.c
> --- sys/arch/armv7/sunxi/sxie.c   13 Apr 2016 11:34:00 -  1.14
> +++ sys/arch/armv7/sunxi/sxie.c   8 Jun 2016 23:03:46 -
> @@ -606,10 +606,7 @@ trynext:
>   m_adj(m, ETHER_ALIGN);
> 
>   /* read the actual packet from fifo XXX through 'align buffer'.. */
> - if (pktlen & 3)
> - rlen = SXIE_ROUNDUP(pktlen, 4);
> - else
> - rlen = pktlen;
> + rlen = SXIE_ROUNDUP(pktlen, 4);
>   bus_space_read_multi_4(sc->sc_iot, sc->sc_ioh,
>   SXIE_RXIO, (uint32_t *)[0], rlen >> 2);
>   memcpy(mtod(m, char *), (char *)[0], pktlen);
> 

No need to second guess SXIE_ROUNDUP

[Tom Cosgrove]

If (pktlen & 3) == 0, SXIE_ROUNDUP returns pktlen anyway (that's its job):
it's defined as

#define SXIE_ROUNDUP(size, unit) (((size) + (unit) - 1) & ~((unit) - 1))

Thanks

Tom


Index: sys/arch/armv7/sunxi/sxie.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/armv7/sunxi/sxie.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 sxie.c
--- sys/arch/armv7/sunxi/sxie.c 13 Apr 2016 11:34:00 -  1.14
+++ sys/arch/armv7/sunxi/sxie.c 8 Jun 2016 23:03:46 -
@@ -606,10 +606,7 @@ trynext:
m_adj(m, ETHER_ALIGN);
 
/* read the actual packet from fifo XXX through 'align buffer'.. */
-   if (pktlen & 3)
-   rlen = SXIE_ROUNDUP(pktlen, 4);
-   else
-   rlen = pktlen;
+   rlen = SXIE_ROUNDUP(pktlen, 4);
bus_space_read_multi_4(sc->sc_iot, sc->sc_ioh,
SXIE_RXIO, (uint32_t *)[0], rlen >> 2);
memcpy(mtod(m, char *), (char *)[0], pktlen);