Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

[Johannes Berg]

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

Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

[Alexei Starovoitov]

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.

Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

[Steven Rostedt]

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

Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

[Kalle Valo]

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