Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint
Alexei Starovoitovwrites: > 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 Emmanuel Grumbach Luca Coelho Intel Linux Wireless Kalle Valo "David S. Miller" Daniel Borkmann Alexei Starovoitov Kees Cook linux-wirel...@vger.kernel.org netdev@vger.kernel.org linux-ker...@vger.kernel.org -- Kalle Valo
Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint
On Thu, 24 May 2018 16:28:39 -0700 Alexei Starovoitovwrote: > 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
Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint
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.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.
Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint
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.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