Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 1C09DC4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     <tony0620emma@gmail.com>, <linux-wireless@vger.kernel.org>,
        <timlee@realtek.com>, <kevin_yang@realtek.com>
Subject: Re: [PATCH 3/4] rtw88: support adaptivity for ETSI/JP DFS region
References: <20210802063140.25670-1-pkshih@realtek.com>
        <20210802063140.25670-4-pkshih@realtek.com>
Date:   Sun, 29 Aug 2021 14:40:12 +0300
In-Reply-To: <20210802063140.25670-4-pkshih@realtek.com> (Ping-Ke Shih's
        message of "Mon, 2 Aug 2021 14:31:39 +0800")
Message-ID: <87ilzo4gyr.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.

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

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