| Message ID | 1422094745-12718-1-git-send-email-hong@topbug.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
Hong Xu <hong@topbug.net> writes: > ath9k and ath9k_htc use the variable name "led_blink" to indicate > whether the module parameter "blink" is on. This name is easy to > conflict with other variables, thus they are renamed. Please state clearly that this fixes a compiler error found by kbuild. Also it's very much recommended to add the actual compiler error to the commit log: drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here
On Sat, Jan 24, 2015 at 11:23 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > Hong Xu <hong@topbug.net> writes: > >> ath9k and ath9k_htc use the variable name "led_blink" to indicate >> whether the module parameter "blink" is on. This name is easy to >> conflict with other variables, thus they are renamed. > > Please state clearly that this fixes a compiler error found by kbuild. > Also it's very much recommended to add the actual compiler error to the > commit log: > > drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' > drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here > Some more things... If you post a vN (referring to your v3) of a patch, please add a history of changes. 2nd, use the Fixes tag [2] in this case with a reference to the culprit commit [1]. Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") Last but not least, honour credits like Reported-by/Tested-by (plus some infos like broken in Linux-next $VERSION). - Sedat - [1] http://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=3a939a671225909c19b09bfcb6e4761109e913d9 [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n159 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 24.01.2015 um 11:33 schrieb Sedat Dilek: > On Sat, Jan 24, 2015 at 11:23 AM, Kalle Valo <kvalo@codeaurora.org> wrote: >> Hong Xu <hong@topbug.net> writes: >> >>> ath9k and ath9k_htc use the variable name "led_blink" to indicate >>> whether the module parameter "blink" is on. This name is easy to >>> conflict with other variables, thus they are renamed. >> >> Please state clearly that this fixes a compiler error found by kbuild. >> Also it's very much recommended to add the actual compiler error to the >> commit log: >> >> drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' >> drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here >> > > Some more things... > > If you post a vN (referring to your v3) of a patch, please add a > history of changes. What kind of history do you wont to have here? Kernel log should not be overloaded with patch preparation history. > 2nd, use the Fixes tag [2] in this case with a reference to the > culprit commit [1]. > > Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") > > Last but not least, honour credits like Reported-by/Tested-by (plus > some infos like broken in Linux-next $VERSION). > > - Sedat - > > [1] http://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=3a939a671225909c19b09bfcb6e4761109e913d9 > [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n159 > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
(I'll drop ath9k-devel as someone from there seems to be spamming with vacation responses) Sedat Dilek <sedat.dilek@gmail.com> writes: > 2nd, use the Fixes tag [2] in this case with a reference to the > culprit commit [1]. > > Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") Good point. I just found a handy tip from SubmittingPatches, just add this to ~/.gitconfig: [core] abbrev = 12 [pretty] fixes = Fixes: %h (\"%s\") And then you get the fixes line automatically formatted for whatever commit you want: $ git log --format=fixes | head -5 Fixes: fa1270678ce4 ("ath10k: Enable the MCS8 and MCS9 at 2.4G band") Fixes: 296f98ed02dd ("ath10k: fix pmf for wmi-tlv on qca6174") Fixes: 3443aaf6f11d ("ath10k: remove sw encryption for pmf") Fixes: d541c931dc93 ("ath10k: disable irqs after fw crash") Fixes: 9f282daeedc9 ("ath10k: disable uapsd autotrigger") Yet another reason why git is so awesome.
On Sat, Jan 24, 2015 at 11:38 AM, Oleksij Rempel <linux@rempel-privat.de> wrote: > Am 24.01.2015 um 11:33 schrieb Sedat Dilek: >> On Sat, Jan 24, 2015 at 11:23 AM, Kalle Valo <kvalo@codeaurora.org> wrote: >>> Hong Xu <hong@topbug.net> writes: >>> >>>> ath9k and ath9k_htc use the variable name "led_blink" to indicate >>>> whether the module parameter "blink" is on. This name is easy to >>>> conflict with other variables, thus they are renamed. >>> >>> Please state clearly that this fixes a compiler error found by kbuild. >>> Also it's very much recommended to add the actual compiler error to the >>> commit log: >>> >>> drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' >>> drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here >>> >> >> Some more things... >> >> If you post a vN (referring to your v3) of a patch, please add a >> history of changes. > > What kind of history do you wont to have here? Kernel log should not be > overloaded with patch preparation history. > Not here. Referring to "[PATCH v3] ath9k_htc: Add a module parameter to disable blink". And no, the history of changes are 1. important 2. not in the "commit-body", so you will not see them when the patch is pushed to the appropriate Git tree. - Sedat - -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 24, 2015 at 11:39 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > (I'll drop ath9k-devel as someone from there seems to be spamming with > vacation responses) > > Sedat Dilek <sedat.dilek@gmail.com> writes: > >> 2nd, use the Fixes tag [2] in this case with a reference to the >> culprit commit [1]. >> >> Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") > > Good point. I just found a handy tip from SubmittingPatches, just add > this to ~/.gitconfig: > > [core] > abbrev = 12 > [pretty] > fixes = Fixes: %h (\"%s\") > > And then you get the fixes line automatically formatted for whatever > commit you want: > > $ git log --format=fixes | head -5 > Fixes: fa1270678ce4 ("ath10k: Enable the MCS8 and MCS9 at 2.4G band") > Fixes: 296f98ed02dd ("ath10k: fix pmf for wmi-tlv on qca6174") > Fixes: 3443aaf6f11d ("ath10k: remove sw encryption for pmf") > Fixes: d541c931dc93 ("ath10k: disable irqs after fw crash") > Fixes: 9f282daeedc9 ("ath10k: disable uapsd autotrigger") > > Yet another reason why git is so awesome. > /o\. Thanks, I added a core and pretty tag to my ~/.gitconfig, espacially the 12-digit thingie was a long standing todo :-). Working with tig I have to check the tipps of Peter Hutterer in his blog for my local ~/.tigrc again. This got lost when changing to Ubuntu/precise. - Sedat - -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > Working with tig I have to check the tipps of Peter Hutterer in his > blog for my local ~/.tigrc again. > This got lost when changing to Ubuntu/precise. > For tigrc... [2] has [1] included. [3] and [4] as a reference to tig ("...is an ncurses-based text-mode interface for git.). - Sedat - [1] http://who-t.blogspot.de/2009/04/git-format-patch-for-single-commit.html [2] http://who-t.blogspot.de/2014/03/using-git-next-level.html [3] http://jonas.nitro.dk/tig/ [4] http://jonas.nitro.dk/tig/manual.html -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sedat Dilek <sedat.dilek@gmail.com> writes: > On Sat, Jan 24, 2015 at 11:23 AM, Kalle Valo <kvalo@codeaurora.org> wrote: >> Hong Xu <hong@topbug.net> writes: >> >>> ath9k and ath9k_htc use the variable name "led_blink" to indicate >>> whether the module parameter "blink" is on. This name is easy to >>> conflict with other variables, thus they are renamed. >> >> Please state clearly that this fixes a compiler error found by kbuild. >> Also it's very much recommended to add the actual compiler error to the >> commit log: >> >> drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' >> drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here >> > > Some more things... > > If you post a vN (referring to your v3) of a patch, please add a > history of changes. > > 2nd, use the Fixes tag [2] in this case with a reference to the > culprit commit [1]. > > Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") > > Last but not least, honour credits like Reported-by/Tested-by (plus > some infos like broken in Linux-next $VERSION). > Thanks for your comments, I should have been more careful with the guideline. I guess we shouldn't credit kbuild for reporting in this case? Hong
On Sat, Jan 24, 2015 at 12:10 PM, Hong Xu <hong@topbug.net> wrote: > > Sedat Dilek <sedat.dilek@gmail.com> writes: > >> On Sat, Jan 24, 2015 at 11:23 AM, Kalle Valo <kvalo@codeaurora.org> wrote: >>> Hong Xu <hong@topbug.net> writes: >>> >>>> ath9k and ath9k_htc use the variable name "led_blink" to indicate >>>> whether the module parameter "blink" is on. This name is easy to >>>> conflict with other variables, thus they are renamed. >>> >>> Please state clearly that this fixes a compiler error found by kbuild. >>> Also it's very much recommended to add the actual compiler error to the >>> commit log: >>> >>> drivers/net/wireless/ath/ath9k/ath9k_htc.o:(.data+0x47c): multiple definition of `led_blink' >>> drivers/net/wireless/ath/ath9k/ath9k.o:(.bss+0x20): first defined here >>> >> >> Some more things... >> >> If you post a vN (referring to your v3) of a patch, please add a >> history of changes. >> >> 2nd, use the Fixes tag [2] in this case with a reference to the >> culprit commit [1]. >> >> Fixes: 3a939a671225 ("ath9k_htc: Add a module parameter to disable blink") >> >> Last but not least, honour credits like Reported-by/Tested-by (plus >> some infos like broken in Linux-next $VERSION). >> > Thanks for your comments, I should have been more careful with the > guideline. I guess we shouldn't credit kbuild for reporting in this case? > Learning by doing :-). Yesterday, I looked trough some Git trees offline and I saw a commit with kbuild-test-bot as "author", so... :-). Not sure, if this is good, but I would prefer to put a note in the commit-body (some call it changelog, I refer to terms in email context)... "...found by kbuild test bot" with appropriate lines of warning. ( See example in [1]. ) - Sedat - [1] http://www.spinics.net/lists/linux-usb/msg110760.html -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h index 1a9fe09..e84b576 100644 --- a/drivers/net/wireless/ath/ath9k/ath9k.h +++ b/drivers/net/wireless/ath/ath9k/ath9k.h @@ -34,7 +34,7 @@ struct ath_vif; extern struct ieee80211_ops ath9k_ops; extern int ath9k_modparam_nohwcrypt; -extern int led_blink; +extern int ath9k_led_blink; extern bool is_ath9k_unloaded; extern int ath9k_use_chanctx; diff --git a/drivers/net/wireless/ath/ath9k/gpio.c b/drivers/net/wireless/ath/ath9k/gpio.c index 2fef7a4..da344b2 100644 --- a/drivers/net/wireless/ath/ath9k/gpio.c +++ b/drivers/net/wireless/ath/ath9k/gpio.c @@ -49,7 +49,7 @@ void ath_init_leds(struct ath_softc *sc) if (AR_SREV_9100(sc->sc_ah)) return; - if (!led_blink) + if (!ath9k_led_blink) sc->led_cdev.default_trigger = ieee80211_get_radio_led_name(sc->hw); diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index c43fec5..300d367 100644 --- a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -45,7 +45,7 @@ extern struct ieee80211_ops ath9k_htc_ops; extern int htc_modparam_nohwcrypt; #ifdef CONFIG_MAC80211_LEDS -extern int led_blink; +extern int ath9k_htc_led_blink; #endif enum htc_phymode { diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c b/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c index 998b558..2aabcbd 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c @@ -279,7 +279,7 @@ void ath9k_init_leds(struct ath9k_htc_priv *priv) else priv->ah->led_pin = ATH_LED_PIN_DEF; - if (!led_blink) + if (!ath9k_htc_led_blink) priv->led_cdev.default_trigger = ieee80211_get_radio_led_name(priv->hw); diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c index 9470127..fd22940 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c @@ -39,8 +39,8 @@ module_param_named(ps_enable, ath9k_ps_enable, int, 0444); MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); #ifdef CONFIG_MAC80211_LEDS -int led_blink = 1; -module_param_named(blink, led_blink, int, 0444); +int ath9k_htc_led_blink = 1; +module_param_named(blink, ath9k_htc_led_blink, int, 0444); MODULE_PARM_DESC(blink, "Enable LED blink on activity"); static const struct ieee80211_tpt_blink ath9k_htc_tpt_blink[] = { diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c index d1c3934..2aef14e 100644 --- a/drivers/net/wireless/ath/ath9k/init.c +++ b/drivers/net/wireless/ath/ath9k/init.c @@ -45,8 +45,8 @@ int ath9k_modparam_nohwcrypt; module_param_named(nohwcrypt, ath9k_modparam_nohwcrypt, int, 0444); MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption"); -int led_blink; -module_param_named(blink, led_blink, int, 0444); +int ath9k_led_blink; +module_param_named(blink, ath9k_led_blink, int, 0444); MODULE_PARM_DESC(blink, "Enable LED blink on activity"); static int ath9k_btcoex_enable;
ath9k and ath9k_htc use the variable name "led_blink" to indicate whether the module parameter "blink" is on. This name is easy to conflict with other variables, thus they are renamed. Signed-off-by: Hong Xu <hong@topbug.net> --- drivers/net/wireless/ath/ath9k/ath9k.h | 2 +- drivers/net/wireless/ath/ath9k/gpio.c | 2 +- drivers/net/wireless/ath/ath9k/htc.h | 2 +- drivers/net/wireless/ath/ath9k/htc_drv_gpio.c | 2 +- drivers/net/wireless/ath/ath9k/htc_drv_init.c | 4 ++-- drivers/net/wireless/ath/ath9k/init.c | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-)