Message-ID: <957f48692a2f0bc4df2d83068073c4822da30eef.camel@perches.com>
Date:   Wed, 05 Aug 2020 11:47:38 -0700
From:   Joe Perches <joe@...ches.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching

On Wed, 2020-08-05 at 19:22 +0100, Russell King - ARM Linux admin wrote:
> On Wed, Aug 05, 2020 at 11:11:28AM -0700, Linus Torvalds wrote:
> > On Wed, Aug 5, 2020 at 7:34 AM Russell King <rmk+kernel@...linux.org.uk> wrote:
> > > Is this something you're willing to merge directly please?
> > 
> > Done.
> > 
> > That said:
> > 
> > > -K:     phylink
> > > +K:     phylink\.h|struct\s+phylink|\.phylink|>phylink_|phylink_(autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)
> > 
> > That's a very awkward pattern. I wonder if there could be better ways
> > to express this (ie "only apply this pattern to these files" kind of
> > thing)
> 
> Yes, it's extremely awkward - I spent much of the morning with perl
> testing it out on the drivers/ subtree.

There are a lot of phylink_<foo> in the kernel.
Are those really the only uses you want to watch?

$ git grep -P -oh 'phylink_\w+'| sort | uniq -c

      4 phylink_add
      7 phylink_an_mode_str
      4 phylink_apply_manual_flow
      3 phylink_attach_phy
     26 phylink_autoneg_inband
      4 phylink_bringup_phy
      3 phylink_change_inband_advert
      6 phylink_clear
      4 phylink_complete
      2 phylink_complete_evt
    145 phylink_config
      3 phylink_connect
      8 phylink_connect_phy
     39 phylink_create
     10 phylink_dbg
      3 phylink_decode_c37_word
      2 phylink_decode_sgmii_word
     22 phylink_destroy
     11 phylink_disable_state
      1 phylink_disconnect
     23 phylink_disconnect_phy
      4 phylink_do_bit
      2 phylink_downs
     12 phylink_err
      7 phylink_ethtool_get_eee
      1 phylink_ethtool_get_eee_err
     10 phylink_ethtool_get_pauseparam
     11 phylink_ethtool_get_wol
     13 phylink_ethtool_ksettings_get
     13 phylink_ethtool_ksettings_set
     10 phylink_ethtool_nway_reset
      7 phylink_ethtool_set_eee
     10 phylink_ethtool_set_pauseparam
      9 phylink_ethtool_set_wol
      2 phylink_fixed_poll
      6 phylink_fixed_state
      4 phylink_gen_key
      5 phylink_get_eee_err
      6 phylink_get_fixed_state
      3 phylink_get_ksettings
      2 phylink_handler
     10 phylink_helper_basex_speed
      6 phylink_info
      4 phylink_init_eee
      3 phylink_is_empty_linkmode
      4 phylink_link_down
      2 phylink_link_handler
    168 phylink_link_state
      2 phylink_link_up
     11 phylink_mac_an_restart
     19 phylink_mac_change
     33 phylink_mac_config
      3 phylink_mac_initial_config
     33 phylink_mac_link_down
     18 phylink_mac_link_state
     28 phylink_mac_link_up
     36 phylink_mac_ops
      5 phylink_mac_pcs_an_restart
     10 phylink_mac_pcs_get_state
      4 phylink_major_config
      2 phylink_merge_link_mode
      4 phylink_mii_c22_pcs_an_restart
      4 phylink_mii_c22_pcs_config
      4 phylink_mii_c22_pcs_get_state
      5 phylink_mii_c22_pcs_set_advertisement
      3 phylink_mii_c45_pcs_get_state
      3 phylink_mii_emul_read
     13 phylink_mii_ioctl
      2 phylink_mii_read
      2 phylink_mii_write
      4 phylink_node
     19 phylink_of_phy_connect
     16 phylink_ops
      2 phylink_op_type
      2 phylink_parse_fixedlink
      2 phylink_parse_mode
      2 phylink_pause_to_str
     18 phylink_pcs
      6 phylink_pcs_ops
      5 phylink_phy_change
      2 phylink_phy_no_inband
      2 phylink_phy_read
      2 phylink_phy_write
      6 phylink_printk
      2 phylink_register
      2 phylink_register_sfp
      2 phylink_resolve
      2 phylink_resolve_flow
      8 phylink_run_resolve
      3 phylink_run_resolve_and_disable
    406 phylink_set
      5 phylink_set_pcs
     23 phylink_set_port_modes
      2 phylink_setup
      2 phylink_sfp_attach
      4 phylink_sfp_config
      2 phylink_sfp_connect_phy
      2 phylink_sfp_detach
      2 phylink_sfp_disconnect_phy
      2 phylink_sfp_link_down
      2 phylink_sfp_link_up
      2 phylink_sfp_module_insert
      2 phylink_sfp_module_start
      2 phylink_sfp_module_stop
     11 phylink_speed_down
      7 phylink_speed_up
     21 phylink_start
     23 phylink_stop
     31 phylink_test
      5 phylink_to_dpaa2_mac
      7 phylink_to_port
      2 phylink_ups
    125 phylink_validate
      4 phylink_warn
      7 phylink_zero

> > Isn't the 'F' pattern already complete enough that maybe the K pattern
> > isn't even worth it?
> 
> Unfortunately not; I used not to have a K: line, which presented the
> problem that we had users of phylink added to the kernel that were not
> being reviewed.  So, the suggestion was to add a K: line.
> 
> However, I'm now being spammed by syzbot (I've received multiple emails
> about the same problem) because, rather than MAINTAINERS being applied
> to just patches, it is now being applied to entire source files.  This
> means that the previous "K: phylink" entry matches not just on patches
> (which can be easily ignored) but entire files, such as
> net/bluetooth/hci_event.c which happens to contain "phylink" in a
> function name.
> 
> So, when syzbot identifies there is a problem in
> net/bluetooth/hci_event.c, it sends me a report, despite it having
> no relevance for me.

Maybe instead syzbot could ignore K: lines
by adding --nokeywords to its command line
for get_maintainer.pl.

Powered by blists - more mailing lists