[PATCH]: Mac80211 driver and I checked the patch

[PATCH]: Mac80211 driver and I checked the patch

[Christopher Sacchi]

The source file needed a change  that was told in the description to
know that the #include needed to be fixed, and the function changed
another value to 0 (yes) in the description (that said FIXME.) The
patch fixes a typo-like error and has been checked with checkpatch.pl
in the scripts directory.
Here's the patch:
--
--- main.c	2012-07-21 20:58:29.000000000 +0000
+++ mainnew.c	2012-08-05 20:00:37.000000000 +0000
@@ -32,7 +32,7 @@
 #include "led.h"
 #include "cfg.h"
 #include "debugfs.h"
-
+#include "net/rfkill/rfkill.h"
 static struct lock_class_key ieee80211_rx_skb_queue_class;

 void ieee80211_configure_filter(struct ieee80211_local *local)
@@ -183,6 +183,7 @@ int ieee80211_hw_config(struct ieee80211
 		 *
 		 */
 		/* WARN_ON(ret); */
+		WARN_ON(1)
 	}

 	return ret;
Signed-off-by: Christopher P. Sacchi <chris.sacchi@gmail.com>
--

-- 
Christopher

Re: [PATCH]: Mac80211 driver and I checked the patch

[Dan Williams]

On Mon, 2012-08-06 at 11:49 +0000, Christopher Sacchi wrote:
> The source file needed a change  that was told in the description to
> know that the #include needed to be fixed, and the function changed
> another value to 0 (yes) in the description (that said FIXME.) The
> patch fixes a typo-like error and has been checked with checkpatch.pl
> in the scripts directory.
> Here's the patch:

It's great that you're sending patches, but it also helps the developers
when the patches are correctly formatted.  See:

http://linux.yyz.us/patch-format.html

Here's a good example:

http://lkml.org/lkml/2012/8/2/286

1) the subject line starts with [PATCH] *and* the module being patches,
then quickly describes the patch
2) There's a Signed-off-by comes *before* the patch data
3) The full path to the files being patched is in the patch header (ie,
not just "main.c" but foo/bar/baz/main.c)

Cheers,
Dan

> --- main.c	2012-07-21 20:58:29.000000000 +0000
> +++ mainnew.c	2012-08-05 20:00:37.000000000 +0000
> @@ -32,7 +32,7 @@
>  #include "led.h"
>  #include "cfg.h"
>  #include "debugfs.h"
> -
> +#include "net/rfkill/rfkill.h"
>  static struct lock_class_key ieee80211_rx_skb_queue_class;
> 
>  void ieee80211_configure_filter(struct ieee80211_local *local)
> @@ -183,6 +183,7 @@ int ieee80211_hw_config(struct ieee80211
>  		 *
>  		 */
>  		/* WARN_ON(ret); */
> +		WARN_ON(1)
>  	}
> 
>  	return ret;
> Signed-off-by: Christopher P. Sacchi <chris.sacchi@gmail.com>
> --
> 

Re: [PATCH]: Mac80211 driver and I checked the patch

[Arend van Spriel]

Very generic subject line. I would expect something like:

[PATCH] mac80211: include rfkill header implicitly

On 08/06/2012 01:49 PM, Christopher Sacchi wrote:
> The source file needed a change  that was told in the description to
> know that the #include needed to be fixed, and the function changed
> another value to 0 (yes) in the description (that said FIXME.) The
> patch fixes a typo-like error and has been checked with checkpatch.pl
> in the scripts directory.


> Here's the patch:

Do need a heads-up stating the obvious.
> --
> --- main.c	2012-07-21 20:58:29.000000000 +0000
> +++ mainnew.c	2012-08-05 20:00:37.000000000 +0000

How did you create this patch? diff -U? When I try to apply your patch:

$ git am patches/mac80211-crappy-patch-test.eml
Applying: Mac80211 driver and I checked the patch
error: mainnew.c: does not exist in index
Patch failed at 0001 Mac80211 driver and I checked the patch
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

You really should do your changes in a git repository.

I changed it to:

--- a/net/mac80211/main.c	2012-07-21 20:58:29.000000000 +0000
+++ b/net/mac80211/main.c	2012-08-05 20:00:37.000000000 +0000

and applied it. That applied, but it does not build.

> @@ -32,7 +32,7 @@
>  #include "led.h"
>  #include "cfg.h"
>  #include "debugfs.h"
> -
> +#include "net/rfkill/rfkill.h"

add empty line between #include and function prototype.

>  static struct lock_class_key ieee80211_rx_skb_queue_class;
> 
>  void ieee80211_configure_filter(struct ieee80211_local *local)
> @@ -183,6 +183,7 @@ int ieee80211_hw_config(struct ieee80211
>  		 *
>  		 */
>  		/* WARN_ON(ret); */
> +		WARN_ON(1)

Still no ';' here.

>  	}
> 
>  	return ret;
> Signed-off-by: Christopher P. Sacchi <chris.sacchi@gmail.com>

Now this line will be added in the code. You really do not want that! (I
hope).

> --
> 

More general: What are you trying to change? I did take a look in
net/mac80211/main.c and found the FIXME you refer to:

int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
{
	:
	:
        if (changed && local->open_count) {
                ret = drv_config(local, changed);
                /*
                 * Goal:
                 * HW reconfiguration should never fail, the driver has told
                 * us what it can support so it should live up to that
promise.
                 *
                 * Current status:
                 * rfkill is not integrated with mac80211 and a
                 * configuration command can thus fail if hardware rfkill
                 * is enabled
                 *
                 * FIXME: integrate rfkill with mac80211 and then add this
                 * WARN_ON() back
                 *
                 */
                /* WARN_ON(ret); */
        }

        return ret;
}

Back to your change:
1) You are a WARN_ON(1). So on every drv_config() call you get a kernel
warning. Unlikely we want that.
2) For the warning to be put back, the comment says "integrate rfkill
with mac80211". Adding an include of the rfkill header file does not
really cover that statement.

All in all you seem to forget a number of steps in
Documentation/SubmittingPatches. Apart from passing checkpatch.pl it
should first of all at least compile :-(

Gr. AvS

Re: [PATCH]: Mac80211 driver and I checked the patch

[Arend van Spriel]

On 08/06/2012 07:59 PM, Arend van Spriel wrote:
> On 08/06/2012 01:49 PM, Christopher Sacchi wrote:
>> > The source file needed a change  that was told in the description to
>> > know that the #include needed to be fixed, and the function changed
>> > another value to 0 (yes) in the description (that said FIXME.) The
>> > patch fixes a typo-like error and has been checked with checkpatch.pl
>> > in the scripts directory.
> 
>> > Here's the patch:
> Do need a heads-up stating the obvious.

I meant "Do not need" :-p

Gr. AvS