[PATCH 1/4] cfg80211: rdev-ops: remove callback check from rdev_set_coalesce()

[PATCH 1/4] cfg80211: rdev-ops: remove callback check from rdev_set_coalesce()

[Arend van Spriel]

The wrapper rdev_set_coalesce() checks whether the driver provides
the set_coalesce callback and returns -ENOTSUPP if not. However, this
check is already performed in nl80211_set_coalesce() resulting in
-EOPNOTSUPP. This patch removes check from rdev wrapper function.

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 net/wireless/rdev-ops.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 85ff30b..903b58a 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1063,11 +1063,10 @@ static inline int
 rdev_set_coalesce(struct cfg80211_registered_device *rdev,
 		  struct cfg80211_coalesce *coalesce)
 {
-	int ret = -ENOTSUPP;
+	int ret;
 
 	trace_rdev_set_coalesce(&rdev->wiphy, coalesce);
-	if (rdev->ops->set_coalesce)
-		ret = rdev->ops->set_coalesce(&rdev->wiphy, coalesce);
+	ret = rdev->ops->set_coalesce(&rdev->wiphy, coalesce);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 	return ret;
 }
-- 
1.9.1

[PATCH 2/4] cfg80211: rdev-ops: remove callback check from rdev_set_mcast_rate()

[Arend van Spriel]

The wrapper rdev_set_mcast_rate() checks whether the driver provides
the set_mcast_rate callback and returns -ENOTSUPP if not. However, this
check is already performed in nl80211_set_mcast_rate() resulting in
-EOPNOTSUPP. This patch removes check from rdev wrapper function.

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 net/wireless/rdev-ops.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 903b58a..d002415 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1050,11 +1050,10 @@ rdev_set_mcast_rate(struct cfg80211_registered_device *rdev,
 		    struct net_device *dev,
 		    int mcast_rate[NUM_NL80211_BANDS])
 {
-	int ret = -ENOTSUPP;
+	int ret;
 
 	trace_rdev_set_mcast_rate(&rdev->wiphy, dev, mcast_rate);
-	if (rdev->ops->set_mcast_rate)
-		ret = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, mcast_rate);
+	ret = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, mcast_rate);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 	return ret;
 }
-- 
1.9.1

[PATCH 3/4] cfg80211: rdev-ops: remove callback check from rdev_start_radar_detection()

[Arend van Spriel]

The wrapper rdev_start_radar_detection() checks whether the driver provides
the start_radar_detection callback and returns -ENOTSUPP if not. However,
this check is already performed in nl80211_start_radar_detection() resulting
in -EOPNOTSUPP. This patch removes check from rdev wrapper function and move
the callback checking in nl80211_start_radar_detection() before the other
checks.

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 net/wireless/nl80211.c  | 7 ++++---
 net/wireless/rdev-ops.h | 7 +++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f02653a..8ab63b5 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6826,6 +6826,9 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	unsigned int cac_time_ms;
 	int err;
 
+	if (!rdev->ops->start_radar_detection)
+		return -EOPNOTSUPP;
+
 	dfs_region = reg_get_dfs_region(wdev->wiphy);
 	if (dfs_region == NL80211_DFS_UNSET)
 		return -EINVAL;
@@ -6845,15 +6848,13 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	if (err < 0)
 		return err;
 
+	/* 0 value means no dfs is required so bail out */
 	if (err == 0)
 		return -EINVAL;
 
 	if (!cfg80211_chandef_dfs_usable(wdev->wiphy, &chandef))
 		return -EINVAL;
 
-	if (!rdev->ops->start_radar_detection)
-		return -EOPNOTSUPP;
-
 	cac_time_ms = cfg80211_chandef_dfs_cac_time(&rdev->wiphy, &chandef);
 	if (WARN_ON(!cac_time_ms))
 		cac_time_ms = IEEE80211_DFS_MIN_CAC_TIME_MS;
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index d002415..2020606 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1034,13 +1034,12 @@ rdev_start_radar_detection(struct cfg80211_registered_device *rdev,
 			   struct cfg80211_chan_def *chandef,
 			   u32 cac_time_ms)
 {
-	int ret = -ENOTSUPP;
+	int ret;
 
 	trace_rdev_start_radar_detection(&rdev->wiphy, dev, chandef,
 					 cac_time_ms);
-	if (rdev->ops->start_radar_detection)
-		ret = rdev->ops->start_radar_detection(&rdev->wiphy, dev,
-						       chandef, cac_time_ms);
+	ret = rdev->ops->start_radar_detection(&rdev->wiphy, dev,
+					       chandef, cac_time_ms);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 	return ret;
 }
-- 
1.9.1

[PATCH 4/4] cfg80211: rdev-ops: remove checks from rdev_{add,del}_tx_ts()

[Arend van Spriel]

