Re: [dpdk-dev] [PATCH v2 4/6] net/ngbe: add device init and uninit

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Saturday, March 20, 2021 1:47 AM, Stephen Hemminger wrote:
> On Fri, 19 Mar 2021 15:26:26 +0800
> Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> 
> > +++ b/drivers/net/ngbe/base/ngbe_osdep.h
> > @@ -0,0 +1,172 @@
> 
> > +#define false               0
> > +#define true                1
> 
> These will conflict with normal definitions in <stdbool.h> Why are you not
using
> that?

I think, "#ifndef false" and "#ifndef true" can be used to avoid adding
<stdbool.h> repeatedly.

> 
> > +#define min(a, b)	RTE_MIN(a, b)
> > +#define max(a, b)	RTE_MAX(a, b)
> > +
> > +/* Bunch of defines for shared code bogosity */
> > +
> > +static inline void UNREFERENCED(const char *a __rte_unused, ...) {}
> > +#define UNREFERENCED_PARAMETER(args...) UNREFERENCED("", ##args)
> > +
> 
> I have seen that before... porting code from Windows?

I am not sure if it was ported from Windows.
It is ported from TXGBE, and TXGBE was coding by another workmate several
years before.

> 
> > +/* Check whether address is multicast. This is little-endian specific
> > +check.*/ #define NGBE_IS_MULTICAST(address) \
> > +		(bool)(((u8 *)(address))[0] & ((u8)0x01))
> > +
> 
> Why not rte_ether_is_multicast_ether_addr?
> 
> 
> > +/* Check whether an address is broadcast. */ #define
> > +NGBE_IS_BROADCAST(address) \
> > +		({typeof(address)addr = (address); \
> > +		(((u8 *)(addr))[0] == ((u8)0xff)) && \
> > +		(((u8 *)(addr))[1] == ((u8)0xff)); })
> > +
> 
> Your code is not correct since it would match ff:ff:01:02:03:04
> 
> Why not rte_ether_is_broadcast_ether_addr?

Thanks for your review.
Such in the case, rte_is_multicast_ether_addr and
rte_is_broadcast_ether_addr should be used.