RE: [PATCH 3/4] rtw88: support adaptivity for ETSI/JP DFS region

[Pkshih <pkshih@realtek.com>]

> -----Original Message-----
> From: kvalo=codeaurora.org@mg.codeaurora.org [mailto:kvalo=codeaurora.org@mg.codeaurora.org] On
> Behalf Of Kalle Valo
> Sent: Sunday, August 29, 2021 7:40 PM
> To: Pkshih
> Cc: tony0620emma@gmail.com; linux-wireless@vger.kernel.org; Timlee; Kevin Yang
> Subject: Re: [PATCH 3/4] rtw88: support adaptivity for ETSI/JP DFS region
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> >
> > Add Energy Detected CCA (EDCCA) mechanism to detect energy on the channel.
> > And EDCCA support adaptivity mode now. From MIC Ordinance Regulating Radio
> > Equipment article 49.20, ETSI EN-300-328 and EN-301-893, the device should
> > be able to dynamically pause TX activity when energy detected on the air.
> > According to ETSI/JP DFS region, driver will set corresponding threshold
> > and stop TX activity if the detected energy exceeds the threshold. For now,
> > we support it on 8822b and 8822c first.
> >
> > By default, EDCCA mechanism is turned on. For ETSI/JP DFS region, it will
> > turn to adaptivity mode. However, with adaptivity, if environment is too
> > nosiy, TX may often be halted. So, a debugfs for EDCCA is added. It can
> > show what EDCCA mode is used currently. And EDCCA mechanism can be turned
> > on/off through the debugfs while debugging.
> >
> > Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> [...]
> 
> > +static ssize_t rtw_debugfs_set_edcca_enable(struct file *filp,
> > +					    const char __user *buffer,
> > +					    size_t count, loff_t *loff)
> > +{
> > +	struct seq_file *seqpriv = (struct seq_file *)filp->private_data;
> > +	struct rtw_debugfs_priv *debugfs_priv = seqpriv->private;
> > +	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
> > +	bool input;
> > +	int err;
> > +
> > +	err = kstrtobool_from_user(buffer, count, &input);
> > +	if (err)
> > +		return err;
> > +
> > +	rtw_edcca_enabled = input;
> > +	rtw_phy_adaptivity_set_mode(rtwdev);
> > +
> > +	return count;
> > +}
> > +
> > +static int rtw_debugfs_get_edcca_enable(struct seq_file *m, void *v)
> > +{
> > +	struct rtw_debugfs_priv *debugfs_priv = m->private;
> > +	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
> > +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> > +
> > +	seq_printf(m, "EDCCA %s: EDCCA mode %d\n",
> > +		   rtw_edcca_enabled ? "enabled" : "disabled",
> > +		   dm_info->edcca_mode);
> > +	return 0;
> > +}
> 
> [...]
> 
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > @@ -23,6 +23,7 @@ EXPORT_SYMBOL(rtw_disable_lps_deep_mode);
> >  bool rtw_bf_support = true;
> >  unsigned int rtw_debug_mask;
> >  EXPORT_SYMBOL(rtw_debug_mask);
> > +bool rtw_edcca_enabled = true;
> 
> You are making rtw_edcca_enabled per driver, should it instead be per
> device? If something is changed via debugfs the assumption is that the
> value is per device.
> 
> For example, let's say you have two rtw88 devices attached on the same
> system, device A and B. If a user changes edcca via debugfs on device A
> it will also change the state in device B. Is that the desired
> functionality? If yes, I think you should add a comment to
> rtw_edcca_enabled explaining that.
> 

As mentioned in commit message, the debugfs is expected to be used when debugging
in noisy environment. In that case, we think all rtw88 devices will probably
be affected. Besides, we believe that turning EDCCA off is a temporary state under
debugging.

Based on the two points, it seems no need to maintain rtw_edcca_enabled by device,
so a comment will be added to explain this.

--
Ping-Ke