netdev - Re: [PATCH v5 02/13] net: ethernet: aquantia: Common functions and definitions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Jan 2017 20:32:02 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     Alexander.Loktionov@...antia.com
Cc:     netdev@...r.kernel.org, vomlehn@...as.net,
        Simon.Edelhaus@...antia.com, Dmitrii.Tarakanov@...antia.com,
        Pavel.Belous@...antia.com, Dmitry.Bezrukov@...antia.com
Subject: Re: [PATCH v5 02/13] net: ethernet: aquantia: Common functions and
 definitions

From: Alexander Loktionov <Alexander.Loktionov@...antia.com>
Date: Thu, 12 Jan 2017 21:02:18 -0800

> +#define AQ_OBJ_HEADER spinlock_t lock; atomic_t flags; atomic_t busy_count
> +
> +struct aq_obj_s {
> +	AQ_OBJ_HEADER;
> +};

Please don't hide multiple declarations and types inside of a macro,
that makes the code harder to understand.

Use a sub-structure or similar, and pass that sub-structure to the
handlers.

> +#define AQ_OBJ_TST(_OBJ_, _FLAG_)  ((_FLAG_) & atomic_read(&(_OBJ_)->flags))
> +
> +#define AQ_OBJ_SET(_OBJ_, _F_) \
 ...
> +#define AQ_OBJ_CLR(_OBJ_, _F_) \

Please don't reinvent the wheel.

Use test_bit, set_bit, clear_bit, test_and_set_bit, and
test_and_clear_bit.  Using an atomic_t for flag bits is completely
inappropriate, that type is primarily meant for atomic counters.

The appropriate type for *_bit() operations is "unsigned long".