Re: [PATCH v2 14/17] PCI: dwc: Check iATU in/outbound ranges setup methods status

From: Serge Semin <fancer.lancer@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: "Serge Semin" <Sergey.Semin@baikalelectronics.ru>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rahul Tanwar" <rtanwar@maxlinear.com>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 14/17] PCI: dwc: Check iATU in/outbound ranges setup methods status
Date: Fri, 20 May 2022 13:36:52 +0300	[thread overview]
Message-ID: <20220520103652.37f6fygtwfmre4ot@mobilestation> (raw)
In-Reply-To: <20220516213506.GA3296584-robh@kernel.org>

On Mon, May 16, 2022 at 04:35:06PM -0500, Rob Herring wrote:
> On Wed, May 04, 2022 at 12:46:35AM +0300, Serge Semin wrote:
> > Let's make the DWC PCIe RC/EP safer and more verbose for the invalid or
> > failed inbound and outbound iATU windows setups. Needless to say that
> > silently ignoring iATU regions setup errors may cause unpredictable
> > errors. For instance if for some reason a cfg or IO window fails to be
> > activated, then any CFG/IO requested won't reach target PCIe devices and
> > the corresponding accessors will return platform-specific random values.
> > 
> > First of all we need to convert dw_pcie_ep_outbound_atu() method to check
> > whether the specified outbound iATU range is successfully setup. That
> > method is called by the pci_epc_ops.map_addr callback. Thus we'll make the
> > EP-specific CPU->PCIe memory mappings saver.
> > 
> > Secondly since the iATU outbound range programming method now returns the
> > operation status, it will be handy to take that status into account in the
> > pci_ops.{map_bus,read,write} methods. Thus any failed mapping will be
> > immediately noticeable by the PCIe CFG operations requesters.
> > 
> > Finally we need to convert the dw_pcie_setup_rc() method to returning the
> > operation status, since the iATU outbound ranges setup procedure may now
> > fail. It will be especially handy in case if the DW PCIe RC DT-node has
> > invalid/unsupported (dma-)ranges property. Note since the suggested
> > modification causes having too wide code indentation, it is reasonable
> > from maintainability and readability points of view to move the outbound
> > ranges setup procedure in the separate function.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   |   9 +-
> >  .../pci/controller/dwc/pcie-designware-host.c | 149 ++++++++++++------
> >  drivers/pci/controller/dwc/pcie-designware.h  |   5 +-
> >  drivers/pci/controller/dwc/pcie-intel-gw.c    |   6 +-
> 

> I worry that this could regress some platforms that happened to work 
> before. But only one way to find out...

You are right. It may especially in the framework of the outbound iATU
windows setup procedure due to the new alignment constraints added in
the previous patch. But in that case the returned failure is well
justified by the consequences of the improper windows setup. Anyway
let's hope the regression won't happen.

> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks.

-Sergey

> 