linux-kernel - Re: [PATCH v16 08/22] PCI: microchip: Change the argument of plda_pcie_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SHXPR01MB08637281B32AEE455F030081E6F42@SHXPR01MB0863.CHNPR01.prod.partner.outlook.cn>
Date: Thu, 23 May 2024 09:22:01 +0000
From: Minda Chen <minda.chen@...rfivetech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Lorenzo Pieralisi <lpieralisi@...nel.org>, Conor Dooley
	<conor@...nel.org>, Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh+dt@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Thomas
 Gleixner <tglx@...utronix.de>, Daire McNamara <daire.mcnamara@...rochip.com>,
	Emil Renner Berthing <emil.renner.berthing@...onical.com>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer
 Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Philipp
 Zabel <p.zabel@...gutronix.de>, Mason Huo <mason.huo@...rfivetech.com>,
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>, Kevin Xie
	<kevin.xie@...rfivetech.com>
Subject: Re: [PATCH v16 08/22] PCI: microchip: Change the argument of
 plda_pcie_setup_iomems()



> 
> On Thu, May 23, 2024 at 01:09:58AM +0000, Minda Chen wrote:
> > > On Wed, May 22, 2024 at 01:50:57AM +0000, Minda Chen wrote:
> > > > > The patch is OK, but the subject line is not very informative.
> > > > > It should be useful all by itself even without the commit log.
> > > > > "Change the argument of X" doesn't say anything about why we
> > > > > would want to do that.
> > > > >
> > > > > On Thu, Mar 28, 2024 at 05:18:21PM +0800, Minda Chen wrote:
> > > > > > If other vendor do not select PCI_HOST_COMMON, the driver data
> > > > > > is not struct pci_host_bridge.
> > > > >
> > > > > Also, I don't think this is the real problem.  Your
> > > > > PCIE_MICROCHIP_HOST Kconfig selects PCI_HOST_COMMON, and the
> > > driver
> > > > > calls pci_host_common_probe(), so the driver wouldn't even build
> > > > > without PCI_HOST_COMMON.
> > > > >
> > > > > This patch is already applied and ready to go, but if you can
> > > > > tell us what's really going on here, I'd like to update the commit log.
> > > > >
> > > > It is modified for Starfive code. Starfive JH7110 PCIe do not
> > > > select PCI_HOST_COMMON
> > > > plda_pcie_setup_iomems() will be changed to common plda code.
> > > >
> > > > I think I can modify the title and commit log like this.
> > > >
> > > > Title:
> > > > PCI: microchip: Get struct pci_host_bridge pointer from platform
> > > > code
> > > >
> > > > Since plda_pcie_setup_iomems() will be a common PLDA core driver
> > > > function, but the argument0 is a struct platform_device pointer.
> > > > plda_pcie_setup_iomems() actually using struct pci_host_bridge
> > > > pointer other than platform_device pointer. Further more if a new
> > > > PLDA core PCIe driver do not select PCI_HOST_COMMON, the platform
> > > > driver data is not struct pci_host_bridge pointer. So get struct
> > > > pci_host_bridge pointer from platform code function
> > > > mc_platform_init() and make it to be an argument of
> > > > plda_pcie_setup_iomems().
> > >
> > > OK, I see what you're doing.  This actually has nothing to do with
> > > whether PCI_HOST_COMMON is *enabled*.  It has to do with whether
> > > drivers use pci_host_common_probe().  Here's what I propose:
> > >
> > >   PCI: plda: Pass pci_host_bridge to plda_pcie_setup_iomems()
> > >
> > >   plda_pcie_setup_iomems() needs the bridge->windows list from struct
> > >   pci_host_bridge and is currently used only by pcie-microchip-host.c.
> This
> > >   driver uses pci_host_common_probe(), which sets a pci_host_bridge as
> the
> > >   drvdata, so plda_pcie_setup_iomems() used platform_get_drvdata() to
> find
> > >   the pci_host_bridge.
> > >
> > >   But we also want to use plda_pcie_setup_iomems() in the new
> pcie-starfive.c
> > >   driver, which does not use pci_host_common_probe() and will have struct
> > >   starfive_jh7110_pcie as its drvdata, so pass the pci_host_bridge directly
> > >   to plda_pcie_setup_iomems() so it doesn't need platform_get_drvdata()
> to
> > >   find it.
> > >
> > OK, Thanks.
> >
> > I see PCIe 6.10 changed have been merged to main line.
> > Should I resend this patch set base on 6.10-rc1?
> 
> No need, I rebased it to f0bae243b2bc ("Merge tag 'pci-v6.10-changes'
> of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci") already and it will be a
> trivial rebase to v6.10-rc1 next week.
> 
> The current pci/controller/microchip branch is at:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=ed261441e22
> 4
> Let me know if anything is missing from there.  I can't merge it into linux-next
> until v6.10-rc1 is tagged, but as soon as it is, I'll put it in linux-next.
> 
I have checked the code and tested it with Starfive VisionFive v2 board, PCIe can work.
All are Okay. Thanks for correct the commit message. It is more elegant.

This is precious experience of developing refactor code. Thank all!

Hi Conor
   Thanks for help us for this patch set !
   I see mars dts have been merged to mainline, this version dts patch can not be merged
   I will resend the dts patch on v6.10-rc1.