Re: 回复: [pci:slot-reset 1/1] drivers/pci/controller/dwc/pcie-dw-rockchip.c:721:58: error: use of undeclared identifier 'PCIE_CLIENT_GENERAL_CON'

[Niklas Cassel <cassel@kernel.org>]

Hello Krzysztof,


FYI: your name appears corrupted in my inbox.

From: Krzysztof Wilczy��ski <your-email-address>

Perhaps because:
Content-Type: text/plain; charset=us-ascii

instead of UTF-8?


On Fri, May 16, 2025 at 01:24:05AM +0900, Krzysztof Wilczy��ski wrote:
> (+Cc Mani for visibility)
> 
> Hello,
> 
> > Please merge it into the following branch; otherwise, this compilation error will occur.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/dw-rockchip
> > 
> > All errors (new ones prefixed by >>):
> > 
> > >> drivers/pci/controller/dwc/pcie-dw-rockchip.c:721:58: error: use of undeclared identifier 'PCIE_CLIENT_GENERAL_CON'
> >      721 |         rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, PCIE_CLIENT_GENERAL_CON);
> >          |                                                                 ^
> >    1 error generated.
> 
> Sorry about this!
> 
> I moved changes from the slot-reset to the controller/dw-rockchip branch,
> to make sure everything has proper dependencies now.  Hopefully, there
> won't be any more issues.


Comparing the commit that landed on the branch, with Wilfred's patch on the
mailing list, I did notice this diff:


diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 9a952871e4d0..c4bd7e0abdf0 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -556,7 +556,7 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
                return ret;
        }
 
-       /* unmask DLL up/down indicator and hot reset/link-down reset irq */
+       /* Unmask DLL up/down indicator and hot reset/link-down reset IRQ. */
        val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED | PCIE_LINK_REQ_RST_NOT_INT, 0);
        rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
 
@@ -747,11 +747,11 @@ static int rockchip_pcie_rc_reset_slot(struct pci_host_bridge *bridge,
 
        ret = pp->ops->init(pp);
        if (ret) {
-               dev_err(dev, "Host init failed: %d\n", ret);
+               dev_err(dev, "host init failed: %d\n", ret);
                goto deinit_clk;
        }
 
-       /* LTSSM enable control mode */
+       /* LTSSM enable control mode. */
        val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
        rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
 
@@ -762,11 +762,11 @@ static int rockchip_pcie_rc_reset_slot(struct pci_host_bridge *bridge,
 
        ret = dw_pcie_setup_rc(pp);
        if (ret) {
-               dev_err(dev, "Failed to setup RC: %d\n", ret);
+               dev_err(dev, "failed to setup RC: %d\n", ret);
                goto deinit_clk;
        }
 
-       /* unmask DLL up/down indicator and hot reset/link-down reset irq */
+       /* Unmask DLL up/down indicator and hot reset/link-down reset IRQ. */
        val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED | PCIE_LINK_REQ_RST_NOT_INT, 0);
        rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
 
@@ -774,9 +774,9 @@ static int rockchip_pcie_rc_reset_slot(struct pci_host_bridge *bridge,
        if (ret)
                goto deinit_clk;
 
-       /* Ignore errors, the link may come up later */
+       /* Ignore errors, the link may come up later. */
        dw_pcie_wait_for_link(pci);
-       dev_dbg(dev, "Slot reset completed\n");
+       dev_dbg(dev, "slot reset completed\n");
        return ret;
 
 deinit_clk:



Reviewing the diff, the changes looks fine to me, but I strongly think
that if the actual code is modified from the submission (rather than
just fixing some minor grammar in the commit message), the (unwritten?)
rule is that the person should add a:

[person: describe modifications from the original submission]

line after the Signed-off-by of the original submission, such that,
e.g. if there is a bug in one of the lines that were changed, the
original author does not unfairly get blamed for some code that he
didn't write.

When only modifying the commit log, there is no possibility that a
functional change was introduced, but as soon as the code/diff is
changed, there is a change that a functional change is introduced
(intentional or not), so there is a big difference between the cases
(in my humble opinion).


Kind regards,
Niklas