Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1185-g841157300a-fm-20221208.002-g84115730
Mime-Version: 1.0
Message-Id: <486f9bc9-408f-4c29-b675-cbd61673f58c@app.fastmail.com>
In-Reply-To: <87k02sd1uz.fsf@toke.dk>
References: <20221215165553.1950307-1-arnd@kernel.org>
 <87k02sd1uz.fsf@toke.dk>
Date:   Fri, 16 Dec 2022 12:16:17 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>,
        "Arnd Bergmann" <arnd@kernel.org>, "Kalle Valo" <kvalo@kernel.org>,
        "Pavel Skripkin" <paskripkin@gmail.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>,
        "Paolo Abeni" <pabeni@redhat.com>,
        "Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp>,
        linux-wireless@vger.kernel.org, Netdev <netdev@vger.kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ath9k: use proper statements in conditionals
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Dec 15, 2022, at 18:16, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> index 30f0765fb9fd..237f4ec2cffd 100644
>> --- a/drivers/net/wireless/ath/ath9k/htc.h
>> +++ b/drivers/net/wireless/ath/ath9k/htc.h
>> @@ -327,9 +327,9 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB=
(struct sk_buff *skb)
>>  }
>> =20
>>  #ifdef CONFIG_ATH9K_HTC_DEBUGFS
>> -#define __STAT_SAFE(hif_dev, expr)	((hif_dev)->htc_handle->drv_priv =
? (expr) : 0)
>> -#define CAB_STAT_INC(priv)		((priv)->debug.tx_stats.cab_queued++)
>> -#define TX_QSTAT_INC(priv, q)		((priv)->debug.tx_stats.queue_stats[q=
]++)
>> +#define __STAT_SAFE(hif_dev, expr)	do { ((hif_dev)->htc_handle->drv_=
priv ? (expr) : 0); } while (0)
>> +#define CAB_STAT_INC(priv)		do { ((priv)->debug.tx_stats.cab_queued+=
+); } while (0)
>> +#define TX_QSTAT_INC(priv, q)		do { ((priv)->debug.tx_stats.queue_st=
ats[q]++); } while (0)
>
> Hmm, is it really necessary to wrap these in do/while constructs? AFAI=
CT
> they're all simple statements already?

It's generally safer to do the same thing on both side of the #ifdef.

The "do { } while (0)" is an empty statement that is needed to fix
the bug on the #else side. The expressions you have on the #ifdef
side can be used as values, and wrapping them in do{}while(0)
turns them into statements (without a value) as well, so fewer
things can go wrong when you only test one side.

I suppose the best solution would be to just use inline functions
for all of them and get rid of the macros.

     Arnd