netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] MAINTAINERS: update phylink/sfp keyword matching
@ 2020-08-05 14:34 Russell King
  2020-08-05 18:11 ` Linus Torvalds
  0 siblings, 1 reply; 11+ messages in thread
From: Russell King @ 2020-08-05 14:34 UTC (permalink / raw)
  To: linux-kernel, Linus Torvalds; +Cc: netdev

syzbot has revealed that the "phylink" keyword exists in non-phylink
related contexts in the bluetooth stack. To avoid receiving
inappropriate notifications, change the keyword matching regexp to
something which avoids this, while still allowing changes to networking
drivers that make use of phylink to be detected.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Linus,

Is this something you're willing to merge directly please?

Thanks.

 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4e2698cc7e23..3b11a8b84129 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15431,7 +15431,7 @@ F:	drivers/net/phy/phylink.c
 F:	drivers/net/phy/sfp*
 F:	include/linux/phylink.h
 F:	include/linux/sfp.h
-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)
 
 SGI GRU DRIVER
 M:	Dimitri Sivanich <sivanich@sgi.com>
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 14:34 [PATCH] MAINTAINERS: update phylink/sfp keyword matching Russell King
@ 2020-08-05 18:11 ` Linus Torvalds
  2020-08-05 18:22   ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Torvalds @ 2020-08-05 18:11 UTC (permalink / raw)
  To: Russell King; +Cc: Linux Kernel Mailing List, Netdev

On Wed, Aug 5, 2020 at 7:34 AM Russell King <rmk+kernel@armlinux.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)

Isn't the 'F' pattern already complete enough that maybe the K pattern
isn't even worth it?

Just a thought, no biggie.

           Linus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:11 ` Linus Torvalds
@ 2020-08-05 18:22   ` Russell King - ARM Linux admin
  2020-08-05 18:47     ` Joe Perches
  2020-08-05 18:54     ` Joe Perches
  0 siblings, 2 replies; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-08-05 18:22 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Netdev

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@armlinux.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.

> 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:22   ` Russell King - ARM Linux admin
@ 2020-08-05 18:47     ` Joe Perches
  2020-08-05 21:24       ` Andrew Lunn
  2020-08-05 22:09       ` Russell King - ARM Linux admin
  2020-08-05 18:54     ` Joe Perches
  1 sibling, 2 replies; 11+ messages in thread
From: Joe Perches @ 2020-08-05 18:47 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Linus Torvalds, Dmitry Vyukov
  Cc: Linux Kernel Mailing List, Netdev

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@armlinux.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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:22   ` Russell King - ARM Linux admin
  2020-08-05 18:47     ` Joe Perches
@ 2020-08-05 18:54     ` Joe Perches
  2020-08-05 22:02       ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 11+ messages in thread
From: Joe Perches @ 2020-08-05 18:54 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Linus Torvalds
  Cc: Linux Kernel Mailing List, Netdev

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@armlinux.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.

And perhaps easier to read would be to use multiple K: lines.
(?: used to avoid unnecessary capture groups)

K:	phylink\.h|struct\s+phylink
K:	(?:\.|\-\>)phylink_
K:	phylink_(?:autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:47     ` Joe Perches
@ 2020-08-05 21:24       ` Andrew Lunn
  2020-08-05 22:09       ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2020-08-05 21:24 UTC (permalink / raw)
  To: Joe Perches
  Cc: Russell King - ARM Linux admin, Linus Torvalds, Dmitry Vyukov,
	Linux Kernel Mailing List, Netdev

On Wed, Aug 05, 2020 at 11:47:38AM -0700, Joe Perches wrote:
> 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@armlinux.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?

Hi Joe

I think Rusells intention here is to match on MAC drivers which make
use of the phylink API exported to them.

SFF/SFP/SFP+ MODULE SUPPORT
M:      Russell King <linux@armlinux.org.uk>
L:      netdev@vger.kernel.org
S:      Maintained
F:      drivers/net/phy/phylink.c
F:      drivers/net/phy/sfp*
F:      include/linux/phylink.h
F:      include/linux/sfp.h
K:      phylink

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

Try that again, but skip files matched by the F: clauses.

I suspect the matches you get then more closely approximates the K:
Russell is suggesting.

   Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:54     ` Joe Perches
@ 2020-08-05 22:02       ` Russell King - ARM Linux admin
  2020-08-05 22:09         ` Joe Perches
  0 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-08-05 22:02 UTC (permalink / raw)
  To: Joe Perches; +Cc: Linus Torvalds, Linux Kernel Mailing List, Netdev

On Wed, Aug 05, 2020 at 11:54:25AM -0700, Joe Perches wrote:
> 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@armlinux.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.
> 
> And perhaps easier to read would be to use multiple K: lines.
> (?: used to avoid unnecessary capture groups)
> 
> K:	phylink\.h|struct\s+phylink
> K:	(?:\.|\-\>)phylink_

That one is definitely incorrect.  It is not .phylink_ or ->phylink_,
it was .phylink (without _) or >phylink_

> K:	phylink_(?:autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)
> 
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 18:47     ` Joe Perches
  2020-08-05 21:24       ` Andrew Lunn
@ 2020-08-05 22:09       ` Russell King - ARM Linux admin
  2020-08-05 23:07         ` Joe Perches
  1 sibling, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-08-05 22:09 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linus Torvalds, Dmitry Vyukov, Linux Kernel Mailing List, Netdev

On Wed, Aug 05, 2020 at 11:47:38AM -0700, Joe Perches wrote:
> 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@armlinux.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?

It is sufficient; as I said, I've spent a morning running this:

#!/usr/bin/perl
$re = 'phylink\.h|struct\s+phylink|\.phylink|>phylink_|phylink_(autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)';
foreach $f (@ARGV) {
        open F, $f;
        $l = 1;
        while (<F>) {
                chomp;
                print "$f:$l: $_\n" if /$re/;
                $l++;
        }
        close F;
}

through:

$ find drivers -type f -print0 | xargs -0 ./check.pl | diff -u pl-ref.out - |less

where pl-ref.out is the original K: matching of just "phylink" and
looking at the differences to ensure I'm excluding just stuff that
doesn't concern me, while getting a high hit rate on the stuff
that I do want.

Now, I'm not saying that there isn't a better way, but this is not
something I want to spend days on.  So I got something that works
for me, and that's what I've sent Linus.

Going through your list...

>       4 phylink_add

Not sure what this is.  Doesn't seem to be anything to do with what
I maintain.

>       7 phylink_an_mode_str

static function.

>       4 phylink_apply_manual_flow

static function.

>       3 phylink_attach_phy

static function.

>      26 phylink_autoneg_inband

This one public and included.

>       4 phylink_bringup_phy

static function.

>       3 phylink_change_inband_advert

static function.

>       6 phylink_clear

This one public and included.

>       4 phylink_complete
>       2 phylink_complete_evt

Nothing to do with phylink.

>     145 phylink_config

Included.

>       3 phylink_connect
>       8 phylink_connect_phy

Both included under one.

>      39 phylink_create

Included.

>      10 phylink_dbg

static function.

... shall I go on?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 22:02       ` Russell King - ARM Linux admin
@ 2020-08-05 22:09         ` Joe Perches
  2020-08-05 22:12           ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2020-08-05 22:09 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Linus Torvalds, Linux Kernel Mailing List, Netdev

On Wed, 2020-08-05 at 23:02 +0100, Russell King - ARM Linux admin wrote:
> On Wed, Aug 05, 2020 at 11:54:25AM -0700, Joe Perches wrote:
> > 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@armlinux.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.
> > 
> > And perhaps easier to read would be to use multiple K: lines.
> > (?: used to avoid unnecessary capture groups)
> > 
> > K:	phylink\.h|struct\s+phylink
> > K:	(?:\.|\-\>)phylink_
> 
> That one is definitely incorrect.  It is not .phylink_ or ->phylink_,
> it was .phylink (without _) or >phylink_

Hi Russell.

I don't see the difference.

All uses of .phylink are followed with _
as far as I can tell.

$ git grep -Poh "\.phylink\S*"|sort|uniq -c
      1 .phylink_fixed_state
      2 .phylink_mac_an_restart
      9 .phylink_mac_config
      1 .phylink_mac_config.
     11 .phylink_mac_link_down
      6 .phylink_mac_link_state
      9 .phylink_mac_link_up
     38 .phylink_validate



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 22:09         ` Joe Perches
@ 2020-08-05 22:12           ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-08-05 22:12 UTC (permalink / raw)
  To: Joe Perches; +Cc: Linus Torvalds, Linux Kernel Mailing List, Netdev

On Wed, Aug 05, 2020 at 03:09:34PM -0700, Joe Perches wrote:
> On Wed, 2020-08-05 at 23:02 +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Aug 05, 2020 at 11:54:25AM -0700, Joe Perches wrote:
> > > 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@armlinux.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.
> > > 
> > > And perhaps easier to read would be to use multiple K: lines.
> > > (?: used to avoid unnecessary capture groups)
> > > 
> > > K:	phylink\.h|struct\s+phylink
> > > K:	(?:\.|\-\>)phylink_
> > 
> > That one is definitely incorrect.  It is not .phylink_ or ->phylink_,
> > it was .phylink (without _) or >phylink_
> 
> Hi Russell.
> 
> I don't see the difference.
> 
> All uses of .phylink are followed with _
> as far as I can tell.
> 
> $ git grep -Poh "\.phylink\S*"|sort|uniq -c
>       1 .phylink_fixed_state
>       2 .phylink_mac_an_restart
>       9 .phylink_mac_config
>       1 .phylink_mac_config.
>      11 .phylink_mac_link_down
>       6 .phylink_mac_link_state
>       9 .phylink_mac_link_up
>      38 .phylink_validate

Yes, you're right, but as I explained, I got something that works for
me, and I wasn't going to put more effort in.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] MAINTAINERS: update phylink/sfp keyword matching
  2020-08-05 22:09       ` Russell King - ARM Linux admin
@ 2020-08-05 23:07         ` Joe Perches
  0 siblings, 0 replies; 11+ messages in thread
From: Joe Perches @ 2020-08-05 23:07 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Linus Torvalds, Dmitry Vyukov, Linux Kernel Mailing List, Netdev

On Wed, 2020-08-05 at 23:09 +0100, Russell King - ARM Linux admin wrote:
> On Wed, Aug 05, 2020 at 11:47:38AM -0700, Joe Perches wrote:
> > 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@armlinux.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?
> 
> It is sufficient; as I said, I've spent a morning running this:

Cool for you, I was just asking.

How you determine what functions you're interested in
is up to you.

Another mechanism might have been something like:

$ git grep -Poh '(?:static\s+inline|EXPORT_SYMBOL).*phylink_[a-z]+' | \ 
  grep -Poh 'phylink_[a-z]+' | sort | uniq

Maybe something that could be made generic in get_maintainer
for people maintaining specific EXPORT_SYMBOL(foo) blocks.

Maybe another letter/look could be added to MAINTAINERS like:

E:	symbol_regex

as another similar mechanism for keywords but just for data
or functions marked as exported symbols.

For example, that E: might help minimize xdp matches as xdp
is used by many other bits of code that aren't express data
path uses.

MAINTAINERS-XDP (eXpress Data Path)
MAINTAINERS-M:  Alexei Starovoitov <ast@kernel.org>
MAINTAINERS-M:  Daniel Borkmann <daniel@iogearbox.net>
MAINTAINERS-M:  David S. Miller <davem@davemloft.net>
MAINTAINERS-M:  Jakub Kicinski <kuba@kernel.org>
MAINTAINERS-M:  Jesper Dangaard Brouer <hawk@kernel.org>
MAINTAINERS-M:  John Fastabend <john.fastabend@gmail.com>
MAINTAINERS-L:  netdev@vger.kernel.org
MAINTAINERS-L:  bpf@vger.kernel.org
MAINTAINERS-S:  Supported
MAINTAINERS-F:  include/net/xdp.h
MAINTAINERS-F:  include/trace/events/xdp.h
MAINTAINERS-F:  kernel/bpf/cpumap.c
MAINTAINERS-F:  kernel/bpf/devmap.c
MAINTAINERS-F:  net/core/xdp.c
MAINTAINERS-N:  xdp
MAINTAINERS:K:  xdp



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-08-05 23:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05 14:34 [PATCH] MAINTAINERS: update phylink/sfp keyword matching Russell King
2020-08-05 18:11 ` Linus Torvalds
2020-08-05 18:22   ` Russell King - ARM Linux admin
2020-08-05 18:47     ` Joe Perches
2020-08-05 21:24       ` Andrew Lunn
2020-08-05 22:09       ` Russell King - ARM Linux admin
2020-08-05 23:07         ` Joe Perches
2020-08-05 18:54     ` Joe Perches
2020-08-05 22:02       ` Russell King - ARM Linux admin
2020-08-05 22:09         ` Joe Perches
2020-08-05 22:12           ` Russell King - ARM Linux admin

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).