From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD160C433FE for ; Mon, 28 Nov 2022 13:29:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231777AbiK1N3R (ORCPT ); Mon, 28 Nov 2022 08:29:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54684 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231773AbiK1N3O (ORCPT ); Mon, 28 Nov 2022 08:29:14 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 495D71CFCD for ; Mon, 28 Nov 2022 05:29:12 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 02CB1B80BA6 for ; Mon, 28 Nov 2022 13:29:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DD332C433C1; Mon, 28 Nov 2022 13:29:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1669642149; bh=YzLvejoEX32ZSiRN/J9lyWo8MYx3z7PhhgVHuNta2TY=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=AFN3WKv1EwrTajpkqhqUPx6VFiAzPSKr/3T7ZBn5X4NaUCNEEKzgZ7bADEtAlzp7C 4gNZ5i6lC9IJOz2CIH7Bio5Dnt75ryY+Iq/InMgDqgs64KaQEvK47rNeppLvomE47M Ptgl3NmeqSV54zreWMSXAvyZHH70u8k110UKMYRXH0LOSGvY50ncRhIu5hR9AhYkDT oEfeMLa0UWSY+49fF2Jx3Yj/2yD7WHCc5pbm3zIxPHLSvjAEZqRtgRSTzBbHtRWH+I 4jUgGVSVF6rb9AzKJ7D5DOosKKB0o7C6EDhujrXBhFl5Syy2zwAXcb0/z0n4f13C9h nL+U+0xv5khvQ== From: Kalle Valo To: Ping-Ke Shih Cc: , Subject: Re: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on condition References: <20221118051042.29968-1-pkshih@realtek.com> <20221118051042.29968-4-pkshih@realtek.com> Date: Mon, 28 Nov 2022 15:29:05 +0200 In-Reply-To: <20221118051042.29968-4-pkshih@realtek.com> (Ping-Ke Shih's message of "Fri, 18 Nov 2022 13:10:39 +0800") Message-ID: <87sfi35hsu.fsf@kernel.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org Ping-Ke Shih writes: > From: Zong-Zhe Yang > > MCC (multi-channel concurrency) related H2Cs require to wait for C2H > responses to judge the execution result and data. We introduce helpers > to assist this process. Besides, we would like the helpers to be generic > for use in driver even outside of MCC H2C/C2H, so we make a independent > patch for them. > > In the following, I describe the things first. > ``` > (A) C2H is generated by FW, and then transferred upto driver. Hence, > driver cannot get it immediately without a bit waitting/blocking. > For this, we choose to use wait_for_completion_*() instead of > busy polling. > (B) From the driver management perspective, a scenario, e.g. MCC, > may have mulitple kind of H2C functions requiring this process > to wait for corresponding C2Hs. But, the driver management flow > uses mutex to protect each behavior. So, one scenario triggers > one H2C function at one time. To avoid rampant instances of > struct completion for each H2C function, we choose to use one > struct completion with one condition flag for one scenario. > (C) C2Hs, which H2Cs will be waitting for, cannot be ordered with > driver management flow, i.e. cannot enqueue work to the same > ordered workqueue and cannot lock by the same mutex, to prevent > H2C side from getting no C2H responses. So, those C2Hs are parsed > in interrupt context directly as done in previous commit. > (D) Following (C), the above underline H2Cs and C2Hs will be handled > in different contexts without sync. So, we use atomic_cmpxchg() > to compare and change the condition in atomic. > ``` > > So, we introduce struct rtw89_wait_info which combines struct completion > and atomic_t. Then, the below are the descriptions for helper functions. > * rtw89_wait_for_cond() to wait for a completion based on a condition. > * rtw89_complete_cond() to complete a given condition and carry data. > Each rtw89_wait_info instance independently determines the meaning of > its waitting conditions. But, RTW89_WAIT_COND_IDLE (UINT_MAX) is reserved. > > Signed-off-by: Zong-Zhe Yang > Signed-off-by: Ping-Ke Shih Just nitpicking a couple of items: Otherwise an excellent commit log but the meaning of C2H and H2C is not clear for me. I guess they mean "chip to host" and "host to chip", but would be good to clarify that in the beginning. > --- a/drivers/net/wireless/realtek/rtw89/core.h > +++ b/drivers/net/wireless/realtek/rtw89/core.h > @@ -2802,6 +2802,34 @@ struct rtw89_mac_info { > u8 cpwm_seq_num; > }; > > +struct rtw89_completion_data { > + bool err; > +#define RTW89_COMPLETION_BUF_SIZE 24 > + u8 buf[RTW89_COMPLETION_BUF_SIZE]; > +}; Having a define withing a struct looks odd to me, I would prefer to have it outside of the struct. > +#define rtw89_completion_cast(cmpl_data, ptr) \ > +({ \ > + typecheck(struct rtw89_completion_data *, cmpl_data); \ > + BUILD_BUG_ON(sizeof(*(ptr)) > RTW89_COMPLETION_BUF_SIZE); \ > + (typeof(ptr))(cmpl_data)->buf; \ > +}) Wouldn't this be cleaner as a static inline function? > +struct rtw89_wait_info { > +#define RTW89_WAIT_COND_IDLE UINT_MAX > + atomic_t cond; > + struct completion completion; > + struct rtw89_completion_data data; > +}; Also here would prefer the define outside the struct. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches