| Message ID | E1k3KUx-0000da-In@rmk-PC.armlinux.org.uk |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | MAINTAINERS: update phylink/sfp keyword matching | expand |
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
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) > > >
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?
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
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.
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
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>
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(-)