> -----Original Message-----
> From: Morten Brørup <m...@smartsharesystems.com>
> Sent: Friday, April 17, 2020 2:55 AM
> To: Joyce Kong <joyce.k...@arm.com>; tho...@monjalon.net;
> step...@networkplumber.org; david.march...@redhat.com;
> jer...@marvell.com; bruce.richard...@intel.com; ravi1.ku...@amd.com;
> rm...@marvell.com; shsha...@marvell.com; xuanziya...@huawei.com;
> cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com; Honnappa
> Nagarahalli <honnappa.nagaraha...@arm.com>; Gavin Hu
> <gavin...@arm.com>; Phil Yang <phil.y...@arm.com>
> Cc: nd <n...@arm.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v8 1/6] lib/eal: implement the family of
> commonbit operation APIs
>
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Joyce Kong
> > Sent: Thursday, April 16, 2020 7:39 AM
> >
> > Bitwise operation APIs are defined and used in a lot of PMDs, which
> > caused a huge code duplication. To reduce duplication, this patch
> > consolidates them into a common API family.
> >
>
> [...]
>
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 32-bit value, then set it to 1
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and set.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_test_and_set_bit32_relaxed(unsigned int nr, volatile uint32_t
> > +*addr) {
> > + RTE_ASSERT(nr < 32);
> > +
> > + uint32_t mask = UINT32_C(1) << nr;
> > + uint32_t val = *addr;
> > + *addr = (*addr) | mask;
>
> Suggestion:
> - *addr = (*addr) | mask;
> + *addr = val | mask;
>
Shall take the advice in v9.
Thanks,
Joyce
> > + return val & mask;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 32-bit value, then clear it to 0
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and clear.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_test_and_clear_bit32_relaxed(unsigned int nr, volatile uint32_t
> > +*addr) {
> > + RTE_ASSERT(nr < 32);
> > +
> > + uint32_t mask = UINT32_C(1) << nr;
> > + uint32_t val = *addr;
> > + *addr = (*addr) & (~mask);
>
> Suggestion:
> - *addr = (*addr) & (~mask);
> + *addr = val & (~mask);
>
Shall take the advice in v9.
> > + return val & mask;
> > +}
> > +
> > +/*---------------------------- 64 bit operations
> > +-------------------------
> > ---*/
> > +
>
> [...]
>
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 64-bit value, then set it to 1
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and set.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint64_t
> > +rte_test_and_set_bit64_relaxed(unsigned int nr, volatile uint64_t
> > +*addr) {
> > + RTE_ASSERT(nr < 64);
> > +
> > + uint64_t mask = UINT64_C(1) << nr;
> > + uint64_t val = *addr;
> > + *addr = (*addr) | mask;
>
> Suggestion:
> - *addr = (*addr) | mask;
> + *addr = val | mask;
>
Shall take the advice in v9.
> > + return val;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Return the original bit from a 64-bit value, then clear it to 0
> > +without
> > + * memory ordering.
> > + *
> > + * @param nr
> > + * The target bit to get and clear.
> > + * @param addr
> > + * The address holding the bit.
> > + * @return
> > + * The original bit.
> > + */
> > +__rte_experimental
> > +static inline uint64_t
> > +rte_test_and_clear_bit64_relaxed(unsigned int nr, volatile uint64_t
> > +*addr) {
> > + RTE_ASSERT(nr < 64);
> > +
> > + uint64_t mask = UINT64_C(1) << nr;
> > + uint64_t val = *addr;
> > + *addr = (*addr) & (~mask);
>
> Suggestion:
> - *addr = (*addr) & (~mask);
> + *addr = val & (~mask);
>
Shall take the advice in v9.
> > + return val & mask;
> > +}
> > +
> > +#endif /* _RTE_BITOPS_H_ */
> > --
> > 2.17.1
> >
