[PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@frijolero.org>

It has happened twice now where elaborate troubleshooting has
undergone on systems where CONFIG_CFG80211_INTERNAL_REGDB [0]
has been set but yet net/wireless/db.txt was not updated.

Despite the documentation on this it seems system integrators could
use some more help with this, so throw out a kernel warning at boot time
when their database is empty, and if they enabled cfg80211 regulatory
debugging (CONFIG_CFG80211_REG_DEBUG), simply fail at build time. If
they didn't enable cfg80211 regulatory debugging then we'll just warn
at boot time. This does mean that the error-prone system integrator
won't likely realize the issue until they boot the machine but -- it
does not seem to make sense to enable a build bug unless someone
really is debugging.

[0] http://wireless.kernel.org/en/developers/Regulatory/CRDA#CONFIG_CFG80211_INTERNAL_REGDB

Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Youngsin Lee <youngsin@qualcomm.com>
Cc: Raja Mani <rmani@qca.qualcomm.com>
Cc: Senthil Kumar Balasubramanian <senthilb@qca.qualcomm.com>
Cc: Vipin Mehta <vipimeht@qca.qualcomm.com>
Cc: yahuan@qca.qualcomm.com
Cc: jjan@qca.qualcomm.com
Cc: vthiagar@qca.qualcomm.com
Cc: henrykim@qualcomm.com
Cc: jouni@qca.qualcomm.com
Cc: athiruve@qca.qualcomm.com
Cc: cjkim@qualcomm.com
Cc: philipk@qca.qualcomm.com
Cc: sunnykim@qualcomm.com
Cc: sskwak@qualcomm.com
Cc: kkim@qualcomm.com
Cc: mattbyun@qualcomm.com
Cc: ryanlee@qualcomm.com
Cc: simbap@qualcomm.com
Cc: krislee@qualcomm.com
Cc: conner@qualcomm.com
Cc: hojinkim@qualcomm.com
Cc: honglee@qualcomm.com
Cc: johnwkim@qualcomm.com
Signed-off-by: Luis R. Rodriguez <mcgrof@frijolero.org>
---

Stephen, John, not sure what is best to address this since I know we shouldn't
be failing to build kernels easily. This will only fail to build if both
CONFIG_CFG80211_REG_DEBUG and CONFIG_CFG80211_INTERNAL_REGDB are enabled.
We could get rid of the build bug check completely but this just means
system integrators will likley fail to fix the issue early. Experience
shows this is a common issue so far and a lot of time is going into debugging
this. I'd like to avoid this in the future. Let me know what you guys think.

 net/wireless/reg.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index e9a0ac8..85f51b3 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
 
 	schedule_work(&reg_regdb_work);
 }
+
+/* Feel free to add any other sanity checks here */
+static void reg_regdb_size_check(void)
+{
+#ifdef CONFIG_CFG80211_REG_DEBUG
+	BUILD_BUG_ON(!reg_regdb_size);
+#else
+	WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
+#endif
+}
 #else
+static inline void reg_regdb_size_check(void) {}
 static inline void reg_regdb_query(const char *alpha2) {}
 #endif /* CONFIG_CFG80211_INTERNAL_REGDB */
 
@@ -2367,6 +2378,7 @@ void /* __init_or_exit */ regulatory_exit(void)
 	mutex_lock(&cfg80211_mutex);
 	mutex_lock(&reg_mutex);
 
+	reg_regdb_size_check();
 	reset_regdomains(true);
 
 	dev_set_uevent_suppress(&reg_pdev->dev, true);
-- 
1.7.10.rc1.22.gf5241

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Johannes Berg]

