Re: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

Re: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240813055917.2320582-1-quic_periyasa@quicinc.com>
References: <20240813055917.2320582-1-quic_periyasa@quicinc.com>
TO: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
TO: johannes@sipsolutions.net
CC: linux-wireless@vger.kernel.org
CC: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>

Hi Karthikeyan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cc32e9fb380d8afdbf3486d7063d5520bfb0f071]

url:    https://github.com/intel-lab-lkp/linux/commits/Karthikeyan-Periyasamy/wifi-cfg80211-check-radio-iface-combination-for-multi-radio-per-wiphy/20240815-005751
base:   cc32e9fb380d8afdbf3486d7063d5520bfb0f071
patch link:    https://lore.kernel.org/r/20240813055917.2320582-1-quic_periyasa%40quicinc.com
patch subject: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: csky-randconfig-r072-20240819 (https://download.01.org/0day-ci/archive/20240820/202408200004.5sqDWe4e-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202408200004.5sqDWe4e-lkp@intel.com/

smatch warnings:
net/wireless/core.c:719 wiphy_verify_combinations() error: uninitialized symbol 'ret'.

vim +/ret +719 net/wireless/core.c

7527a782e187d1 Johannes Berg          2011-05-13  698  
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  699  static int wiphy_verify_combinations(struct wiphy *wiphy)
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  700  {
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  701  	int i, ret;
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  702  
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  703  	if (wiphy->n_radio) {
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  704  		for (i = 0; i < wiphy->n_radio; i++) {
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  705  			const struct wiphy_radio *radio = &wiphy->radio[i];
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  706  
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  707  			ret = wiphy_verify_iface_combinations(wiphy,
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  708  							      radio->iface_combinations,
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  709  							      radio->n_iface_combinations);
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  710  			if (ret)
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  711  				return ret;
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  712  		}
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  713  	} else {
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  714  		ret = wiphy_verify_iface_combinations(wiphy,
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  715  						      wiphy->iface_combinations,
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  716  						      wiphy->n_iface_combinations);
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  717  	}
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  718  
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13 @719  	return ret;
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  720  }
acac11b3dc4e4f Karthikeyan Periyasamy 2024-08-13  721  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
due to the condition check on global interface combination that DFS only
works on one channel. In a multi-radio scenario, global interface
combination encompasses the capabilities of all radio combinations, so it
supports more than one channel with DFS. For multi-radio per wiphy,
interface combination verification needs to be performed for radio specific
interface combinations. This is necessary as the global interface
combination combines the capabilities of all radio combinations.

Fixes: a01b1e9f9955 ("wifi: mac80211: add support for DFS with multiple radios")
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 v2:
  - Rebased to ToT

 net/wireless/core.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4d5d351bd0b5..de33bdde1e29 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -603,16 +603,19 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 }
 EXPORT_SYMBOL(wiphy_new_nm);
 
-static int wiphy_verify_combinations(struct wiphy *wiphy)
+static
+int wiphy_verify_iface_combinations(struct wiphy *wiphy,
+				    const struct ieee80211_iface_combination *iface_comb,
+				    int n_iface_comb)
 {
 	const struct ieee80211_iface_combination *c;
 	int i, j;
 
-	for (i = 0; i < wiphy->n_iface_combinations; i++) {
+	for (i = 0; i < n_iface_comb; i++) {
 		u32 cnt = 0;
 		u16 all_iftypes = 0;
 
-		c = &wiphy->iface_combinations[i];
+		c = &iface_comb[i];
 
 		/*
 		 * Combinations with just one interface aren't real,
@@ -693,6 +696,29 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 	return 0;
 }
 
+static int wiphy_verify_combinations(struct wiphy *wiphy)
+{
+	int i, ret;
+
+	if (wiphy->n_radio) {
+		for (i = 0; i < wiphy->n_radio; i++) {
+			const struct wiphy_radio *radio = &wiphy->radio[i];
+
+			ret = wiphy_verify_iface_combinations(wiphy,
+							      radio->iface_combinations,
+							      radio->n_iface_combinations);
+			if (ret)
+				return ret;
+		}
+	} else {
+		ret = wiphy_verify_iface_combinations(wiphy,
+						      wiphy->iface_combinations,
+						      wiphy->n_iface_combinations);
+	}
+
+	return ret;
+}
+
 int wiphy_register(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);

base-commit: cc32e9fb380d8afdbf3486d7063d5520bfb0f071
-- 
2.34.1

Re: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

Hi,

So ... I don't think this is correct, for multiple reasons.

One, you're completely removing *any* validation of the (global)
interface combinations in case per-radio interface combinations are
present. This is clearly not desirable. You're arguing the DFS vs.
multi-channel check shouldn't be done, but that doesn't imply removing
all checks entirely.

Secondly, I'm not convinced that the DFS vs. multi-channel check should
actually be removed, though I'll admit that this may be a bit
questionable. My argument would be something like this: The global
interface combinations exist to let existing software (that isn't aware
of multi-radio yet) continue functioning as-is. Since it is not radio-
aware, multi-channel can mean many different things to it, including the
ability to use say two 2.4 GHz channels at the same time, by time-
sharing. This is e.g. used to support concurrent P2P-GO and (BSS) client
today. But DFS capability on this is broken, since you're not on the
correct channel all the time, hence the check.

Therefore, I think this patch is entirely wrong, and you need to
advertise only a lowest common denominator on the global interface
combinations (where the num_different_channels is actually possible to
support on each individual radio, since no band separating is implied by
it).

johannes

Re: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

Oh and with all that said - we still need a patch like this, but it
should check all radios *and* the global combinations.

johannes

Re: [PATCH v2] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

On 8/28/2024 4:19 PM, Johannes Berg wrote:
> Hi,
> 
> So ... I don't think this is correct, for multiple reasons.
> 
> One, you're completely removing *any* validation of the (global)
> interface combinations in case per-radio interface combinations are
> present. This is clearly not desirable. You're arguing the DFS vs.
> multi-channel check shouldn't be done, but that doesn't imply removing
> all checks entirely.
> 

In case of per-radio interface combinations, two global interface 
combinations advertised.

1. NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS (new global - supported 
interface combinations for all radios combined)

nl80211.h

@NL80211_ATTR_WIPHY_RADIOS: Nested attribute describing physical radios
      belonging to this wiphy. See &enum nl80211_wiphy_radio_attrs.

@NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS: Nested attribute listing the
      supported interface combinations for all radios combined. In each
      nested item, it contains attributes defined in
      &enum nl80211_if_combination_attrs.


2. NL80211_ATTR_INTERFACE_COMBINATIONS (exist global - supported 
interface combination of zeroth radio )

since new global is combined of all radios, so it fails most of the 
check in wiphy_verify_combinations() like DFS only works on one channel, 
Only a single P2P_DEVICE can be allowed.

> Secondly, I'm not convinced that the DFS vs. multi-channel check should
> actually be removed, though I'll admit that this may be a bit
> questionable. My argument would be something like this: The global
> interface combinations exist to let existing software (that isn't aware
> of multi-radio yet) continue functioning as-is. Since it is not radio-
> aware, multi-channel can mean many different things to it, including the
> ability to use say two 2.4 GHz channels at the same time, by time-
> sharing. This is e.g. used to support concurrent P2P-GO and (BSS) client
> today. But DFS capability on this is broken, since you're not on the
> correct channel all the time, hence the check.
> 

Exist global interface combination is advertise the zeroth radio iface 
combination. So it is validated as part of per-radio interface 
combination check.


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி