| Message ID | 20241004-doc-mc-clean-v1-1-20c28dcb0d52@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] docs: netdev: document guidance on cleanup patches | expand |
On Fri, 04 Oct 2024 10:49:53 +0100 Simon Horman wrote: > The purpose of this section is to document what is the current practice > regarding clean-up patches which address checkpatch warnings and similar > problems. I feel there is a value in having this documented so others > can easily refer to it. > > Clearly this topic is subjective. And to some extent the current > practice discourages a wider range of patches than is described here. > But I feel it is best to start somewhere, with the most well established > part of the current practice. > > -- > I did think this was already documented. And perhaps it is. > But I was unable to find it after a quick search. Thanks a lot for documenting it, this is great! All the suggestions below are optional, happy to merge as is. > +Clean-Up Patches > +~~~~~~~~~~~~~~~~ nit: other sections use sentence-like capitalization (only capitalizing the first word), is that incorrect? Or should we ay "Clean-up patches" here? > +Netdev discourages patches which perform simple clean-ups, which are not in > +the context of other work. For example addressing ``checkpatch.pl`` > +warnings, or :ref:`local variable ordering<rcs>` issues. This is because it > +is felt that the churn that such changes produce comes at a greater cost > +than the value of such clean-ups. Should we add "conversions to managed APIs"? It's not a recent thing, people do like to post patches doing bulk conversions which bring very little benefit. On the opposite side we could mention that spelling fixes are okay. Not sure if that would muddy the waters too much..
On Mon, Oct 07, 2024 at 08:24:30AM -0700, Jakub Kicinski wrote: > On Fri, 04 Oct 2024 10:49:53 +0100 Simon Horman wrote: > > The purpose of this section is to document what is the current practice > > regarding clean-up patches which address checkpatch warnings and similar > > problems. I feel there is a value in having this documented so others > > can easily refer to it. > > > > Clearly this topic is subjective. And to some extent the current > > practice discourages a wider range of patches than is described here. > > But I feel it is best to start somewhere, with the most well established > > part of the current practice. > > > > -- > > I did think this was already documented. And perhaps it is. > > But I was unable to find it after a quick search. > > Thanks a lot for documenting it, this is great! > All the suggestions below are optional, happy to merge as is. > > > +Clean-Up Patches > > +~~~~~~~~~~~~~~~~ > > nit: other sections use sentence-like capitalization (only capitalizing > the first word), is that incorrect? Or should we ay "Clean-up patches" > here? I think we should be consistent here (I'm intentionally avoiding answering what is correct :) > > > +Netdev discourages patches which perform simple clean-ups, which are not in > > +the context of other work. For example addressing ``checkpatch.pl`` > > +warnings, or :ref:`local variable ordering<rcs>` issues. This is because it > > +is felt that the churn that such changes produce comes at a greater cost > > +than the value of such clean-ups. > > Should we add "conversions to managed APIs"? It's not a recent thing, > people do like to post patches doing bulk conversions which bring very > little benefit. Well yes, I agree that is well established, and a common target of patches. But isn't that covered by the previous section? "Using device-managed and cleanup.h constructs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "Netdev remains skeptical about promises of all “auto-cleanup” APIs, including even devm_ helpers, historically. They are not the preferred style of implementation, merely an acceptable one. ... https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs We could merge or otherwise rearrange that section with the one proposed by this patch. But I didn't feel it was necessary last week. > On the opposite side we could mention that spelling fixes are okay. > Not sure if that would muddy the waters too much.. I think we can and should. Perhaps another section simply stating that spelling (and grammar?) fixes are welcome.
On Mon, 7 Oct 2024 16:55:21 +0100 Simon Horman wrote: > > > +Netdev discourages patches which perform simple clean-ups, which are not in > > > +the context of other work. For example addressing ``checkpatch.pl`` > > > +warnings, or :ref:`local variable ordering<rcs>` issues. This is because it > > > +is felt that the churn that such changes produce comes at a greater cost > > > +than the value of such clean-ups. > > > > Should we add "conversions to managed APIs"? It's not a recent thing, > > people do like to post patches doing bulk conversions which bring very > > little benefit. > > Well yes, I agree that is well established, and a common target of patches. > But isn't that covered by the previous section? > > "Using device-managed and cleanup.h constructs > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > "Netdev remains skeptical about promises of all “auto-cleanup” APIs, > including even devm_ helpers, historically. They are not the preferred > style of implementation, merely an acceptable one. > > ... > > https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs > > We could merge or otherwise rearrange that section with the one proposed by > this patch. But I didn't feel it was necessary last week. Somewhat, we don't push back on correct use of device-managed APIs. But converting ancient drivers to be device-managed just to save 2 or 3 LoC is pointless churn. Which in my mind falls squarely under the new section, the new section is intended for people sending trivial patches. > > On the opposite side we could mention that spelling fixes are okay. > > Not sure if that would muddy the waters too much.. > > I think we can and should. Perhaps another section simply stating > that spelling (and grammar?) fixes are welcome. Hm, dunno, for quotability I'd have a weak preference for a single section describing what is and isn't acceptable as a standalone cleanup.
On Mon, Oct 07, 2024 at 09:08:28AM -0700, Jakub Kicinski wrote: > On Mon, 7 Oct 2024 16:55:21 +0100 Simon Horman wrote: > > > > +Netdev discourages patches which perform simple clean-ups, which are not in > > > > +the context of other work. For example addressing ``checkpatch.pl`` > > > > +warnings, or :ref:`local variable ordering<rcs>` issues. This is because it > > > > +is felt that the churn that such changes produce comes at a greater cost > > > > +than the value of such clean-ups. > > > > > > Should we add "conversions to managed APIs"? It's not a recent thing, > > > people do like to post patches doing bulk conversions which bring very > > > little benefit. > > > > Well yes, I agree that is well established, and a common target of patches. > > But isn't that covered by the previous section? > > > > "Using device-managed and cleanup.h constructs > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > "Netdev remains skeptical about promises of all “auto-cleanup” APIs, > > including even devm_ helpers, historically. They are not the preferred > > style of implementation, merely an acceptable one. > > > > ... > > > > https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs > > > > We could merge or otherwise rearrange that section with the one proposed by > > this patch. But I didn't feel it was necessary last week. > > Somewhat, we don't push back on correct use of device-managed APIs. > But converting ancient drivers to be device-managed just to save > 2 or 3 LoC is pointless churn. Which in my mind falls squarely > under the new section, the new section is intended for people sending > trivial patches. Thanks, I can try and work with that. Do you want to call out older drivers too? I was intentionally skipping that for now. But I do agree it should be mentioned at some point. > > > On the opposite side we could mention that spelling fixes are okay. > > > Not sure if that would muddy the waters too much.. > > > > I think we can and should. Perhaps another section simply stating > > that spelling (and grammar?) fixes are welcome. > > Hm, dunno, for quotability I'd have a weak preference for a single > section describing what is and isn't acceptable as a standalone cleanup. Sure.
On Mon, 7 Oct 2024 17:15:01 +0100 Simon Horman wrote: > > > We could merge or otherwise rearrange that section with the one proposed by > > > this patch. But I didn't feel it was necessary last week. > > > > Somewhat, we don't push back on correct use of device-managed APIs. > > But converting ancient drivers to be device-managed just to save > > 2 or 3 LoC is pointless churn. Which in my mind falls squarely > > under the new section, the new section is intended for people sending > > trivial patches. > > Thanks, I can try and work with that. Do you want to call out older drivers > too? I was intentionally skipping that for now. But I do agree it should > be mentioned at some point. What is and isn't considered old may be hard to determine. I hope that your existing "not part of other work" phrasing will give us the same effect, as there's usually no other work for old drivers.
On Mon, Oct 07, 2024 at 09:54:12AM -0700, Jakub Kicinski wrote: > On Mon, 7 Oct 2024 17:15:01 +0100 Simon Horman wrote: > > > > We could merge or otherwise rearrange that section with the one proposed by > > > > this patch. But I didn't feel it was necessary last week. > > > > > > Somewhat, we don't push back on correct use of device-managed APIs. > > > But converting ancient drivers to be device-managed just to save > > > 2 or 3 LoC is pointless churn. Which in my mind falls squarely > > > under the new section, the new section is intended for people sending > > > trivial patches. > > > > Thanks, I can try and work with that. Do you want to call out older drivers > > too? I was intentionally skipping that for now. But I do agree it should > > be mentioned at some point. > > What is and isn't considered old may be hard to determine. I hope that > your existing "not part of other work" phrasing will give us the same > effect, as there's usually no other work for old drivers. I agree that would be very subjective. And your point about the presence of other work is well made. I'll work on v2 based on this short discussion. Thanks.
diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst index c9edf9e7362d..da9980ad0c57 100644 --- a/Documentation/process/maintainer-netdev.rst +++ b/Documentation/process/maintainer-netdev.rst @@ -355,6 +355,8 @@ just do it. As a result, a sequence of smaller series gets merged quicker and with better review coverage. Re-posting large series also increases the mailing list traffic. +.. _rcs: + Local variable ordering ("reverse xmas tree", "RCS") ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -391,6 +393,15 @@ APIs and helpers, especially scoped iterators. However, direct use of ``__free()`` within networking core and drivers is discouraged. Similar guidance applies to declaring variables mid-function. +Clean-Up Patches +~~~~~~~~~~~~~~~~ + +Netdev discourages patches which perform simple clean-ups, which are not in +the context of other work. For example addressing ``checkpatch.pl`` +warnings, or :ref:`local variable ordering<rcs>` issues. This is because it +is felt that the churn that such changes produce comes at a greater cost +than the value of such clean-ups. + Resending after review ~~~~~~~~~~~~~~~~~~~~~~
The purpose of this section is to document what is the current practice regarding clean-up patches which address checkpatch warnings and similar problems. I feel there is a value in having this documented so others can easily refer to it. Clearly this topic is subjective. And to some extent the current practice discourages a wider range of patches than is described here. But I feel it is best to start somewhere, with the most well established part of the current practice. -- I did think this was already documented. And perhaps it is. But I was unable to find it after a quick search. Signed-off-by: Simon Horman <horms@kernel.org> --- Documentation/process/maintainer-netdev.rst | 11 +++++++++++ 1 file changed, 11 insertions(+)