| Message ID | 20250202021155.1019222-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] MAINTAINERS: add entry for ethtool | expand |
On Sat, Feb 01, 2025 at 06:11:55PM -0800, Jakub Kicinski wrote: > I feel like we don't do a good enough keeping authors of driver > APIs around. The ethtool code base was very nicely compartmentalized > by Michal. Establish a precedent of creating MAINTAINERS entries > for "sections" of the ethtool API. Use Andrew and cable test as > a sample entry. The entry should ideally cover 3 elements: > a core file, test(s), and keywords. The last one is important > because we intend the entries to cover core code *and* reviews > of drivers implementing given API! > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > This patch is a nop from process perspective, since Andrew already > is a maintainer and reviews all this code. Let's focus on discussing > merits of the "section entries" in abstract? In the first instance this seems like a good direction to go in to me. My only slight concern is that we might see an explosion in entries. Do we think so? Do we mind if that happens?
On Mon, Feb 03, 2025 at 10:56:47AM +0000, Simon Horman wrote: > On Sat, Feb 01, 2025 at 06:11:55PM -0800, Jakub Kicinski wrote: > > I feel like we don't do a good enough keeping authors of driver > > APIs around. The ethtool code base was very nicely compartmentalized > > by Michal. Establish a precedent of creating MAINTAINERS entries > > for "sections" of the ethtool API. Use Andrew and cable test as > > a sample entry. The entry should ideally cover 3 elements: > > a core file, test(s), and keywords. The last one is important > > because we intend the entries to cover core code *and* reviews > > of drivers implementing given API! > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > --- > > This patch is a nop from process perspective, since Andrew already > > is a maintainer and reviews all this code. Let's focus on discussing > > merits of the "section entries" in abstract? > > In the first instance this seems like a good direction to go in to me. > My only slight concern is that we might see an explosion in entries. I don't think that will happen. I don't think we really have many sections of ethtool which people personally care about, always try to review across all drivers. Even if it does explode, so what. Is ./scripts/get_maintainer.pl the bottleneck in any workflows? Andrew
On Mon, 3 Feb 2025 14:29:23 +0100 Andrew Lunn wrote: > > In the first instance this seems like a good direction to go in to me. > > My only slight concern is that we might see an explosion in entries. > > I don't think that will happen. I don't think we really have many > sections of ethtool which people personally care about, always try to > review across all drivers. Agreed, FWIW. > Even if it does explode, so what. Is ./scripts/get_maintainer.pl the > bottleneck in any workflows? Only concern there could be the keywords, we had issues with regexps being too expensive in the past. There are plenty examples of how to do them right now, tho.
On 2/2/25 3:11 AM, Jakub Kicinski wrote: > This patch is a nop from process perspective, since Andrew already > is a maintainer and reviews all this code. Let's focus on discussing > merits of the "section entries" in abstract? Should the keyword be a little more generic, i.e. just 'cable_test'? AFAICS the current one doesn't catch the device drivers, I agree encouraging more driver API reviewer would be great, but I personally have a slight preference to add/maintain entries only they actually affect the process. What about tying the creation of the entry to some specific contribution? /P
On Mon, Feb 03, 2025 at 02:29:23PM +0100, Andrew Lunn wrote: > On Mon, Feb 03, 2025 at 10:56:47AM +0000, Simon Horman wrote: > > On Sat, Feb 01, 2025 at 06:11:55PM -0800, Jakub Kicinski wrote: > > > I feel like we don't do a good enough keeping authors of driver > > > APIs around. The ethtool code base was very nicely compartmentalized > > > by Michal. Establish a precedent of creating MAINTAINERS entries > > > for "sections" of the ethtool API. Use Andrew and cable test as > > > a sample entry. The entry should ideally cover 3 elements: > > > a core file, test(s), and keywords. The last one is important > > > because we intend the entries to cover core code *and* reviews > > > of drivers implementing given API! > > > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > --- > > > This patch is a nop from process perspective, since Andrew already > > > is a maintainer and reviews all this code. Let's focus on discussing > > > merits of the "section entries" in abstract? > > > > In the first instance this seems like a good direction to go in to me. > > My only slight concern is that we might see an explosion in entries. > > I don't think that will happen. I don't think we really have many > sections of ethtool which people personally care about, always try to > review across all drivers. > > Even if it does explode, so what. Is ./scripts/get_maintainer.pl the > bottleneck in any workflows? Thanks Andrew, I'm not overly concerned by the points I raised either, but I did think they were worth raising. And given that doing so didn't raise any alarm bells (so far), I'm happy for this patch to proceed. Reviewed-by: Simon Horman <horms@kernel.org>
On Tue, 4 Feb 2025 10:26:40 +0100 Paolo Abeni wrote: > On 2/2/25 3:11 AM, Jakub Kicinski wrote: > > This patch is a nop from process perspective, since Andrew already > > is a maintainer and reviews all this code. Let's focus on discussing > > merits of the "section entries" in abstract? > > Should the keyword be a little more generic, i.e. just 'cable_test'? > AFAICS the current one doesn't catch the device drivers, > > I agree encouraging more driver API reviewer would be great, but I > personally have a slight preference to add/maintain entries only they > actually affect the process. You're right, I was going after the op name. Seems like a good default keyword. But it appears that there are two layers of ops, one called start_cable_test and the next cable_test_start, so this isn't catching actual drivers. > What about tying the creation of the entry to some specific contribution? Sure. I'm adding this so that we have a commit to point people at as an example when they contribute what should be a new section. Maybe I don't understand the question..
On 2/4/25 4:37 PM, Jakub Kicinski wrote: > On Tue, 4 Feb 2025 10:26:40 +0100 Paolo Abeni wrote: >> What about tying the creation of the entry to some specific contribution? > > Sure. I'm adding this so that we have a commit to point people at > as an example when they contribute what should be a new section. > Maybe I don't understand the question.. I meant that this section could be added together with a new associated name when a suitable person will pop-up. Or course that would not help as an example, and I initially did see there was such a goal. I'm fine with adding new entry even now. /P
On Tue, Feb 04, 2025 at 07:37:59AM -0800, Jakub Kicinski wrote: > On Tue, 4 Feb 2025 10:26:40 +0100 Paolo Abeni wrote: > > On 2/2/25 3:11 AM, Jakub Kicinski wrote: > > > This patch is a nop from process perspective, since Andrew already > > > is a maintainer and reviews all this code. Let's focus on discussing > > > merits of the "section entries" in abstract? > > > > Should the keyword be a little more generic, i.e. just 'cable_test'? > > AFAICS the current one doesn't catch the device drivers, > > > > I agree encouraging more driver API reviewer would be great, but I > > personally have a slight preference to add/maintain entries only they > > actually affect the process. > > You're right, I was going after the op name. Seems like a good default > keyword. But it appears that there are two layers of ops, one called > start_cable_test and the next cable_test_start, so this isn't catching > actual drivers. https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/phy.h#L1080 The op in the driver structure is cable_test_start. There is also cable_test_tdr_start but so far only Marvell hardware supports raw TDR data. At the moment, cable_test_start does not produce any false positives anywhere else in the kernel. Andrew
diff --git a/MAINTAINERS b/MAINTAINERS index 4e701b9a57e4..9bf31ba720b6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16469,6 +16469,12 @@ F: include/uapi/linux/ethtool* F: net/ethtool/ F: tools/testing/selftests/drivers/net/*/ethtool* +NETWORKING [ETHTOOL CABLE TEST] +M: Andrew Lunn <andrew@lunn.ch> +F: net/ethtool/cabletest.c +F: tools/testing/selftests/drivers/net/*/ethtool* +K: start_cable_test + NETWORKING [GENERAL] M: "David S. Miller" <davem@davemloft.net> M: Eric Dumazet <edumazet@google.com>
I feel like we don't do a good enough keeping authors of driver APIs around. The ethtool code base was very nicely compartmentalized by Michal. Establish a precedent of creating MAINTAINERS entries for "sections" of the ethtool API. Use Andrew and cable test as a sample entry. The entry should ideally cover 3 elements: a core file, test(s), and keywords. The last one is important because we intend the entries to cover core code *and* reviews of drivers implementing given API! Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- This patch is a nop from process perspective, since Andrew already is a maintainer and reviews all this code. Let's focus on discussing merits of the "section entries" in abstract? --- MAINTAINERS | 6 ++++++ 1 file changed, 6 insertions(+)