linux-kernel - RE: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

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: <SN7PR12MB7201459AEFB6DFF600E753928BE0A@SN7PR12MB7201.namprd12.prod.outlook.com>
Date:   Mon, 28 Aug 2023 12:01:52 +0000
From:   "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "Simek, Michal" <michal.simek@....com>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>
Subject: RE: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

Hi Bjorn,

> -----Original Message-----
> From: Havalige, Thippeswamy
> Sent: Monday, August 28, 2023 2:39 PM
> To: Bjorn Helgaas <helgaas@...nel.org>
> Cc: linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> pci@...r.kernel.org; devicetree@...r.kernel.org; bhelgaas@...gle.com;
> lpieralisi@...nel.org; robh@...nel.org; kw@...ux.com; Simek, Michal
> <michal.simek@....com>; krzysztof.kozlowski+dt@...aro.org; Gogada,
> Bharat Kumar <bharat.kumar.gogada@....com>
> Subject: RE: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver
> 
> Hi Bjorn,
> 
> Thanks, ll take all your comments and update them in next patch.
> 
> Regards,
> Thippeswamy H
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@...nel.org>
> > Sent: Tuesday, August 22, 2023 1:44 AM
> > To: Havalige, Thippeswamy <thippeswamy.havalige@....com>
> > Cc: linux-arm-kernel@...ts.infradead.org;
> > linux-kernel@...r.kernel.org; linux- pci@...r.kernel.org;
> > devicetree@...r.kernel.org; bhelgaas@...gle.com;
> > lpieralisi@...nel.org; robh@...nel.org; kw@...ux.com; Simek, Michal
> > <michal.simek@....com>; krzysztof.kozlowski+dt@...aro.org; Gogada,
> > Bharat Kumar <bharat.kumar.gogada@....com>
> > Subject: Re: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root
> > Port driver
> >
> > On Fri, Aug 18, 2023 at 03:05:07PM +0530, Thippeswamy Havalige wrote:
> > > Add support for Xilinx XDMA Soft IP core as Root Port.
> > >
> > > The Zynq UltraScale+ MPSoCs devices support XDMA soft IP module in
> > > programmable logic.
> > >
> > > The integrated XDMA soft IP block has integrated bridge function
> > > that can act as PCIe Root Port.
> > >
> > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@....com>
> > > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@....com>
> > > ---
> > > changes in v6:
> > > - Replaced chained irq's with regular interrupts.
> >
> > Thanks a million for working this out!
> >
> > Trivial comments below, wait a couple days before reposting in case
> > there are other comments.
> >
> > > +static inline bool xilinx_pl_dma_pcie_link_up(struct pl_dma_pcie
> > > +*port) {
> > > +	return (pcie_read(port, XILINX_PCIE_DMA_REG_PSCR) &
> > > +		XILINX_PCIE_DMA_REG_PSCR_LNKUP) ? 1 : 0;
> >
> > This function returns bool, so I think true/false would be more
> > appropriate than 1/0.
> >
> > > +static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus,
> > > +					    unsigned int devfn)
> > > +{
> > > +	struct pl_dma_pcie *port = bus->sysdata;
> > > +
> > > +	/* Check if link is up when trying to access downstream ports */
> > > +	if (!pci_is_root_bus(bus)) {
> > > +		/*
> > > +		 * Checking whether link is up here is a last line of defence,
> > > +		 * if the link goes down after we check for link-up, we have a
> > > +		 * problem: if a PIO request is initiated while link-down, the
> > > +		 * whole controller hangs, and even after link comes up again,
> > > +		 * previous PIO requests won't work, and a reset of the whole
> > > +		 * PCIe controller is needed. Henceforth we need link-up
> > check
> > > +		 * here to avoid sending PIO request when link is down. This
> > > +		 * check is racy by definition and does not make controller
> > hang
> > > +		 * if the link goes down after this check is performed.
> >
> > This comment doesn't make sense to me.  "If PIO request initiated
> > while link- down, controller hangs ... This check is racy and does not
> > make controller hang if link goes down."  Which is it?
- Here checking link up treats device as invalid.

Please find comment that I ll update in next patch and 
Please letme know if any changes are needed.

  /*
                 * Checking whether the link is up. Here is the last line of
                 * defence. If the link goes down after we check for link-up,
                 * we have a problem. If a PIO request is initiated while link
                 * is down, the whole controller hangs. Even after link comes up
                 * again, previous PIO requests won't work, and a reset of the
                 * whole PCIe controller is needed. Henceforth we need link-up
                 * check here to treat device as invalid and avoid sending PIO
                 * request when link is down and this check is inherently racy
                 * by definition.
*/
> >
> > My *guess* is that this check narrows the window but doesn't close it,
> > so if
> > xilinx_pl_dma_pcie_link_up() finds the link up, but the link goes down
> > before
> > pci_generic_config_read() initiates the PIO request, the controller
> > hangs, and a reset is required.
> >
> > > +		 */
> > > +		if (!xilinx_pl_dma_pcie_link_up(port))
> > > +			return false;
> > > +	} else if (devfn > 0)
> > > +		/* Only one device down on each root port */
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> >
> > > +/* INTx error interrupts are Xilinx controller specific interrupt,
> > > +used to
> > > + * notify user about error's such as cfg timeout, slave unsupported
> > > +requests,
> >
> > s/error's/errors/
> >
> > > + * fatal and non fatal error etc.
> >
> > > +		err = devm_request_irq(dev, irq,
> > xilinx_pl_dma_pcie_intr_handler,
> > > +				       IRQF_SHARED | IRQF_NO_THREAD,
> > intr_cause[i].sym, port);
> >
> > Rewrap to fit in 80 columns.
> >
> > > +	/* Needed for MSI DECODE MODE */
> > > +	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK,
> > XILINX_PCIE_DMA_REG_MSI_LOW_MASK);
> > > +	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK,
> > > +XILINX_PCIE_DMA_REG_MSI_HI_MASK);
> >
> > Rewrap.
> >
> > Bjorn