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;
Authenticated-By: 
From:   Ping-Ke Shih <pkshih@realtek.com>
To:     Kalle Valo <kvalo@kernel.org>
CC:     Kevin Yang <kevin_yang@realtek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on condition
Thread-Topic: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on
 condition
Thread-Index: AQHY+wxGkFx0DY+dqU+L+TRf49zoQK5UZLq/gAD3tKA=
Date:   Tue, 29 Nov 2022 04:18:12 +0000
Message-ID: <019c1e3d4fb54b189b98e8b238fe3c9c@realtek.com>
References: <20221118051042.29968-1-pkshih@realtek.com>
        <20221118051042.29968-4-pkshih@realtek.com> <87sfi35hsu.fsf@kernel.org>
In-Reply-To: <87sfi35hsu.fsf@kernel.org>
Accept-Language: en-US, zh-TW
Content-Language: zh-TW
x-kse-attachmentfiltering-interceptor-info: no applicable attachment filtering
 rules found
x-kse-antivirus-interceptor-info: scan successful
x-kse-antivirus-info: =?us-ascii?Q?Clean,_bases:_2022/11/28_=3F=3F_11:42:00?=
x-kse-bulkmessagesfiltering-scan-result: protection disabled
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE,
        SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org


> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Monday, November 28, 2022 9:29 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Kevin Yang <kevin_yang@realtek.com>; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on condition
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> >
> > 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 <kevin_yang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> 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.

will add them by v2.

> 
> > --- 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.

will fix it by v2.

> 
> > +#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?

inline function isn't suitable, because 'ptr' could be various type.
We plan to do casting barely at callers.

> 
> > +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.

will fix it by v2.

Thank you
Ping-Ke