Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Dec 2022 20:02:22 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Christoph Hellwig <hch@infradead.org>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Vinod Koul <vkoul@kernel.org>, Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Cai Huoqing <cai.huoqing@linux.dev>,
        Jingoo Han <jingoohan1@gmail.com>, Frank Li <Frank.Li@nxp.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
        caihuoqing <caihuoqing@baidu.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
        linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 23/25] PCI: dwc: Restore DMA-mask after MSI-data
 allocation
Message-ID: <20221216170222.5vv6bdfrejesbvs5@mobilestation>
References: <20221214235305.31744-24-Sergey.Semin@baikalelectronics.ru>
 <Y5rJJfZeVqliA5Rg@infradead.org>
 <20221215092721.tvz3hpaql3kotgnu@mobilestation>
 <07ec7610-f1be-9b5c-416d-17781a22427d@arm.com>
 <20221215235218.wsuwy5uckqfxjnb6@mobilestation>
 <Y5wgvdnMWQDxkUd+@infradead.org>
 <20221216093423.4bettdxisserdzsh@mobilestation>
 <Y5w/MkA4N857+AWQ@infradead.org>
 <20221216101827.owq7qpakjduf3rit@mobilestation>
 <22bae859-58ee-80cd-f31b-2313c2e47531@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <22bae859-58ee-80cd-f31b-2313c2e47531@arm.com>
Precedence: bulk

On Fri, Dec 16, 2022 at 02:01:20PM +0000, Robin Murphy wrote:
> On 2022-12-16 10:18, Serge Semin wrote:
> > On Fri, Dec 16, 2022 at 01:49:38AM -0800, Christoph Hellwig wrote:
> > > On Fri, Dec 16, 2022 at 12:34:23PM +0300, Serge Semin wrote:
> > > > What about instead of save/restore pattern I'll just change the
> > > > dma_set_mask_and_coherent() method with the dma_set_coherent_mask()
> > > > function call? It seems cleaner. Like this:
> > > 
> > > > Thus the platform-specific streaming DMA mask would be preserved.
> > > > Since it's PCIe then having the streaming DMA-mask less than 32-bits
> > > > wide is very much improbable. Moreover DW PCIe AXI-interface can be
> > > > synthesize only with one out of two address bus widths: 32 and 64.
> > > 
> > 
> > > Where platform-specific means the dwc subdriver?
> > 
> > Right. I meant the streaming DMA-mask set by the low-level DWC PCIe drivers
> > (like pcie-qcom(-ep)?.c, pcie-bt1.c, etc). It's very much important to
> > have the real DMA-mask (at least the streaming one) set for the eDMA-capable
> > controllers so the DMA-engine clients would work with the best performance.
> > 
> > > Yes, that seems to work.
> > 
> > Ok. I'll just use the direct dma_set_coherent_mask() method here then.
> > 
> > > Alternatively have a flag that says which streaming mask
> > > to set.
> > 
> > I'd prefer to have more flexibility here relying on the low-level
> > drivers to set the mask(s) instead of adding the new flag, just in case
> > if there is vendor-specific IP-core/platform changes in the address
> > bus width.
> 

> Presumably the low-level glue drivers could pass a bus size or mask value in
> struct dw_pcie_rp/dw_pcie, so the actual dma_set_mask() call itself could be
> centralised? I guess there's also an argument that only glue drivers which
> care about eDMA need to care about setting a mask at all, so I don't have a
> string preference either way.

There is another peculiarity here due to which the centralized
approach turns to be less suitable. The dw_pcie_msi_host_init()
method, which currently updates the mask, isn't always executed. It's
run only if pci_msi_enabled() returns true and there is no
platform-specific dw_pci_rp.msi_host_init callback specified. Thus if
we got to implement the centralized DMA-mask setting up procedure then
it should have been done in the always executed place. That code would
have also implied the 32-bit coherent DMA-mask if the generic iMSI-RX
config is required. Thus the iMSI-RX-specific setups (data allocation
and the mask settings) would have been split up into different places
which would be less maintainable.

Moreover as you correctly noted the real DMA-mask setting up is only
needed for the eDMA-capable controllers. Meanwhile in the most of the
cases there is no eDMA embedded in the PCIe controller, but due to the
centralized approach we would need to set some mask anyway. Since I
don't know the real address bus width of all the already available DW
PCIe platforms  we'll have to fallback to using some default mask
value, which might incorrectly describe the actual device capability
(and possible cause some side-effects/regressions).

To sum up weighing up pros and cons the centralized approach seems to
me more complex, less maintainable and less flexible. In my opinion
relying on the glue-drivers to set the mask if it's needed to be set
(that is there is the embedded eDMA) is the best and the simplest
choice.

> If you'd rather stick with that approach then
> it might be worth a brief comment at each site to clarify why the other mask
> is being set from an entirely different place, just in case anyone comes
> along and tries to "fix" it.

Exactly what I was going to do. I'll add a brief comment why the
coherent DMA-mask is updated in the dw_pcie_msi_host_init() method.

-Serge(y)
> 
> Cheers,
> Robin.