linux-kernel - Re: [PATCH v2] PCI: cadence: Refactor driver to use as a core library

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] [day] [month] [year] [list]
Message-ID: <20191101132629.GF9723@e119886-lin.cambridge.arm.com>
Date:   Fri, 1 Nov 2019 13:26:29 +0000
From:   Andrew Murray <andrew.murray@....com>
To:     Tom Joseph <tjoseph@...ence.com>
Cc:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] PCI: cadence: Refactor driver to use as a core library

On Wed, Oct 30, 2019 at 03:03:19PM +0000, Tom Joseph wrote:
> Hi Andrew,
> 
>  I noticed that I missed to respond to your question here.
> 

> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index fe9f9f1..cafbed0 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -48,6 +48,34 @@ config PCIE_CADENCE_EP
> > >  	  endpoint mode. This PCIe controller may be embedded into many
> > >  	  different vendors SoCs.
> > >
> > > +config PCIE_CADENCE_PLAT
> > > +	bool
> > > +
> > > +config PCIE_CADENCE_PLAT_HOST
> > > +	bool "Cadence PCIe platform host controller"
> > > +	depends on OF
> > > +	depends on PCI
> > > +	select IRQ_DOMAIN
> > > +	select PCIE_CADENCE
> > > +	select PCIE_CADENCE_HOST
> > > +	select PCIE_CADENCE_PLAT
> > > +	help
> > > +	  Say Y here if you want to support the Cadence PCIe platform
> > controller in
> > > +	  host mode. This PCIe controller may be embedded into many
> > different
> > > +	  vendors SoCs.
> > > +
> > > +config PCIE_CADENCE_PLAT_EP
> > > +	bool "Cadence PCIe platform endpoint controller"
> > > +	depends on OF
> > > +	depends on PCI_ENDPOINT
> > > +	select PCIE_CADENCE
> > > +	select PCIE_CADENCE_EP
> > > +	select PCIE_CADENCE_PLAT
> > > +	help
> > > +	  Say Y here if you want to support the Cadence PCIe  platform
> > controller in
> > > +	  endpoint mode. This PCIe controller may be embedded into many
> > > +	  different vendors SoCs.
> > > +
> > >  endmenu
> > >
> > >  config PCIE_XILINX_NWL
> > > diff --git a/drivers/pci/controller/Makefile
> > b/drivers/pci/controller/Makefile
> > > index d56a507..676a41e 100644
> > > --- a/drivers/pci/controller/Makefile
> > > +++ b/drivers/pci/controller/Makefile
> > > @@ -2,6 +2,7 @@
> > >  obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence.o
> > >  obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
> > >  obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
> > > +obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
> > >  obj-$(CONFIG_PCI_FTPCI100) += pci-ftpci100.o
> > 
> > I think in addition to the above hunks you also need the following:
> > 
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -28,25 +28,16 @@ config PCIE_CADENCE
> >         bool
> > 
> >  config PCIE_CADENCE_HOST
> > -       bool "Cadence PCIe host controller"
> > +       bool
> >         depends on OF
> > -       depends on PCI
> >         select IRQ_DOMAIN
> >         select PCIE_CADENCE
> > -       help
> > -         Say Y here if you want to support the Cadence PCIe controller in host
> > -         mode. This PCIe controller may be embedded into many different
> > vendors
> > -         SoCs.
> > 
> >  config PCIE_CADENCE_EP
> > -       bool "Cadence PCIe endpoint controller"
> > +       bool
> >         depends on OF
> >         depends on PCI_ENDPOINT
> >         select PCIE_CADENCE
> > -       help
> > -         Say Y here if you want to support the Cadence PCIe  controller in
> > -         endpoint mode. This PCIe controller may be embedded into many
> > -         different vendors SoCs.
> > 
> >  config PCIE_CADENCE_PLAT
> >         bool
> > 
> > I removed the 'depends on PCI' as you get that for free seeing as the
> > "PCI controller drivers" menu depends on PCI.
> > 
> > Removing the help and text prevents anything from being shown in the
> > Kconfig
> > system - I think you need that here to avoid confusing the user (and to make
> > this just like DWC).
> > 
> > I'm happy with the above. Though just like DWC, I find this confusing. This
> > allows future Cadence based controller drivers to include support for the EP
> > or host library by 'selecting PCIE_CADENCE_(HOST,EP)' resulting in those
> > libraries being compiled in. But despite this the user can still unselect
> > PCIE_CADENCE_PLAT_HOST which simply prevents that HOST,EP library
> > functions
> > from being called - i.e. to override and disable that functionality.
> 
> Thanks for the spotting this and for the explanation . I have corrected these in v3.
> > 
> > There is no reason that this needs to look like the DWC Kconfig, if there is
> > a better way that can provide additional benefits then please feel free to
> > change it.


> > > +
> > > +	platform_set_drvdata(pdev, cdns_plat_pcie);
> > > +	if (is_rc) {
> > > +		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_PLAT_HOST))
> > > +			return -ENODEV;
> > 
> > To continue my earlier point, I haven't understood why (in the DWC case)
> > this
> > isn't just CONFIG_PCIE_CADENCE_HOST - i.e. why not a single CONFIG for
> > the HOST
> > (instead of _HOST AND _PLAT_HOST)?
> > 
> 
> My understanding is that, this would be a place where SoC/platform specific code could be inserted.
> It might not be obvious here (as there is nothing much platform specific) , but just for demo purpose.
>  

Thanks for this. 

Thanks,

Andrew Murray