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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
> 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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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