by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on condition

> -----Original Message-----
> From: Kalle Valo <[email protected]>
> Sent: Monday, November 28, 2022 9:29 PM
> To: Ping-Ke Shih <[email protected]>
> Cc: Kevin Yang <[email protected]>; [email protected]
> Subject: Re: [PATCH 3/6] wifi: rtw89: introduce helpers to wait/complete on condition
>
> Ping-Ke Shih <[email protected]> writes:
>
> > From: Zong-Zhe Yang <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Ping-Ke Shih <[email protected]>
>
> 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

2022-11-29 05:57:54

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 4/6] wifi: rtw89: mac: process MCC related C2H

Ping-Ke Shih <[email protected]> writes:

>> -----Original Message-----
>> From: Kalle Valo <[email protected]>
>> Sent: Monday, November 28, 2022 9:31 PM
>> To: Ping-Ke Shih <[email protected]>
>> Cc: Kevin Yang <[email protected]>; [email protected]
>> Subject: Re: [PATCH 4/6] wifi: rtw89: mac: process MCC related C2H
>>
>> Ping-Ke Shih <[email protected]> writes:
>>
>> > From: Zong-Zhe Yang <[email protected]>
>> >
>> > Process C2H(s) related to MCC (multi-channel concurrency). These handling,
>> > which either call rtw89_complete_cond() or show message in debug mode, can
>> > be considered atomic/lock-free. So, they should be safe to be processed
>> > directly after C2H pre-check in previous patch.
>> >
>> > Signed-off-by: Zong-Zhe Yang <[email protected]>
>> > Signed-off-by: Ping-Ke Shih <[email protected]>
>>
>> [...]
>>
>> > +static
>> > +void (* const rtw89_mac_c2h_mcc_handler[])(struct rtw89_dev *rtwdev,
>> > + struct sk_buff *c2h, u32 len) = {
>> > + [RTW89_MAC_C2H_FUNC_MCC_RCV_ACK] = rtw89_mac_c2h_mcc_rcv_ack,
>> > + [RTW89_MAC_C2H_FUNC_MCC_REQ_ACK] = rtw89_mac_c2h_mcc_req_ack,
>> > + [RTW89_MAC_C2H_FUNC_MCC_TSF_RPT] = rtw89_mac_c2h_mcc_tsf_rpt,
>> > + [RTW89_MAC_C2H_FUNC_MCC_STATUS_RPT] = rtw89_mac_c2h_mcc_status_rpt,
>> > +};
>>
>> static const?
>>
>
> These are function pointers, so const is before instance name,
> i.e. '... const rtw89_mac_c2h_mcc_handler ..'

Ah indeed, missed that. Thanks.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches