| Message ID | 20221118051042.29968-4-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: rtw89: preparation of MCC | expand |
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. > --- 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.
> -----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
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c index f30aadc41f2be..e0b044a33207a 100644 --- a/drivers/net/wireless/realtek/rtw89/core.c +++ b/drivers/net/wireless/realtek/rtw89/core.c @@ -2941,6 +2941,41 @@ void rtw89_core_update_beacon_work(struct work_struct *work) mutex_unlock(&rtwdev->mutex); } +int rtw89_wait_for_cond(struct rtw89_wait_info *wait, unsigned int cond) +{ + struct completion *cmpl = &wait->completion; + unsigned long timeout; + unsigned int cur; + + cur = atomic_cmpxchg(&wait->cond, RTW89_WAIT_COND_IDLE, cond); + if (cur != RTW89_WAIT_COND_IDLE) + return -EBUSY; + + timeout = wait_for_completion_timeout(cmpl, RTW89_WAIT_FOR_COND_TIMEOUT); + if (timeout == 0) { + atomic_set(&wait->cond, RTW89_WAIT_COND_IDLE); + return -ETIMEDOUT; + } + + if (wait->data.err) + return -EFAULT; + + return 0; +} + +void rtw89_complete_cond(struct rtw89_wait_info *wait, unsigned int cond, + const struct rtw89_completion_data *data) +{ + unsigned int cur; + + cur = atomic_cmpxchg(&wait->cond, cond, RTW89_WAIT_COND_IDLE); + if (cur != cond) + return; + + wait->data = *data; + complete(&wait->completion); +} + int rtw89_core_start(struct rtw89_dev *rtwdev) { int ret; diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h index 3fff666c0e84a..5a7d5514bba9a 100644 --- 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]; +}; + +#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; \ +}) + +struct rtw89_wait_info { +#define RTW89_WAIT_COND_IDLE UINT_MAX + atomic_t cond; + struct completion completion; + struct rtw89_completion_data data; +}; + +#define RTW89_WAIT_FOR_COND_TIMEOUT msecs_to_jiffies(100) + +static inline void rtw89_init_wait(struct rtw89_wait_info *wait) +{ + init_completion(&wait->completion); + atomic_set(&wait->cond, RTW89_WAIT_COND_IDLE); +} + enum rtw89_fw_type { RTW89_FW_NORMAL = 1, RTW89_FW_WOWLAN = 3, @@ -4440,6 +4468,9 @@ int rtw89_regd_init(struct rtw89_dev *rtwdev, void rtw89_regd_notifier(struct wiphy *wiphy, struct regulatory_request *request); void rtw89_traffic_stats_init(struct rtw89_dev *rtwdev, struct rtw89_traffic_stats *stats); +int rtw89_wait_for_cond(struct rtw89_wait_info *wait, unsigned int cond); +void rtw89_complete_cond(struct rtw89_wait_info *wait, unsigned int cond, + const struct rtw89_completion_data *data); int rtw89_core_start(struct rtw89_dev *rtwdev); void rtw89_core_stop(struct rtw89_dev *rtwdev); void rtw89_core_update_beacon_work(struct work_struct *work);