On Mon, 2012-03-19 at 14:35 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@frijolero.org>
> 
> It has happened twice now where elaborate troubleshooting has
> undergone on systems where CONFIG_CFG80211_INTERNAL_REGDB [0]
> has been set but yet net/wireless/db.txt was not updated.
> 
> Despite the documentation on this it seems system integrators could
> use some more help with this, so throw out a kernel warning at boot time
> when their database is empty, and if they enabled cfg80211 regulatory
> debugging (CONFIG_CFG80211_REG_DEBUG), simply fail at build time. If
> they didn't enable cfg80211 regulatory debugging then we'll just warn
> at boot time. This does mean that the error-prone system integrator
> won't likely realize the issue until they boot the machine but -- it
> does not seem to make sense to enable a build bug unless someone
> really is debugging.
> 
> [0] http://wireless.kernel.org/en/developers/Regulatory/CRDA#CONFIG_CFG80211_INTERNAL_REGDB
> 
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Youngsin Lee <youngsin@qualcomm.com>
> Cc: Raja Mani <rmani@qca.qualcomm.com>
> Cc: Senthil Kumar Balasubramanian <senthilb@qca.qualcomm.com>
> Cc: Vipin Mehta <vipimeht@qca.qualcomm.com>
> Cc: yahuan@qca.qualcomm.com
> Cc: jjan@qca.qualcomm.com
> Cc: vthiagar@qca.qualcomm.com
> Cc: henrykim@qualcomm.com
> Cc: jouni@qca.qualcomm.com
> Cc: athiruve@qca.qualcomm.com
> Cc: cjkim@qualcomm.com
> Cc: philipk@qca.qualcomm.com
> Cc: sunnykim@qualcomm.com
> Cc: sskwak@qualcomm.com
> Cc: kkim@qualcomm.com
> Cc: mattbyun@qualcomm.com
> Cc: ryanlee@qualcomm.com
> Cc: simbap@qualcomm.com
> Cc: krislee@qualcomm.com
> Cc: conner@qualcomm.com
> Cc: hojinkim@qualcomm.com
> Cc: honglee@qualcomm.com
> Cc: johnwkim@qualcomm.com
> Signed-off-by: Luis R. Rodriguez <mcgrof@frijolero.org>
> ---
> 
> Stephen, John, not sure what is best to address this since I know we shouldn't
> be failing to build kernels easily. This will only fail to build if both
> CONFIG_CFG80211_REG_DEBUG and CONFIG_CFG80211_INTERNAL_REGDB are enabled.
> We could get rid of the build bug check completely but this just means
> system integrators will likley fail to fix the issue early. Experience
> shows this is a common issue so far and a lot of time is going into debugging
> this. I'd like to avoid this in the future. Let me know what you guys think.
> 
>  net/wireless/reg.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index e9a0ac8..85f51b3 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
>  
>  	schedule_work(&reg_regdb_work);
>  }
> +
> +/* Feel free to add any other sanity checks here */
> +static void reg_regdb_size_check(void)
> +{
> +#ifdef CONFIG_CFG80211_REG_DEBUG
> +	BUILD_BUG_ON(!reg_regdb_size);
> +#else
> +	WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
> +#endif

That ifdef seems a bit pointless? If anything I would have expected it
the other way around since the BUILD_BUG_ON compiles to nothing?

johannes

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Luis R. Rodriguez]

On Mon, Mar 19, 2012 at 11:02 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2012-03-19 at 14:35 -0700, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <mcgrof@frijolero.org>
>>
>> It has happened twice now where elaborate troubleshooting has
>> undergone on systems where CONFIG_CFG80211_INTERNAL_REGDB [0]
>> has been set but yet net/wireless/db.txt was not updated.
>>
>> Despite the documentation on this it seems system integrators could
>> use some more help with this, so throw out a kernel warning at boot time
>> when their database is empty, and if they enabled cfg80211 regulatory
>> debugging (CONFIG_CFG80211_REG_DEBUG), simply fail at build time. If
>> they didn't enable cfg80211 regulatory debugging then we'll just warn
>> at boot time. This does mean that the error-prone system integrator
>> won't likely realize the issue until they boot the machine but -- it
>> does not seem to make sense to enable a build bug unless someone
>> really is debugging.
>>
>> [0] http://wireless.kernel.org/en/developers/Regulatory/CRDA#CONFIG_CFG80211_INTERNAL_REGDB
>>
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Cc: Youngsin Lee <youngsin@qualcomm.com>
>> Cc: Raja Mani <rmani@qca.qualcomm.com>
>> Cc: Senthil Kumar Balasubramanian <senthilb@qca.qualcomm.com>
>> Cc: Vipin Mehta <vipimeht@qca.qualcomm.com>
>> Cc: yahuan@qca.qualcomm.com
>> Cc: jjan@qca.qualcomm.com
>> Cc: vthiagar@qca.qualcomm.com
>> Cc: henrykim@qualcomm.com
>> Cc: jouni@qca.qualcomm.com
>> Cc: athiruve@qca.qualcomm.com
>> Cc: cjkim@qualcomm.com
>> Cc: philipk@qca.qualcomm.com
>> Cc: sunnykim@qualcomm.com
>> Cc: sskwak@qualcomm.com
>> Cc: kkim@qualcomm.com
>> Cc: mattbyun@qualcomm.com
>> Cc: ryanlee@qualcomm.com
>> Cc: simbap@qualcomm.com
>> Cc: krislee@qualcomm.com
>> Cc: conner@qualcomm.com
>> Cc: hojinkim@qualcomm.com
>> Cc: honglee@qualcomm.com
>> Cc: johnwkim@qualcomm.com
>> Signed-off-by: Luis R. Rodriguez <mcgrof@frijolero.org>
>> ---
>>
>> Stephen, John, not sure what is best to address this since I know we shouldn't
>> be failing to build kernels easily. This will only fail to build if both
>> CONFIG_CFG80211_REG_DEBUG and CONFIG_CFG80211_INTERNAL_REGDB are enabled.
>> We could get rid of the build bug check completely but this just means
>> system integrators will likley fail to fix the issue early. Experience
>> shows this is a common issue so far and a lot of time is going into debugging
>> this. I'd like to avoid this in the future. Let me know what you guys think.
>>
>>  net/wireless/reg.c |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index e9a0ac8..85f51b3 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
>>
>>       schedule_work(&reg_regdb_work);
>>  }
>> +
>> +/* Feel free to add any other sanity checks here */
>> +static void reg_regdb_size_check(void)
>> +{
>> +#ifdef CONFIG_CFG80211_REG_DEBUG
>> +     BUILD_BUG_ON(!reg_regdb_size);
>> +#else
>> +     WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
>> +#endif
>
> That ifdef seems a bit pointless? If anything I would have expected it
> the other way around since the BUILD_BUG_ON compiles to nothing?

As I tested it, the BUILD_BUG_ON() forces a compile failure.

  Luis

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Johannes Berg]

On Tue, 2012-03-20 at 00:00 -0700, Luis R. Rodriguez wrote:

> >>  net/wireless/reg.c |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> >> index e9a0ac8..85f51b3 100644
> >> --- a/net/wireless/reg.c
> >> +++ b/net/wireless/reg.c
> >> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
> >>
> >>       schedule_work(&reg_regdb_work);
> >>  }
> >> +
> >> +/* Feel free to add any other sanity checks here */
> >> +static void reg_regdb_size_check(void)
> >> +{
> >> +#ifdef CONFIG_CFG80211_REG_DEBUG
> >> +     BUILD_BUG_ON(!reg_regdb_size);
> >> +#else
> >> +     WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
> >> +#endif
> >
> > That ifdef seems a bit pointless? If anything I would have expected it
> > the other way around since the BUILD_BUG_ON compiles to nothing?
> 
> As I tested it, the BUILD_BUG_ON() forces a compile failure.

Right. Why would you not want that always?

johannes

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Luis R. Rodriguez]

On Tue, Mar 20, 2012 at 12:08 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2012-03-20 at 00:00 -0700, Luis R. Rodriguez wrote:
>
>> >>  net/wireless/reg.c |   12 ++++++++++++
>> >>  1 file changed, 12 insertions(+)
>> >>
>> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> >> index e9a0ac8..85f51b3 100644
>> >> --- a/net/wireless/reg.c
>> >> +++ b/net/wireless/reg.c
>> >> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
>> >>
>> >>       schedule_work(&reg_regdb_work);
>> >>  }
>> >> +
>> >> +/* Feel free to add any other sanity checks here */
>> >> +static void reg_regdb_size_check(void)
>> >> +{
>> >> +#ifdef CONFIG_CFG80211_REG_DEBUG
>> >> +     BUILD_BUG_ON(!reg_regdb_size);
>> >> +#else
>> >> +     WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
>> >> +#endif
>> >
>> > That ifdef seems a bit pointless? If anything I would have expected it
>> > the other way around since the BUILD_BUG_ON compiles to nothing?
>>
>> As I tested it, the BUILD_BUG_ON() forces a compile failure.
>
> Right. Why would you not want that always?

Ah well that is a question for you, John and Stephen. I didn't use
that *always* given that it would break random build testing whenever
CFG80211_INTERNAL_REGDB was enabled, given that it requires a manual
cp of db.txt from wireless-regdb. With this it would only break build
testing with debugging cfg80211 regulatory, both
CFG80211_INTERNAL_REGDB and CONFIG_CFG80211_REG_DEBUG enabled. If you
guys are happy with it then so am I -- I prefer it, just didn't want
any surprises or anyone reporting an unexpected build breakage later.

  Luis

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[John W. Linville]

On Tue, Mar 20, 2012 at 12:13:34AM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 20, 2012 at 12:08 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Tue, 2012-03-20 at 00:00 -0700, Luis R. Rodriguez wrote:
> >
> >> >>  net/wireless/reg.c |   12 ++++++++++++
> >> >>  1 file changed, 12 insertions(+)
> >> >>
> >> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> >> >> index e9a0ac8..85f51b3 100644
> >> >> --- a/net/wireless/reg.c
> >> >> +++ b/net/wireless/reg.c
> >> >> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
> >> >>
> >> >>       schedule_work(&reg_regdb_work);
> >> >>  }
> >> >> +
> >> >> +/* Feel free to add any other sanity checks here */
> >> >> +static void reg_regdb_size_check(void)
> >> >> +{
> >> >> +#ifdef CONFIG_CFG80211_REG_DEBUG
> >> >> +     BUILD_BUG_ON(!reg_regdb_size);
> >> >> +#else
> >> >> +     WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
> >> >> +#endif
> >> >
> >> > That ifdef seems a bit pointless? If anything I would have expected it
> >> > the other way around since the BUILD_BUG_ON compiles to nothing?
> >>
> >> As I tested it, the BUILD_BUG_ON() forces a compile failure.
> >
> > Right. Why would you not want that always?
> 
> Ah well that is a question for you, John and Stephen. I didn't use
> that *always* given that it would break random build testing whenever
> CFG80211_INTERNAL_REGDB was enabled, given that it requires a manual
> cp of db.txt from wireless-regdb. With this it would only break build
> testing with debugging cfg80211 regulatory, both
> CFG80211_INTERNAL_REGDB and CONFIG_CFG80211_REG_DEBUG enabled. If you
> guys are happy with it then so am I -- I prefer it, just didn't want
> any surprises or anyone reporting an unexpected build breakage later.

My first inclination is like Johannes, just break the build in this
case.  But the random build test breakage could be an annoyance for
the folks doing that.  I don't suppose there is any Kconfig magic
that would prevent selecting CFG80211_INTERNAL_REGDB unless there is
a non-zero db.txt file?

Also, I guess that an empty db.txt just forces the user back to
the build it "world" domain.  While that is less than ideal, it is
sufficient for some minimal functionality.  So breaking the build in
that case seems like bad form too.

Maybe a runtime warning is sufficient?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] cfg80211: warn if db.txt is empty with CONFIG_CFG80211_INTERNAL_REGDB

