Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes:
> From: Zong-Zhe Yang <kevin_yang@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
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
