Re: [PATCH 0/8] PCI: rockchip: Fix PCIe endpoint controller driver

From: Rick Wertenbroek <rick.wertenbroek@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: alberto.dassatti@heig-vd.ch, xxm@rock-chips.com,
	wenrui.li@rock-chips.com, rick.wertenbroek@heig-vd.ch,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Jani Nikula" <jani.nikula@intel.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Mikko Kovanen" <mikko.kovanen@aavamobile.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH 0/8] PCI: rockchip: Fix PCIe endpoint controller driver
Date: Thu, 26 Jan 2023 16:23:57 +0100	[thread overview]
Message-ID: <CAAEEuhq9X0ppqTMp7fnZapbubf9k8xhH=u3gPva3hEpAdawK3w@mail.gmail.com> (raw)
In-Reply-To: <20230126145200.GA1270846@bhelgaas>

Hello, Bjorn
Thank you for your prompt reply and helpful comments.

Le jeu. 26 janv. 2023 à 15:52, Bjorn Helgaas <helgaas@kernel.org> a écrit :
>
> Hi Rick,
>
> Thanks very much for your work.
>
> On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> > This is a series of patches that fixes the PCIe endpoint controller driver
> > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> >
> > The original driver in mainline had issues and would not allow for the
> > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> > that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> > endpoint.
>
> So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
> PCIe controller") when it actually didn't work?  Ouch.  Thanks for
> fixing it and testing it.

It seems it wasn't fully tested, the code compiles and kernel module loads,
but further functionality didn't seem to have been tested
(e.g., lspci, and with the pcitest tool and pci_endpoit_test_driver).

>
> For the next iteration, can you please update these subject lines and
> commit logs to:

Thank you, I will prepare the changes and add them to the next iteration
with changes from other comments that may arise.

>
>   - Use imperative mood, i.e., read like a command, instead of a past
>     tense description of what was done.  For example, say "Remove
>     writes to unused registers" instead of "Removed writes ..."
>
>   - Be more specific when possible.  "Fix" conveys no information
>     about the actual code change.  For example, "Fixed endpoint
>     controller Configuration Request Retry Status" gives a general
>     idea, but it would be more useful if it said something about
>     clearing config mode after probe.
>
>   - Say what the patch does in the commit log.  The current ones often
>     describe a *problem*, but do not explicitly say what the patch
>     does.  The commit log should be complete in itself even without
>     the subject line, so it usually contains a slightly expanded
>     version of the subject line.
>
>   - Split patches that do more than one logical thing.  The commit log
>     for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
>     but the patch also adds an unrelated check for multi-function
>     devices.

I will. I tried to split everything into the function it was related to, but I
now understand I should split even more so that the commit message
and changes are more tightly linked.

>
>   - If a patch is a fix for an existing issue and may need to be
>     backported, identify the commit that introduced the issue and add
>     "Fixes: " lines.  This helps distros figure out whether and how
>     far to backport patches.

Does this mean I should refer to the commit cf590b078391
("PCI: rockchip: Add EP driver for Rockchip PCIe controller") ?
Because it wasn't working in the first place ?

>
>   - Refer to the device consistently.  I see:
>       RK3399 PCI EP core
>       RK3399 SoC PCIe EP core
>       RK3399 PCIe endpoint core
>     I vote for "RK3399 PCIe Endpoint core".

I agree.

>
> Notes about imperative mood:
>   https://chris.beams.io/posts/git-commit/
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94

Thank you for all the pointers, I'll take them into account for the
next iteration. This is the first time I actually submitted a series of
patches to the LKML so it's all relatively new to me.