| Message ID | 153030405971.57832.12860154795039493576.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Jun 29, 2018 at 03:27:39PM -0500, Bjorn Helgaas wrote: > + - Wrap changelogs to fit in 80 columns when shown by "git show", which > + adds 4 spaces. I use "textwidth=75" in vim. I guess the ideal width is subjective, I usually wrap at 72 chars because then you've got 4 blanks on either side when viewed with "git log", which I find neater than maxing out the horizontal width. In some cases I deliberately wrap at less than 72 chars or allow 73 chars if it avoids vastly unequal widths in a paragraph. Often when I later doublecheck what you've committed, I find that you've rewrapped everything to 75 chars and the result doesn't look as neat as I wanted it to be. Not a big deal, but thought I'd mention it now that you're codifying this "rule" somewhat more formally. Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 03:27:39PM -0500, Bjorn Helgaas wrote: > --- /dev/null > +++ b/Documentation/PCI/submitting-patches.txt > @@ -0,0 +1,153 @@ > +Start with Documentation/process/submitting-patches.rst for general > +guidance. > + > +These are things I look at when reviewing patches. For an uninitiated reader who doesn't know that you're currently the (sole) maintainer of the PCI subsystem, this sentence might look odd. Who's "I"? What happens if you onboard co-maintainers, are you going to change this to "we"? > + - Wrap code and comments to fit in 80 columns. Exception: I prefer > + printk strings to be in one piece for searchability, so don't split > + quoted strings to make them fit in 80 columns. This is a duplication of Documentation/process/coding-style.rst, section 2. > + - Follow the existing convention Run "git log --oneline <file>" and make > + your subject line match previous changes in format, capitalization, and > + sentence structure. For example, native host bridge driver patch > + titles look like this: > + > + PCI: vmd: Remove IRQ affinity so we can allocate more IRQs > + PCI: mediatek: Add MSI support for MT2712 and MT7622 > + PCI: rockchip: Remove IRQ domain if probe fails A quick "git log --oneline --no-merges drivers/pci" shows that the prefixes in use aren't consistent at all: Sometimes a slash is used to separate "PCI" from the subpart touched by the patch, sometimes a colon, e.g. "PCI/AER: " versus "PCI: shpchp: ". Your own patches aren't consistent in that respect. Sometimes, only "PCI: " is given as prefix, even though the commit only touches a subpart such as "sysfs", so could easily specify more precisely what it's touching. If you value consistency, it would be good to codify the preferred form right here. > + - Include specific details, e.g., write "Add XYZ controller support" > + instead of "add support for new generation controller". Why not simply "Support XYZ controller"? One word less, more succinct. > + - Always copy linux-pci@vger.kernel.org and linux-kernel@vger.kernel.org. I'd drop linux-kernel here. The volume on that list is already like drinking from a firehose, I doubt it adds much value to cc it. Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 01, 2018 at 07:45:08PM +0200, Lukas Wunner wrote: > On Fri, Jun 29, 2018 at 03:27:39PM -0500, Bjorn Helgaas wrote: > > --- /dev/null > > +++ b/Documentation/PCI/submitting-patches.txt > > @@ -0,0 +1,153 @@ > > +Start with Documentation/process/submitting-patches.rst for general > > +guidance. > > + > > +These are things I look at when reviewing patches. > > For an uninitiated reader who doesn't know that you're currently the > (sole) maintainer of the PCI subsystem, this sentence might look odd. > Who's "I"? What happens if you onboard co-maintainers, are you > going to change this to "we"? You're absolutely right. That was appropriate for email (where I originally posted this) but certainly not for the tree. > > + - Follow the existing convention Run "git log --oneline <file>" and make > > + your subject line match previous changes in format, capitalization, and > > + sentence structure. For example, native host bridge driver patch > > + titles look like this: > > + > > + PCI: vmd: Remove IRQ affinity so we can allocate more IRQs > > + PCI: mediatek: Add MSI support for MT2712 and MT7622 > > + PCI: rockchip: Remove IRQ domain if probe fails > > A quick "git log --oneline --no-merges drivers/pci" shows that the prefixes > in use aren't consistent at all: Sometimes a slash is used to separate > "PCI" from the subpart touched by the patch, sometimes a colon, e.g. > "PCI/AER: " versus "PCI: shpchp: ". Your own patches aren't consistent > in that respect. Sometimes, only "PCI: " is given as prefix, even though > the commit only touches a subpart such as "sysfs", so could easily specify > more precisely what it's touching. > > If you value consistency, it would be good to codify the preferred form > right here. I was trying to make the point that whenever we're doing *anything* in a shared project like Linux, it's better to look at existing practice and follow it than it is to slavishly follow rules. So I purposely didn't enumerate all the cases. I think if you look at logs of individual files, they're pretty consistent. I generally use "PCI/XXX" for things in the core (mostly capabilities like MSI, AER, DPC, etc) and "PCI: xxx:" for drivers (shpchp, pciehp, etc). IIRC, I adopted this from previous practice. Of course there are things I don't apply that are different. And Rafael does most of the PM stuff, so I try to follow *his* convention for that. > > + - Always copy linux-pci@vger.kernel.org and linux-kernel@vger.kernel.org. > > I'd drop linux-kernel here. The volume on that list is already like > drinking from a firehose, I doubt it adds much value to cc it. It's useful as an archive and for people not subscribed to linux-pci who happen to see the occasional thing they want to respond to. Nobody expects to be able to follow everything on LKML. get_maintainer.pl reports LKML for everything, and I'm not trying to innovate by being different. But on reflection, I think the overall value of this writeup is minimal. It's a lot of repetition of things already documented elsewhere and most of it boils down to "pay attention to existing practice and don't do things differently unless you're innovating and adding value." That *should* be obvious, and if it's not, I doubt that adding one more thing to read is going to make it more obvious. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 10:59:46AM -0500, Bjorn Helgaas wrote: > But on reflection, I think the overall value of this writeup is > minimal. It's a lot of repetition of things already documented > elsewhere and most of it boils down to "pay attention to existing > practice and don't do things differently unless you're innovating and > adding value." That *should* be obvious, and if it's not, I doubt > that adding one more thing to read is going to make it more obvious. So my opinion is that your writeup does contain valid points that are worth documenting: For an open source project, a top priority is to attract and retain contributors who improve the bus factor, who keep the code base alive and maintained, thereby avoiding bit rot. Knowledge diffusion, including documentation of best practices and conventions, goes a long way towards that goal. Your writeup was mainly from a maintainer perspective: "consistency makes maintenance easier". But consistency is also valuable from a contributor perspective: It makes it easier to dive into a code base and find your way around, and that includes changelogs in the git history. There are important bits of knowledge in the writeup, if those can be distilled, the result would very much be valuable to have in the tree. Example: > I generally use > "PCI/XXX" for things in the core (mostly capabilities like MSI, AER, > DPC, etc) and "PCI: xxx:" for drivers (shpchp, pciehp, etc). That was in fact unknown to me. If you find it difficult to put yourself in the shoes of a contributor, I could try to rework the document and distill the points I find important. Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2018 at 04:31:36PM +0200, Lukas Wunner wrote: > On Thu, Jul 12, 2018 at 10:59:46AM -0500, Bjorn Helgaas wrote: > > But on reflection, I think the overall value of this writeup is > > minimal. It's a lot of repetition of things already documented > > elsewhere and most of it boils down to "pay attention to existing > > practice and don't do things differently unless you're innovating and > > adding value." That *should* be obvious, and if it's not, I doubt > > that adding one more thing to read is going to make it more obvious. > > So my opinion is that your writeup does contain valid points that are > worth documenting: For an open source project, a top priority is to > attract and retain contributors who improve the bus factor, who keep > the code base alive and maintained, thereby avoiding bit rot. > > Knowledge diffusion, including documentation of best practices and > conventions, goes a long way towards that goal. Your writeup was > mainly from a maintainer perspective: "consistency makes maintenance > easier". But consistency is also valuable from a contributor > perspective: It makes it easier to dive into a code base and find > your way around, and that includes changelogs in the git history. > > There are important bits of knowledge in the writeup, if those can > be distilled, the result would very much be valuable to have in the tree. > > Example: > > > I generally use > > "PCI/XXX" for things in the core (mostly capabilities like MSI, AER, > > DPC, etc) and "PCI: xxx:" for drivers (shpchp, pciehp, etc). > > That was in fact unknown to me. > > If you find it difficult to put yourself in the shoes of a contributor, > I could try to rework the document and distill the points I find important. I definitely want to make it easy and attractive for people to contribute to Linux in general. I'd be glad for any hints. You're right, it's hard for me to put myself in the shoes of a contributor, at least a new one. I guess I'm hesitant because a lot of the things I included there seem like they border on the obsessive, and I don't want to ask contributors to make trivial changes to fit in with my personal quirks. Since they're my quirks, I'd rather just silently fix them up as I apply patches. If there were a wider consensus on some things and they could be folded into Documentation/process/ somehow, that would be ideal, but I'm not sure there would be enough of a consensus, and I don't really want to start a big discussion about it. But maybe there are a few things that wouldn't be controversial, I dunno. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/PCI/00-INDEX b/Documentation/PCI/00-INDEX index 00c9a90b6f38..0f1d1de087f1 100644 --- a/Documentation/PCI/00-INDEX +++ b/Documentation/PCI/00-INDEX @@ -12,6 +12,8 @@ pci.txt - info on the PCI subsystem for device driver authors pcieaer-howto.txt - the PCI Express Advanced Error Reporting Driver Guide HOWTO +submitting-patches.txt + - hints about how to submit PCI patches endpoint/pci-endpoint.txt - guide to add endpoint controller driver and endpoint function driver. endpoint/pci-endpoint-cfs.txt diff --git a/Documentation/PCI/submitting-patches.txt b/Documentation/PCI/submitting-patches.txt new file mode 100644 index 000000000000..d6a694182446 --- /dev/null +++ b/Documentation/PCI/submitting-patches.txt @@ -0,0 +1,153 @@ +Start with Documentation/process/submitting-patches.rst for general +guidance. + +These are things I look at when reviewing patches. Most of the technical +things are obvious or covered elsewhere. Some things here are cosmetic +personal preferences, but consistency makes maintenance easier. I silently +fix up most of the trivial things, so don't get too hung up on the details. + +Write the patch: + + - Make each patch small but complete by itself. Don't worry about making + patches *too* small; it's trivial for me to squash patches together if + necessary. + + - Make sure the kernel builds after every patch. You can't introduce a + problem in one patch and fix it in a subsequent patch. If one of the + autobuilders finds a build issue, I'll revert the patch unless you send + a fix. + + - Please do send whitespace and coding style fixes, but don't mix them + with other substantive changes. It's easier for people to backport + fixes if whitespace changes are at the end of a series. + + - Wrap code and comments to fit in 80 columns. Exception: I prefer + printk strings to be in one piece for searchability, so don't split + quoted strings to make them fit in 80 columns. + + - Follow the existing style for code, names, and indentation. When + you're finished, the file should look like it was all written by one + person. + +Write the changelog title (first line of the changelog): + + - Follow the existing convention Run "git log --oneline <file>" and make + your subject line match previous changes in format, capitalization, and + sentence structure. For example, native host bridge driver patch + titles look like this: + + PCI: vmd: Remove IRQ affinity so we can allocate more IRQs + PCI: mediatek: Add MSI support for MT2712 and MT7622 + PCI: rockchip: Remove IRQ domain if probe fails + + - Write a complete sentence, starting with a capitalized verb. + + - Include specific details, e.g., write "Add XYZ controller support" + instead of "add support for new generation controller". + + - Do not include a trailing period in the title. + +Write the changelog: + + - Make the changelog readable without the title. The changelog is not a + continuation of the title, so it should make sense by itself. Always + include a changelog, even if it is the same as the title. + + - Explain the change (not just "Fix a problem"), but do it as concisely + as possible. Include function names, references to sections of the + spec, URLs for bug reports, etc. This makes reviewing and future + maintenance easier. + + - Capitalize initialisms ("PCI", "IRQ", "ID", "MSI", etc) in all English + text, including title, changelog, and comments. These are usually + written in lower-case in the C code, but please follow normal English + conventions in text. + + - Include "()" after function names and "[]" after array names as a + visual clue that these refer to something in the code. + + - Include dmesg output and stack trace when relevant. Prune details that + aren't relevant, e.g., you can usually remove timestamps and function + addresses. The objective is to concisely illustrate the issue and make + it discoverable by search engines. + + - Use spaces (not tabs) in the changelog because "git log" indents the + changelog and things aligned with tabs won't stay aligned. + + - Wrap changelogs to fit in 80 columns when shown by "git show", which + adds 4 spaces. I use "textwidth=75" in vim. + + - Order tags as suggested by Ingo [1] (extended): + + Fixes: + Link: + Reported-by: + Tested-by: + Signed-off-by: (author) + Signed-off-by: (chain) + Reviewed-by: + Acked-by: + Cc: stable@vger.kernel.org # 3.4+ + Cc: (other) + + - Include a "Fixes:" reference when you're fixing a previous commit and + copy the author of the previous commit. This helps people figure out + where a change needs to be backported. + + - Include specific commit references when possible, e.g., 'e77f847df54c + ("PCI: rockchip: Add Rockchip PCIe controller support")'. I use this + alias to generate them: + + alias gsr='git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"' + + - Include bugzilla URLs if available (kernel.org bugzilla preferred), + e.g., + + Link: https://bugzilla.redhat.com/show_bug.cgi?id=1409201 + + - Include problem report URLs. Use kernel.org URLs, e.g., + http://lkml.kernel.org/r/<Message-ID>, because they don't depend on + other mirror sites: + + Link: http://lkml.kernel.org/r/4bcbcbc1-7c79-09f0-5071-bc2f53bf6574@kernel.dk + + - Include specific references to the spec when possible, e.g., "PCIe + r3.1, sec 7.8.2". If you're talking about something mentioned in the + spec, use the same name and capitalization as the spec. + + - Include a "Cc: stable@vger.kernel.org" tag if you want one, and + indicate which kernels need it. + +Post the patch: + + - Use scripts/get_maintainer.pl to find the maintainers of files you're + changing, and copy the maintainers and authors of recent or related + changes. + + - Always copy linux-pci@vger.kernel.org and linux-kernel@vger.kernel.org. + I don't apply patches that haven't appeared on the linux-pci mailing + list, even if you send them to me directly. This is partly to make + sure everyone has a chance to review it and partly because I use the + Patchwork tracker [2], which only tracks things on the linux-pci list. + + - If you send more than one patch and they're related, always include a + "[0/n]" cover letter. This makes it easy for me to reply to the cover + letter saying "I applied this series." I use "stg -e -v v1 --to=... + patch1..patchN". + + - If you post a new version, please make sure it includes "[v2]" or + similar in the subject line. If it's a series, I want a new version + of the entire series. I don't want updates of individual patches + within the series -- that's too hard for me to keep track of. It's + perfectly fine if some patches in a v2 series are the same as in the + initial posting. + + - If you want the patch in the current release, include a cover letter + and tell me that. Otherwise, I assume all patches are intended for the + next merge window. + + - If you're really gung-ho, you can go to Patchwork [2] and mark your + superseded patches as "Superseded" so I don't have to do that myself. + +[1] http://lkml.kernel.org/r/20120711080446.GA17713@gmail.com +[2] https://patchwork.ozlabs.org/project/linux-pci/list/