[Luis R. Rodriguez]

On Tue, Mar 20, 2012 at 6:46 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Tue, Mar 20, 2012 at 12:13:34AM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 20, 2012 at 12:08 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Tue, 2012-03-20 at 00:00 -0700, Luis R. Rodriguez wrote:
>> >
>> >> >>  net/wireless/reg.c |   12 ++++++++++++
>> >> >>  1 file changed, 12 insertions(+)
>> >> >>
>> >> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> >> >> index e9a0ac8..85f51b3 100644
>> >> >> --- a/net/wireless/reg.c
>> >> >> +++ b/net/wireless/reg.c
>> >> >> @@ -388,7 +388,18 @@ static void reg_regdb_query(const char *alpha2)
>> >> >>
>> >> >>       schedule_work(&reg_regdb_work);
>> >> >>  }
>> >> >> +
>> >> >> +/* Feel free to add any other sanity checks here */
>> >> >> +static void reg_regdb_size_check(void)
>> >> >> +{
>> >> >> +#ifdef CONFIG_CFG80211_REG_DEBUG
>> >> >> +     BUILD_BUG_ON(!reg_regdb_size);
>> >> >> +#else
>> >> >> +     WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
>> >> >> +#endif
>> >> >
>> >> > That ifdef seems a bit pointless? If anything I would have expected it
>> >> > the other way around since the BUILD_BUG_ON compiles to nothing?
>> >>
>> >> As I tested it, the BUILD_BUG_ON() forces a compile failure.
>> >
>> > Right. Why would you not want that always?
>>
>> Ah well that is a question for you, John and Stephen. I didn't use
>> that *always* given that it would break random build testing whenever
>> CFG80211_INTERNAL_REGDB was enabled, given that it requires a manual
>> cp of db.txt from wireless-regdb. With this it would only break build
>> testing with debugging cfg80211 regulatory, both
>> CFG80211_INTERNAL_REGDB and CONFIG_CFG80211_REG_DEBUG enabled. If you
>> guys are happy with it then so am I -- I prefer it, just didn't want
>> any surprises or anyone reporting an unexpected build breakage later.
>
> My first inclination is like Johannes, just break the build in this
> case.  But the random build test breakage could be an annoyance for
> the folks doing that.  I don't suppose there is any Kconfig magic
> that would prevent selecting CFG80211_INTERNAL_REGDB unless there is
> a non-zero db.txt file?

That'd be really cool.

> Also, I guess that an empty db.txt just forces the user back to
> the build it "world" domain.  While that is less than ideal, it is
> sufficient for some minimal functionality.  So breaking the build in
> that case seems like bad form too.
>
> Maybe a runtime warning is sufficient?

That's what I have when CONFIG_CFG80211_REG_DEBUG is disabled in the
patch. My only concern with this is that so far based on experience a
lot of debugging may have undergone by then. But I'm also OK with this
as well to avoid build breakage suffering. Will send a v2 that just
does this.

  Luis