* Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint [not found] <20180328190540.370956-5-ast@kernel.org> @ 2018-05-23 11:03 ` Johannes Berg 2018-05-24 23:28 ` Alexei Starovoitov 0 siblings, 1 reply; 4+ messages in thread From: Johannes Berg @ 2018-05-23 11:03 UTC (permalink / raw) To: Alexei Starovoitov, davem Cc: daniel, torvalds, peterz, rostedt, mathieu.desnoyers, netdev, kernel-team, linux-api, linux-wireless On Wed, 2018-03-28 at 12:05 -0700, Alexei Starovoitov wrote: > fix iwlwifi_dev_ucode_error tracepoint to pass pointer to a table > instead of all 17 arguments by value. > dvm/main.c and mvm/utils.c have 'struct iwl_error_event_table' > defined with very similar yet subtly different fields and offsets. > tracepoint is still common and using definition of 'struct iwl_error_event_table' > from dvm/commands.h while copying fields. > Long term this tracepoint probably should be split into two. It would've been nice to CC the wireless list for wireless related patches ... > --- a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c > @@ -30,6 +30,7 @@ > #ifndef __CHECKER__ > #include "iwl-trans.h" > > +#include "dvm/commands.h" In particular, this breaks the whole driver abstraction. > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c > @@ -549,12 +549,7 @@ static void iwl_mvm_dump_lmac_error_log(struct iwl_mvm *mvm, u32 base) > > IWL_ERR(mvm, "Loaded firmware version: %s\n", mvm->fw->fw_version); > > - trace_iwlwifi_dev_ucode_error(trans->dev, table.error_id, table.tsf_low, > - table.data1, table.data2, table.data3, > - table.blink2, table.ilink1, > - table.ilink2, table.bcon_time, table.gp1, > - table.gp2, table.fw_rev_type, table.major, > - table.minor, table.hw_ver, table.brd_ver); > + trace_iwlwifi_dev_ucode_error(trans->dev, &table, table.hw_ver, table.brd_ver); This is also utterly wrong because mvm has - for better or worse - a different type "struct iwl_error_event_table" in this file ... This really should never have gotten into the tree. johannes ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint 2018-05-23 11:03 ` [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint Johannes Berg @ 2018-05-24 23:28 ` Alexei Starovoitov 2018-05-24 23:39 ` Steven Rostedt 2018-05-25 10:48 ` Kalle Valo 0 siblings, 2 replies; 4+ messages in thread From: Alexei Starovoitov @ 2018-05-24 23:28 UTC (permalink / raw) To: Johannes Berg Cc: Alexei Starovoitov, davem, daniel, torvalds, peterz, rostedt, mathieu.desnoyers, netdev, kernel-team, linux-api, linux-wireless On Wed, May 23, 2018 at 01:03:08PM +0200, Johannes Berg wrote: > On Wed, 2018-03-28 at 12:05 -0700, Alexei Starovoitov wrote: > > fix iwlwifi_dev_ucode_error tracepoint to pass pointer to a table > > instead of all 17 arguments by value. > > dvm/main.c and mvm/utils.c have 'struct iwl_error_event_table' > > defined with very similar yet subtly different fields and offsets. > > tracepoint is still common and using definition of 'struct iwl_error_event_table' > > from dvm/commands.h while copying fields. > > Long term this tracepoint probably should be split into two. > > It would've been nice to CC the wireless list for wireless related > patches ... Ohh. I didn't realize that networking wireless doesn't fall under netdev. I thought wireless folks are silent because they are embarrassed by a function with 17 arguments. > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c > > @@ -30,6 +30,7 @@ > > #ifndef __CHECKER__ > > #include "iwl-trans.h" > > > > +#include "dvm/commands.h" > > In particular, this breaks the whole driver abstraction. > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c > > @@ -549,12 +549,7 @@ static void iwl_mvm_dump_lmac_error_log(struct iwl_mvm *mvm, u32 base) > > > > IWL_ERR(mvm, "Loaded firmware version: %s\n", mvm->fw->fw_version); > > > > - trace_iwlwifi_dev_ucode_error(trans->dev, table.error_id, table.tsf_low, > > - table.data1, table.data2, table.data3, > > - table.blink2, table.ilink1, > > - table.ilink2, table.bcon_time, table.gp1, > > - table.gp2, table.fw_rev_type, table.major, > > - table.minor, table.hw_ver, table.brd_ver); > > + trace_iwlwifi_dev_ucode_error(trans->dev, &table, table.hw_ver, table.brd_ver); > > This is also utterly wrong because mvm has - for better or worse - a > different type "struct iwl_error_event_table" in this file ... As I was trying to explain in the commit log the single struct is used in both places, but differences in two "struct iwl_error_event_table" are carefully matched field and by field. For two extra fields it was not possible and they are passed separately as you can see above. I still believe that tracepoint output is still exactly the same before and after the patch. I guess you see the breakage because new fields got added into one "struct iwl_error_event_table", but were not added to its evil twin "struct iwl_error_event_table" with the same name after the patch landed ? imo wireless folks need to avoid such naming conflicts. I suggest to isolate common fields into separate base struct and give two children structs different names. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint 2018-05-24 23:28 ` Alexei Starovoitov @ 2018-05-24 23:39 ` Steven Rostedt 2018-05-25 10:48 ` Kalle Valo 1 sibling, 0 replies; 4+ messages in thread From: Steven Rostedt @ 2018-05-24 23:39 UTC (permalink / raw) To: Alexei Starovoitov Cc: Johannes Berg, Alexei Starovoitov, davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev, kernel-team, linux-api, linux-wireless On Thu, 24 May 2018 16:28:39 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > Ohh. I didn't realize that networking wireless doesn't fall under netdev. > I thought wireless folks are silent because they are embarrassed > by a function with 17 arguments. Please lets refrain from the demeaning comments. I agree with your argument, but not the tone. -- Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint 2018-05-24 23:28 ` Alexei Starovoitov 2018-05-24 23:39 ` Steven Rostedt @ 2018-05-25 10:48 ` Kalle Valo 1 sibling, 0 replies; 4+ messages in thread From: Kalle Valo @ 2018-05-25 10:48 UTC (permalink / raw) To: Alexei Starovoitov Cc: Johannes Berg, Alexei Starovoitov, davem, daniel, torvalds, peterz, rostedt, mathieu.desnoyers, netdev, kernel-team, linux-api, linux-wireless Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Wed, May 23, 2018 at 01:03:08PM +0200, Johannes Berg wrote: >> On Wed, 2018-03-28 at 12:05 -0700, Alexei Starovoitov wrote: >> > fix iwlwifi_dev_ucode_error tracepoint to pass pointer to a table >> > instead of all 17 arguments by value. >> > dvm/main.c and mvm/utils.c have 'struct iwl_error_event_table' >> > defined with very similar yet subtly different fields and offsets. >> > tracepoint is still common and using definition of 'struct iwl_error_event_table' >> > from dvm/commands.h while copying fields. >> > Long term this tracepoint probably should be split into two. >> >> It would've been nice to CC the wireless list for wireless related >> patches ... > > Ohh. I didn't realize that networking wireless doesn't fall under netdev. > I thought wireless folks are silent because they are embarrassed > by a function with 17 arguments. Really, this is the level of discussion now? You don't even bother to CC the driver maintainers and your first reaction is that they are just too embarrassed to answer? Oh man... But if you continue doing these kind of "drive-by cleanups" to a subsystem you are not familiar with, at least don't assume anything and instead use get_maintainer script to find the maintainers so that the patches get reviewed and applied to the correct tree: $ scripts/get_maintainer.pl --no-rolestats --no-git -f drivers/net/wireless/intel/iwlwifi/dvm/main.c Johannes Berg <johannes.berg@intel.com> Emmanuel Grumbach <emmanuel.grumbach@intel.com> Luca Coelho <luciano.coelho@intel.com> Intel Linux Wireless <linuxwifi@intel.com> Kalle Valo <kvalo@codeaurora.org> "David S. Miller" <davem@davemloft.net> Daniel Borkmann <daniel@iogearbox.net> Alexei Starovoitov <ast@kernel.org> Kees Cook <keescook@chromium.org> linux-wireless@vger.kernel.org netdev@vger.kernel.org linux-kernel@vger.kernel.org -- Kalle Valo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-25 10:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180328190540.370956-5-ast@kernel.org>
2018-05-23 11:03 ` [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint Johannes Berg
2018-05-24 23:28 ` Alexei Starovoitov
2018-05-24 23:39 ` Steven Rostedt
2018-05-25 10:48 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).