Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Johannes Berg]

On Mon, 2015-01-05 at 22:03 +0530, Avinash Patil wrote:
> Checking for carrier state during start_radar_detection is needed
> only for devices which support offchannel CAC.
> This patch provides this additional check of extended feature offchannel
> CAC support while checking for carrier state.
> 
> Signed-off-by: Avinash Patil <patila@marvell.com>
> ---
>  include/uapi/linux/nl80211.h | 3 +++
>  net/wireless/nl80211.c       | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 735ab43..c318802 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -4205,10 +4205,13 @@ enum nl80211_feature_flags {
>  /**
>   * enum nl80211_ext_feature_index - bit index of extended features.
>   *
> + * @NL80211_EXT_FEATURE_OFFCHAN_CAC: This device/driver supports
> + *	offchannel Channel Availability Check(CAC).
>   * @NUM_NL80211_EXT_FEATURES: number of extended features.
>   * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
>   */
>  enum nl80211_ext_feature_index {
> +	NL80211_EXT_FEATURE_OFFCHAN_CAC,
>  
>  	/* add new features before the definition below */
>  	NUM_NL80211_EXT_FEATURES,
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 39753de..b2abb37 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -6138,7 +6138,9 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
>  	if (err)
>  		return err;
>  
> -	if (netif_carrier_ok(dev))
> +	if (wiphy_ext_feature_isset(&rdev->wiphy,
> +				    NL80211_EXT_FEATURE_OFFCHAN_CAC) &&
> +	    netif_carrier_ok(dev))
>  		return -EBUSY;

Wait - doesn't that have to be !feature_isset()?

johannes

RE: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Avinash Patil]

________________________________________
From: Johannes Berg [johannes@sipsolutions.net]
Sent: Monday, January 05, 2015 6:27 PM
To: Avinash Patil
Cc: linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo
Subject: Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

On Mon, 2015-01-05 at 22:03 +0530, Avinash Patil wrote:
> Checking for carrier state during start_radar_detection is needed
> only for devices which support offchannel CAC.
> This patch provides this additional check of extended feature offchannel
> CAC support while checking for carrier state.
>
> Signed-off-by: Avinash Patil <patila@marvell.com>
> ---
>  include/uapi/linux/nl80211.h | 3 +++
>  net/wireless/nl80211.c       | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 735ab43..c318802 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -4205,10 +4205,13 @@ enum nl80211_feature_flags {
>  /**
>   * enum nl80211_ext_feature_index - bit index of extended features.
>   *
> + * @NL80211_EXT_FEATURE_OFFCHAN_CAC: This device/driver supports
> + *   offchannel Channel Availability Check(CAC).
>   * @NUM_NL80211_EXT_FEATURES: number of extended features.
>   * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
>   */
>  enum nl80211_ext_feature_index {
> +     NL80211_EXT_FEATURE_OFFCHAN_CAC,
>
>       /* add new features before the definition below */
>       NUM_NL80211_EXT_FEATURES,
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 39753de..b2abb37 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -6138,7 +6138,9 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
>       if (err)
>               return err;
>
> -     if (netif_carrier_ok(dev))
> +     if (wiphy_ext_feature_isset(&rdev->wiphy,
> +                                 NL80211_EXT_FEATURE_OFFCHAN_CAC) &&
> +         netif_carrier_ok(dev))
>               return -EBUSY;

>Wait - doesn't that have to be !feature_isset()?

>johannes

If Offchannel CAC is supported (driver has set this bit in wiphy's extended features) & carrier is ON, return EBUSY as offchannel CAC may be ongoing, isnt it?
I am confused ..

-Avinash. 

Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Johannes Berg]

On Mon, 2015-01-05 at 05:28 -0800, Avinash Patil wrote:

> > -     if (netif_carrier_ok(dev))
> > +     if (wiphy_ext_feature_isset(&rdev->wiphy,
> > +                                 NL80211_EXT_FEATURE_OFFCHAN_CAC) &&
> > +         netif_carrier_ok(dev))
> >               return -EBUSY;
> 
> >Wait - doesn't that have to be !feature_isset()?
> 
> >johannes
> 
> If Offchannel CAC is supported (driver has set this bit in wiphy's
> extended features) & carrier is ON, return EBUSY as offchannel CAC may
> be ongoing, isnt it?

Well, my thinking is this - a new feature flag should allow something
new.

Therefore, the patch should essentially be this:

+ if (!new_feature)
  if (do_old_check)

Now wrapping that into a single if gives

- if (do_old_check)
+ if (!new_feature && do_old_check)

so the patch looks wrong to me.

johannes

RE: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Avinash Patil]

________________________________________
From: linux-wireless-owner@vger.kernel.org [linux-wireless-owner@vger.kernel.org] On Behalf Of Johannes Berg [johannes@sipsolutions.net]
Sent: Monday, January 05, 2015 7:17 PM
To: Avinash Patil
Cc: linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo
Subject: Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

On Mon, 2015-01-05 at 05:28 -0800, Avinash Patil wrote:

> > -     if (netif_carrier_ok(dev))
> > +     if (wiphy_ext_feature_isset(&rdev->wiphy,
> > +                                 NL80211_EXT_FEATURE_OFFCHAN_CAC) &&
> > +         netif_carrier_ok(dev))
> >               return -EBUSY;
>
> >Wait - doesn't that have to be !feature_isset()?
>
> >johannes
>
> If Offchannel CAC is supported (driver has set this bit in wiphy's
> extended features) & carrier is ON, return EBUSY as offchannel CAC may
> be ongoing, isnt it?

>Well, my thinking is this - a new feature flag should allow something
>new.

>Therefore, the patch should essentially be this:

>+ if (!new_feature)
>if (do_old_check)

>Now wrapping that into a single if gives

>- if (do_old_check)
>+ if (!new_feature && do_old_check)

>so the patch looks wrong to me.

I think here we want to  do_old_check only when new_feature is supported; because old check is causing issue for device where new feature is not supported.

Earlier it was:
- if (do_old_check)
Now it is:
+if (new_feature && do_old_check)

I think check is still correct. As you suggested we can nest it...

Please correct me if I am wrong.

-Avinash.

Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Johannes Berg]

On Mon, 2015-01-05 at 06:20 -0800, Avinash Patil wrote:

> I think here we want to  do_old_check only when new_feature is
> supported; because old check is causing issue for device where new
> feature is not supported.

But then you should be testing !new_feature to do the old_check

johannes

[PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Avinash Patil]

Checking for carrier state during start_radar_detection is needed
only for devices which support offchannel CAC.
This patch provides this additional check of extended feature offchannel
CAC support while checking for carrier state.

Signed-off-by: Avinash Patil <patila@marvell.com>
---
 include/uapi/linux/nl80211.h | 3 +++
 net/wireless/nl80211.c       | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 735ab43..c318802 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4205,10 +4205,13 @@ enum nl80211_feature_flags {
 /**
  * enum nl80211_ext_feature_index - bit index of extended features.
  *
+ * @NL80211_EXT_FEATURE_OFFCHAN_CAC: This device/driver supports
+ *	offchannel Channel Availability Check(CAC).
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
 enum nl80211_ext_feature_index {
+	NL80211_EXT_FEATURE_OFFCHAN_CAC,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 39753de..b2abb37 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6138,7 +6138,9 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if (netif_carrier_ok(dev))
+	if (wiphy_ext_feature_isset(&rdev->wiphy,
+				    NL80211_EXT_FEATURE_OFFCHAN_CAC) &&
+	    netif_carrier_ok(dev))
 		return -EBUSY;
 
 	if (wdev->cac_started)
-- 
1.8.1.4

Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Johannes Berg]

Based on our discussion, you don't actually want to support something
like off-channel CAC (which doesn't make much sense anyway), you just
have some issues with the carrier handling in the driver and bring up
the carrier ON by default rather than OFF.

I'll drop this completely for now.

johannes

RE: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

[Avinash Patil]

Sure Johannes.

We need to fix carrier state handling in ndo_open and this patch would not be needed.

Thanks,
Avinash
________________________________________
From: Johannes Berg [johannes@sipsolutions.net]
Sent: Wednesday, January 07, 2015 6:33 PM
To: Avinash Patil
Cc: linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo
Subject: Re: [PATCH v3] cfg80211: check for carrier state only when offchanel CAC supported

Based on our discussion, you don't actually want to support something
like off-channel CAC (which doesn't make much sense anyway), you just
have some issues with the carrier handling in the driver and bring up
the carrier ON by default rather than OFF.

I'll drop this completely for now.

johannes