| Message ID | 20180328190540.370956-5-ast@kernel.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf, tracing: introduce bpf raw tracepoints | expand |
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
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.
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
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
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/main.c b/drivers/net/wireless/intel/iwlwifi/dvm/main.c index d11d72615de2..e68254e12764 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/main.c +++ b/drivers/net/wireless/intel/iwlwifi/dvm/main.c @@ -1651,12 +1651,7 @@ static void iwl_dump_nic_error_log(struct iwl_priv *priv) priv->status, table.valid); } - trace_iwlwifi_dev_ucode_error(trans->dev, table.error_id, table.tsf_low, - table.data1, table.data2, table.line, - table.blink2, table.ilink1, table.ilink2, - table.bcon_time, table.gp1, table.gp2, - table.gp3, table.ucode_ver, table.hw_ver, - 0, table.brd_ver); + trace_iwlwifi_dev_ucode_error(trans->dev, &table, 0, table.brd_ver); IWL_ERR(priv, "0x%08X | %-28s\n", table.error_id, desc_lookup(table.error_id)); IWL_ERR(priv, "0x%08X | uPc\n", table.pc); diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace-iwlwifi.h b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace-iwlwifi.h index 9518a82f44c2..27e3e4e96aa2 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace-iwlwifi.h +++ b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace-iwlwifi.h @@ -126,14 +126,11 @@ TRACE_EVENT(iwlwifi_dev_tx, __entry->framelen, __entry->skbaddr) ); +struct iwl_error_event_table; TRACE_EVENT(iwlwifi_dev_ucode_error, - TP_PROTO(const struct device *dev, u32 desc, u32 tsf_low, - u32 data1, u32 data2, u32 line, u32 blink2, u32 ilink1, - u32 ilink2, u32 bcon_time, u32 gp1, u32 gp2, u32 rev_type, - u32 major, u32 minor, u32 hw_ver, u32 brd_ver), - TP_ARGS(dev, desc, tsf_low, data1, data2, line, - blink2, ilink1, ilink2, bcon_time, gp1, gp2, - rev_type, major, minor, hw_ver, brd_ver), + TP_PROTO(const struct device *dev, const struct iwl_error_event_table *table, + u32 hw_ver, u32 brd_ver), + TP_ARGS(dev, table, hw_ver, brd_ver), TP_STRUCT__entry( DEV_ENTRY __field(u32, desc) @@ -155,20 +152,20 @@ TRACE_EVENT(iwlwifi_dev_ucode_error, ), TP_fast_assign( DEV_ASSIGN; - __entry->desc = desc; - __entry->tsf_low = tsf_low; - __entry->data1 = data1; - __entry->data2 = data2; - __entry->line = line; - __entry->blink2 = blink2; - __entry->ilink1 = ilink1; - __entry->ilink2 = ilink2; - __entry->bcon_time = bcon_time; - __entry->gp1 = gp1; - __entry->gp2 = gp2; - __entry->rev_type = rev_type; - __entry->major = major; - __entry->minor = minor; + __entry->desc = table->error_id; + __entry->tsf_low = table->tsf_low; + __entry->data1 = table->data1; + __entry->data2 = table->data2; + __entry->line = table->line; + __entry->blink2 = table->blink2; + __entry->ilink1 = table->ilink1; + __entry->ilink2 = table->ilink2; + __entry->bcon_time = table->bcon_time; + __entry->gp1 = table->gp1; + __entry->gp2 = table->gp2; + __entry->rev_type = table->gp3; + __entry->major = table->ucode_ver; + __entry->minor = table->hw_ver; __entry->hw_ver = hw_ver; __entry->brd_ver = brd_ver; ), diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c index 50510fb6ab8c..6aa719865a58 100644 --- 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" #define CREATE_TRACE_POINTS #include "iwl-devtrace.h" diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/utils.c b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c index d65e1db7c097..5442ead876eb 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/utils.c +++ 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); IWL_ERR(mvm, "0x%08X | %-28s\n", table.error_id, desc_lookup(table.error_id)); IWL_ERR(mvm, "0x%08X | trm_hw_status0\n", table.trm_hw_status0);
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. Signed-off-by: Alexei Starovoitov <ast@kernel.org> --- drivers/net/wireless/intel/iwlwifi/dvm/main.c | 7 +--- .../wireless/intel/iwlwifi/iwl-devtrace-iwlwifi.h | 39 ++++++++++------------ drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c | 1 + drivers/net/wireless/intel/iwlwifi/mvm/utils.c | 7 +--- 4 files changed, 21 insertions(+), 33 deletions(-)