The wrappers check if callback is specified. Just have the checks
in nl80211.c checking the NL80211_FEATURE_SUPPORTS_WMM_ADMISSION
feature flag. If the driver sets this flag the callbacks are being
checked in wiphy_register().

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 net/wireless/core.c     |  4 ++++
 net/wireless/nl80211.c  |  3 +++
 net/wireless/rdev-ops.h | 12 +++++-------
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7645e97..ef83db8 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -579,6 +579,10 @@ int wiphy_register(struct wiphy *wiphy)
 		     !rdev->ops->tdls_cancel_channel_switch)))
 		return -EINVAL;
 
+	if (WARN_ON((wiphy->features & NL80211_FEATURE_SUPPORTS_WMM_ADMISSION)
+		    && (!rdev->ops->add_tx_ts || !rdev->ops->del_tx_ts)))
+		return -EINVAL;
+
 	/*
 	 * if a wiphy has unsupported modes for regulatory channel enforcement,
 	 * opt-out of enforcement checking
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8ab63b5..e818cec 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10890,6 +10890,9 @@ static int nl80211_del_tx_ts(struct sk_buff *skb, struct genl_info *info)
 	u8 tsid;
 	int err;
 
+	if (!(rdev->wiphy.features & NL80211_FEATURE_SUPPORTS_WMM_ADMISSION))
+		return -EOPNOTSUPP;
+
 	if (!info->attrs[NL80211_ATTR_TSID] || !info->attrs[NL80211_ATTR_MAC])
 		return -EINVAL;
 
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 2020606..be89461 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -978,13 +978,12 @@ rdev_add_tx_ts(struct cfg80211_registered_device *rdev,
 	       struct net_device *dev, u8 tsid, const u8 *peer,
 	       u8 user_prio, u16 admitted_time)
 {
-	int ret = -EOPNOTSUPP;
+	int ret;
 
 	trace_rdev_add_tx_ts(&rdev->wiphy, dev, tsid, peer,
 			     user_prio, admitted_time);
-	if (rdev->ops->add_tx_ts)
-		ret = rdev->ops->add_tx_ts(&rdev->wiphy, dev, tsid, peer,
-					   user_prio, admitted_time);
+	ret = rdev->ops->add_tx_ts(&rdev->wiphy, dev, tsid, peer,
+				   user_prio, admitted_time);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 
 	return ret;
@@ -994,11 +993,10 @@ static inline int
 rdev_del_tx_ts(struct cfg80211_registered_device *rdev,
 	       struct net_device *dev, u8 tsid, const u8 *peer)
 {
-	int ret = -EOPNOTSUPP;
+	int ret;
 
 	trace_rdev_del_tx_ts(&rdev->wiphy, dev, tsid, peer);
-	if (rdev->ops->del_tx_ts)
-		ret = rdev->ops->del_tx_ts(&rdev->wiphy, dev, tsid, peer);
+	ret = rdev->ops->del_tx_ts(&rdev->wiphy, dev, tsid, peer);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 
 	return ret;
-- 
1.9.1

Re: [PATCH 1/4] cfg80211: rdev-ops: remove callback check from rdev_set_coalesce()

[Johannes Berg]

On Wed, 2016-08-10 at 12:33 +0200, Arend van Spriel wrote:
> The wrapper rdev_set_coalesce() checks whether the driver provides
> the set_coalesce callback and returns -ENOTSUPP if not. However, this
> check is already performed in nl80211_set_coalesce() resulting in
> -EOPNOTSUPP. This patch removes check from rdev wrapper function.

What's the point though? Presumably the compiler will optimise it out,
and it seems safer to have it this way? Same for all patches in this
series.

johannes

Re: [PATCH 1/4] cfg80211: rdev-ops: remove callback check from rdev_set_coalesce()

[Arend Van Spriel]

On 11-8-2016 14:48, Johannes Berg wrote:
> On Wed, 2016-08-10 at 12:33 +0200, Arend van Spriel wrote:
>> The wrapper rdev_set_coalesce() checks whether the driver provides
>> the set_coalesce callback and returns -ENOTSUPP if not. However, this
>> check is already performed in nl80211_set_coalesce() resulting in
>> -EOPNOTSUPP. This patch removes check from rdev wrapper function.
> 
> What's the point though? Presumably the compiler will optimise it out,
> and it seems safer to have it this way? Same for all patches in this
> series.

I was in doubt to raise the question first about getting this stuff
consistent, ie. keep rdev-ops as flat as possible, but decided just to
put it out there in patch format. My bad :-)

If you want the rdev-ops to be safe against (future) callers not
checking the callback, it seems you should add a check in all rdev-ops
where the callback is optional.

Regards,
Arend

Re: [PATCH 1/4] cfg80211: rdev-ops: remove callback check from rdev_set_coalesce()

[Johannes Berg]

> I was in doubt to raise the question first about getting this stuff
> consistent, ie. keep rdev-ops as flat as possible, but decided just
> to put it out there in patch format. My bad :-)

:-)

> If you want the rdev-ops to be safe against (future) callers not
> checking the callback, it seems you should add a check in all rdev-
> ops where the callback is optional.

Yeah, perhaps. I don't really mind hugely either way, but I think
removing them now would mostly be unwarranted churn.

johannes